Details

Reviewers

bb
elexis

Commits

rP19354: Add barter buttons to the trade window for quicker access and to support mods…

Trac Tickets

#4366

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

Start a new game session (recommended to start with "High" or greater starting resources for later steps)
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
Go to the Town Phase (use cheats if you like) and build a Market
Open the Trade Window
- This time, the icons do appear.
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.
With the market still selected, open the Trade Window
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)
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

Branch

barterIconShift

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 364
Build 587: Vulcan Build	Jenkins
Build 585: arc lint + arc unit

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.

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	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.

Harbormaster completed remote builds in B364: Diff 347.Jan 26 2017, 1:58 AM

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	\n
binaries/data/mods/public/gui/session/menu.js
31	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	/** * How many resources to sell when pressing the massbarter hotkey? */
33	/* * Names of the GUI buttons in the bartering panel? */
500	Moving those four variables above the function seems fine, as they will be hoisted either way
532	Ups
572	(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	This function is used once and contains two statements, seems pointless
594	`traders` seems to broad, how about `traderCountText`?
610	Index not used -> for ... of
binaries/data/mods/public/gui/session/selection_panels.js
110	Remove braces
117	stuff -> code
1191	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	Trailing space
148	Dash between variable name and description
150	That param doesnt exist, does it?
159	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	Trailing space
209	to barter anything
binaries/data/mods/public/gui/session/top_panel/button_trade.xml
8	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	Happen to recall why we need the z index?
33	Wondering if singular is better suited. Caps indeed have a better appearance
37	Did you try with 8? The number should be bound to the actual GUI limit
43	Properties should be ordered by relevance, name > type > (size, style, sprite) > (ghost / z)
63	Unused name can be removed
71	A translation comment that it's shown in the trading goods selection wouldn't hurt
binaries/data/mods/public/simulation/components/GuiInterface.js
153	unneeded comment
571	Ack
1954	Unused? Nucular?

This revision now requires changes to proceed.Feb 6 2017, 4:54 PM

@elexis

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	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	Twice. `session.js@777`
610	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	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	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	Singular implies there can only ever be one. This isn't highlander.
37	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	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	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	Not familiar with "Nucular", assuming you meant "Nuclear" and that you're asking me to nuke it. 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.

Harbormaster completed remote builds in B535: Diff 537.Feb 16 2017, 7:46 PM

elexis mentioned this in D117: Add spying to the game.Feb 22 2017, 4:24 AM

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	Don't forget that a sentence ends with a period :P
559	perhaps add whitespaces here, while on it, same twice below
binaries/data/mods/public/gui/session/selection_panels_helpers.js
1	This global is only used once, in selection_panels, could be inlined, but meh
4	It seems all these 2 globals are unused, nuke?
binaries/data/mods/public/simulation/components/tests/test_GuiInterface.js
62	could become arrow function, but seeing surrounding code: mehh

Failed hunks in three files unfortunately

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	The same could be applied to comments left whilst reviewing... ?
binaries/data/mods/public/gui/session/selection_panels_helpers.js
4	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
351	@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	It indeed could, but I'll leave it as-is to maintain consistency with the style of the rest of the file.

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.

Harbormaster completed remote builds in B662: Diff 685.Mar 2 2017, 9:54 PM

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
120–121	These comments probably belong better in function definition (where they are already), but idc.
binaries/data/mods/public/gui/session/selection_panels_helpers.js
173	Use more periods :P : `i.e.`
208	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.
214	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
351	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..."

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	Adding Currently selected resource type to sell in the barter GUI.
523	Misses warn + break; if the mod specifies too many resources. Prints weird error stack otherwise.
609	}; -> }
binaries/data/mods/public/gui/session/selection_panels.js
110–111	`rowLength` -> rowLength
117–118	comment redundant with the jsdoc comment on the function
120–121	that what bb said
123–126	unneeded comment IMO
binaries/data/mods/public/gui/session/selection_panels_helpers.js
143	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`.
208	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
351	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	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

Closed by commit rP19354: Add barter buttons to the trade window for quicker access and to support mods… (authored by elexis). · Explain WhyMar 28 2017, 4:34 AM

This revision was automatically updated to reflect the committed changes.

wowgetoffyourcellphone added a subscriber: wowgetoffyourcellphone.Mar 28 2017, 9:29 AM

Copy Barter UI to Trade Panel
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 347

binaries/data/config/default.cfg

binaries/data/mods/public/gui/session/hotkeys/misc.xml

binaries/data/mods/public/gui/session/menu.js

binaries/data/mods/public/gui/session/selection_panels.js

binaries/data/mods/public/gui/session/selection_panels_helpers.js

binaries/data/mods/public/gui/session/session.js

binaries/data/mods/public/gui/session/top_panel/button_trade.xml

binaries/data/mods/public/gui/session/trade_window.xml

binaries/data/mods/public/gui/session/unit_actions.js

binaries/data/mods/public/simulation/components/GuiInterface.js

binaries/data/mods/public/simulation/components/tests/test_GuiInterface.js

Copy Barter UI to Trade PanelClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 347

binaries/data/config/default.cfg

binaries/data/mods/public/gui/session/hotkeys/misc.xml

binaries/data/mods/public/gui/session/menu.js

binaries/data/mods/public/gui/session/selection_panels.js

binaries/data/mods/public/gui/session/selection_panels_helpers.js

binaries/data/mods/public/gui/session/session.js

binaries/data/mods/public/gui/session/top_panel/button_trade.xml

binaries/data/mods/public/gui/session/trade_window.xml

binaries/data/mods/public/gui/session/unit_actions.js

binaries/data/mods/public/simulation/components/GuiInterface.js

binaries/data/mods/public/simulation/components/tests/test_GuiInterface.js

Copy Barter UI to Trade Panel
ClosedPublic
Actions

Revision Contents
Changeset List