Page MenuHomeWildfire Games

Copy Barter UI to Trade Panel
ClosedPublic

Authored by s0600204 on Jan 23 2017, 11:31 PM.

Details

Summary

Since #3934, the simulation of 0ad has supported adding further resources. This presents a problem with parts of the session gui that are limited at four resources due to the size of their parent GUI objects.

This revision focuses on the Barter Panel. Please see the original ticket on trac (#4366) for previous discussion.

Test Plan
  1. Start a new game session (recommended to start with "High" or greater starting resources for later steps)
  2. Open the Trade Window from the top of the session gui
    • Note that there's space reserved for the icons, but as you the player have no markets, that the icons do not yet appear
  3. Go to the Town Phase (use cheats if you like) and build a Market
  4. Open the Trade Window
    • This time, the icons do appear.
  5. Select the newly built Market
    • If you have four or fewer barter-able resources, the icons should still appear in the Barter Panel.
    • If you happen to have more than four barter-able resources, the Barter Panel should not appear.
  6. With the market still selected, open the Trade Window
  7. Select a different resource in the trade window (or the panel, if applicable)
    • The icons in the trade window should reflect the new selection (with the barter panel, if showing, changing at the same time)
  8. Barter a resource
    • Regardless of whether you barter in the Panel or the Window, the resource should be bartered correctly
    • Both the panel and the window should show the same new barter value for the bought resource

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
  • Compatibility of the proposed barter dialog with a barter feature that would depend on the market entity ('supermarket') is also given, because the dialog could then contain a dropdown with the list of unique market types. The barter selection panel also doesn't conflict as it can still depend on the selected entity,)
  • Agree with bb that you should add a openTrade() to updateGUIObjects() if g_IsTradeOpen (since the strings depend on the number of idlers which depends on the most recent simulation state, and updateGUIObjects is called only onSimulationUpdate) i. It is only called once per turn and doesn't really cost performance, the things are drawn once per frame anyway.
s0600204 updated this revision to Diff 347.EditedJan 26 2017, 1:13 AM

Changes in response to initial comments

  • Match global, rather than file's internal, code style for consts
  • Use => functions for Trade buttons
  • Update trader strings on simulation update
  • Update both sets of barter buttons on press of any "Sell {x}" button, rather than just the set that button belonged to
s0600204 marked 2 inline comments as done.Jan 26 2017, 1:24 AM
s0600204 added inline comments.
binaries/data/mods/public/gui/session/menu.js
532 ↗(On Diff #315)

Copy-paste error in resource agnostic work. Blame elexis for not catching it ;P

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/249/ for more details.

elexis requested changes to this revision.Feb 6 2017, 4:54 PM

Mostly nitpicking.
Doubting that we can fit 8 resources in the barter dialog.
Dynamic resizing could be done, but we could also just resize to guarantee a high number of resources like 8 or 12

binaries/data/mods/public/gui/session/hotkeys/misc.xml
78 ↗(On Diff #347)

\n

binaries/data/mods/public/gui/session/menu.js
532 ↗(On Diff #315)

Ups

31 ↗(On Diff #347)

While at it, why not add a JSdoc comment explaining what the variables are about?
This one: Minimum amount of resources that can be bartered in a single step.

How about adding a default.cfg entry (and the multiplier) under gui.session.barter, so that players could edit it (we could provide options entries for that once we have more space in that page)?

Preventing players from bartering NaN resources is in the scope of D66 and should be done in the sim, Still if adding the cfg entry, make sure players cant shoot themselves into the foot.

32 ↗(On Diff #347)
/**
  * How many resources to sell when pressing the massbarter hotkey?
 */
33 ↗(On Diff #347)
/*
  * Names of the GUI buttons in the bartering panel?
 */
500 ↗(On Diff #347)

Moving those four variables above the function seems fine, as they will be hoisted either way

572 ↗(On Diff #347)

(Interesting, thought this updateTradeButtons call was unneeded as only the simupdates can change the actual probability, but the copy of the simstate data received by the guiinterface call is modified by the GUI, keeping it up to date instantaneously, good stuff)

591 ↗(On Diff #347)

This function is used once and contains two statements, seems pointless

594 ↗(On Diff #347)

traders seems to broad, how about traderCountText?

610 ↗(On Diff #347)

Index not used -> for ... of

binaries/data/mods/public/gui/session/selection_panels.js
110 ↗(On Diff #347)

Remove braces

117 ↗(On Diff #347)

stuff -> code

1191 ↗(On Diff #347)

Slightly misleading, since the context here is determining the order and if there is a comment here,
it should explain why the order is exactly this way.

So the only reason Barter appears first could be a garrisonable market or a market with alert button.

Not changing the comment should be good enough.

binaries/data/mods/public/gui/session/selection_panels_helpers.js
147 ↗(On Diff #347)

Trailing space

148 ↗(On Diff #347)

Dash between variable name and description

150 ↗(On Diff #347)

That param doesnt exist, does it?

159 ↗(On Diff #347)

Seems more clean to me to move this and his brother in the other function to init, even if that's in a different file

177 ↗(On Diff #347)

Trailing space

209 ↗(On Diff #347)

to barter anything

binaries/data/mods/public/gui/session/top_panel/button_trade.xml
8 ↗(On Diff #347)

Don't think we need this TODO. If its too ugly, then artists will notice that from seeing the image, but not reading the XML files

binaries/data/mods/public/gui/session/trade_window.xml
7 ↗(On Diff #347)

Happen to recall why we need the z index?

33 ↗(On Diff #347)

Wondering if singular is better suited. Caps indeed have a better appearance

37 ↗(On Diff #347)

Did you try with 8? The number should be bound to the actual GUI limit

43 ↗(On Diff #347)

Properties should be ordered by relevance, name > type > (size, style, sprite) > (ghost / z)

63 ↗(On Diff #347)

Unused name can be removed

71 ↗(On Diff #347)

A translation comment that it's shown in the trading goods selection wouldn't hurt

binaries/data/mods/public/simulation/components/GuiInterface.js
153 ↗(On Diff #347)

unneeded comment

571 ↗(On Diff #347)

Ack

1954 ↗(On Diff #347)

Unused? Nucular?

This revision now requires changes to proceed.Feb 6 2017, 4:54 PM
s0600204 marked 21 inline comments as done.Feb 16 2017, 7:02 PM

Doubting that we can fit 8 resources in the barter dialog.
Dynamic resizing could be done, but we could also just resize to guarantee a high number of resources like 8 or 12

Dynamic resizing is already added by this patch. Here's what it looks like with six resources:


(You didn't think it odd that I reduced the width of the panel as set in XML from 268 to 160 pixels?)

binaries/data/mods/public/gui/session/menu.js
31 ↗(On Diff #347)

I'll add JSDoc comments. But I feel that adding default.cfg entries (and the required input validation code) is beyond the scope of this ticket/revision.

591 ↗(On Diff #347)

Twice. session.js@777

610 ↗(On Diff #347)

Um, what? The index is used. A for..in could be used instead, but not a for..of.

binaries/data/mods/public/gui/session/selection_panels_helpers.js
148 ↗(On Diff #347)

The examples in the code convention guidelines lack a dash, and should be updated if you wish to enforce that.

binaries/data/mods/public/gui/session/trade_window.xml
7 ↗(On Diff #347)

When I first added the barter icons to the barter dialog, It made it taller. Although I decreased (or increased, depending on how you look at it) the top size value by the same amount as I increased the bottom, on min supported resolution this made the bottom of the dialog be covered by the entity selection gui at the bottom. I solved that by moving the dialog up the screen, but didn't get round to removing the z.

33 ↗(On Diff #347)

Singular implies there can only ever be one. This isn't highlander.

37 ↗(On Diff #347)

Not sure what you mean. 8 was chosen as it is double the current number of resources in "vanilla" 0ad providing some headroom for modders (as well as being a nice round number (from a programmers/binary perspective)). It will work as well as any other integer (that is greater than 4) placed here.

43 ↗(On Diff #347)

This should be mentioned in the code convention guidelines. It's kinda pointless to have written guidelines if they don't match what's expected from contributors.

71 ↗(On Diff #347)

Yes, we certainly don't want translators getting confused and translating it as if it was "Goods" as in "Good and Evil".

Would be funny though.

binaries/data/mods/public/simulation/components/GuiInterface.js
1954 ↗(On Diff #347)

Not familiar with "Nucular", assuming you meant "Nuclear" and that you're asking me to nuke it. Done.

s0600204 updated this revision to Diff 537.Feb 16 2017, 7:02 PM
s0600204 edited edge metadata.
s0600204 marked 5 inline comments as done.

Updates in response to comments from elexis on Phabricator

Notably:

  • Comment style update
  • XML GUI attribute order
  • Nuke unused function added in an earlier commit on this revision

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/364/ for more details.

bb edited edge metadata.Feb 22 2017, 6:39 PM

IIRC, I suggested adding a help button on the barter panel (like the trade panel has now). It could be decided to add this later, but I think it is good to add it in same commit.

binaries/data/mods/public/gui/session/menu.js
42 ↗(On Diff #537)

Don't forget that a sentence ends with a period :P

573 ↗(On Diff #537)

perhaps add whitespaces here, while on it, same twice below

binaries/data/mods/public/gui/session/selection_panels_helpers.js
1 ↗(On Diff #537)

This global is only used once, in selection_panels, could be inlined, but meh

4 ↗(On Diff #537)

It seems all these 2 globals are unused, nuke?

binaries/data/mods/public/simulation/components/tests/test_GuiInterface.js
62 ↗(On Diff #537)

could become arrow function, but seeing surrounding code: mehh

elexis added a comment.Mar 1 2017, 5:07 PM

Failed hunks in three files unfortunately

s0600204 updated this revision to Diff 685.Mar 2 2017, 8:38 PM

Rebase for @elexis, and add punctuation to the end of JS comments and a help button for @bb.

In a politer parallel universe, @elexis wrote:

We've had recent commits that have affected the files you've altered, causing failed hunks and preventing a clean apply. Could I request a rebase, please?

Sure!

binaries/data/mods/public/gui/session/menu.js
42 ↗(On Diff #537)

The same could be applied to comments left whilst reviewing... 😛

binaries/data/mods/public/gui/session/selection_panels_helpers.js
4 ↗(On Diff #537)

These constants (including GATE_ACTIONS above) have nothing to do with the intent of this revision. As such, removing/relocating them is out of context for this patch.

binaries/data/mods/public/gui/session/session.js
403 ↗(On Diff #685)

@bb (or anyone), feel free to suggest better text if this is not sufficiently clear

binaries/data/mods/public/simulation/components/tests/test_GuiInterface.js
62 ↗(On Diff #537)

It indeed could, but I'll leave it as-is to maintain consistency with the style of the rest of the file.

Vulcan added a comment.Mar 2 2017, 9:54 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/454/ for more details.

bb added a comment.Mar 3 2017, 11:41 AM

When opening the barter dialog for the first time the info button flashes and gets removed on sim update. I guess that can be solved with disabling him in barterOpenCommon. Note that this also happens with the barter buttons themselfs when opening the panel the first time after your last market is destroyed, so I guess whole barter thingy needs to be dis/en-abled in barterOpenCommon.

binaries/data/mods/public/gui/session/selection_panels.js
115 ↗(On Diff #685)

These comments probably belong better in function definition (where they are already), but idc.

binaries/data/mods/public/gui/session/selection_panels_helpers.js
203 ↗(On Diff #685)

Use more periods :P : i.e.

238 ↗(On Diff #685)

These (L238+L230) comments are confusing, imo they should also point at the difference in the code. Perhaps Select colour for the buy button of this (resourceCode) recource. and Select colour for sell buttons. are better.

244 ↗(On Diff #685)

Also on sim update the currently selected resources is updated 4 times (once for each resource), don't know how hard it is to fix that, perhaps just place a comment about it.

binaries/data/mods/public/gui/session/session.js
403 ↗(On Diff #685)

I did remove the braces as it is not unimportant/irrelevant information, but seeing the other info string it could be left like this too.

Besides that: "Upon each press *on* one of..."

elexis accepted this revision.EditedMar 28 2017, 4:27 AM

Addresses all comments and design suggestions from many months ago (2016-09-05-QuakeNet-%230ad-dev.log not removing the nice selection panels in vanilla while adding support for mods with many resources and keeping support for market-dependent barterprices).
Tested with 10 resources, works nicely.
Not sure why bb didn't press the accept button. IMO there is no reason to wait pressing it, even when having few comment optimizations suggested.
Sorry that it took a while again, requiring you to rebase the patch.
Thanks for the feature!

binaries/data/mods/public/gui/session/menu.js
45 ↗(On Diff #685)

Adding

  • Currently selected resource type to sell in the barter GUI.
602 ↗(On Diff #685)

Misses warn + break; if the mod specifies too many resources. Prints weird error stack otherwise.

688 ↗(On Diff #685)

}; -> }

binaries/data/mods/public/gui/session/selection_panels.js
105 ↗(On Diff #685)

rowLength -> rowLength

112 ↗(On Diff #685)

comment redundant with the jsdoc comment on the function

115 ↗(On Diff #685)

that what bb said

118 ↗(On Diff #685)

unneeded comment IMO

binaries/data/mods/public/gui/session/selection_panels_helpers.js
173 ↗(On Diff #685)

The same thing that happened in D108 should happen to this file. These 170 lines of unrelated code should be moved to the related code.
The barter functions are used outside of selection_panels.js, so this file seems wrong to me.
Moving to menu.js.

238 ↗(On Diff #685)

Agree with bb. I had to test the code to understand what this code actually does.

Select color of the sell button
Select color of the buy button
seems better to me. If we want to know when which color is chosen, we can still decipher the code instead of deciphering the comments

binaries/data/mods/public/gui/session/session.js
403 ↗(On Diff #685)

On first sight agreed with bb that the parenthesis can be removed, on second sight agreed with bb that we can keep it for consistency, on third sight removing those parenthesis as the other string should have them removed too.

Beisides, inserted that "one" as correctly suggested by bb.

binaries/data/mods/public/gui/session/trade_window.xml
37 ↗(On Diff #347)

Right, the dialog resizes when increasing that number. Guess 1024x768 dictates the limit. That one works even with 10, so bumping (perhaps more, didn't test with 12)

This revision is now accepted and ready to land.Mar 28 2017, 4:27 AM
This revision was automatically updated to reflect the committed changes.