[PATCH] Move barter UI to trade panel

[elexis]

Since #3934 0 A.D. supports adding and removing resources. However the current barter panel has space for at most 4 resources. The 2 most recent commits in ​https://github.com/s0600204/0ad/commits/barterIconShift move the bartering UI from the bottom panel to the trade dialog, alleviating this issue: On first sight it might seem like a usability penalty, but on second sight it doesn't. Currently, in order to barter one has to either recall where the market was built or recall how a market model looks and select it before being able to access the barter UI. With the patch, one can skip the first part, so it might be faster. On the other hand if one knows where a market was built (good practice: market at well known locations like besides CC), opening the trade dialog requires more mouse movement (distance in pixels). A hotkey to open the trade dialog would solve that issue again.

[s0600204]

Changes in one single patch, recently rebased (r19015, 36247e)

[Vladislav Belov]

Thoughts:

• probably we need disabled state for an image, instead of changing the sprite every update :P
• menu.js:631 empty line
• resCode is resourceCode and a in for (let a of ["Buy", "Sell"]) is action?

[Stan]

Also why is there a var at the top of the file ?

[Imarok]

Replying to stanislas69:

Also why is there a var at the top of the file ?

Because you cannot/shouldn't use let for globals.

[bb]

Menu.js: Use g_Foo for globals L631-2 double newline let a be some more descriptive Don't use ["Buy", "Sell"] when you have a const. L638,646,654 inconsistent use of whitespace L645,653 trailing comma

Trade Window.xml L22 38+84=122 L68 38+32=70

Guiinterface.js Nuke calls to cmpBarter.GetPrices() in other parts and use the new function. L2033 trailing comma, maybe meh.

A hotkey for triggering this panel would indeed be nice. This then trigger the menu when closed and closes when opened.

---

Side notes: We have a set trade amount and barter multiplier, maybe this should become an option just as the regular batch number is. When we have idle traders we get a orange message in the gui, but the period after the sentence isn't coloured which looks a bit strange imo.

[s0600204]

Updated patch

[bb]

It seems you have missed a couple of remarks, so I simply list them again :P

• Use g_Foo for globals, so we get g_BarterActions etc.
• L637,645,653 inconsistent use of whitespace (yes they moved a line in new patch), I suggest adding a whitespace in L645 and L653: "color: 255 0 0 80:".
• A hotkey.

And probably add a captial letter to L639 while on it.

[elexis]

An issue fatherbushido mentioned, that also became my biggest concern with this patch is the removal of the selection panel barter UI.

We could keep the barter UI and show them if and only if we have 4 or less resource types available. Thus players won't miss the dialog and we have fallback support for mods with more than 4 resources.

[s0600204]

It seems you have missed a couple of remarks

No, I just ignored the ones that made no particular sense or were too vague, in the hope that you might elaborate further. And you have.

Use g_Foo for globals, so we get g_BarterActions etc.

I refer you to the ​code style guidelines as written in the wiki, specifically the part about matching the style of new/modified code to the rest of the file so that files remain stylistically self-consistent as far as that is possible, and how that takes precedence over matching to a global style.

L637,645,653 inconsistent use of whitespace (yes they moved a line in new patch), I suggest adding a whitespace in L645 and L653: "color: 255 0 0 80:".

So that's what you meant. Done. (And will be included in the next patch upload.)

A hotkey.

Do we have any free? Is there a list of unused keys somewhere?

---

An issue fatherbushido mentioned, that also became my biggest concern with this patch is the removal of the selection panel barter UI.

Would you like to link to where he expressed his concern so I may read it myself and so that we have all information pertinent to this ticket contained within or linked to this ticket?

We could keep the barter UI and show them if and only if we have 4 or less resource types available. Thus players won't miss the dialog and we have fallback support for mods with more than 4 resources.

How feasible from a maintainability perspective do you think that suggestion is?

[bb]

Replying to s0600204:

Use g_Foo for globals, so we get g_BarterActions etc.

I refer you to the ​code style guidelines as written in the wiki, specifically the part about matching the style of new/modified code to the rest of the file so that files remain stylistically self-consistent as far as that is possible, and how that takes precedence over matching to a global style.

Tbh I dont see any caps only globals in that file [menu.js], so given this statement we should change to g_Foo things. Also Coding Convention give us When unavoidable, global variables should be named with a g_ prefix so we should do g_Foo.

L637,645,653 inconsistent use of whitespace (yes they moved a line in new patch), I suggest adding a whitespace in L645 and L653: "color: 255 0 0 80:".

So that's what you meant. Done. (And will be included in the next patch upload.)

Don't mind asking stuff like this, I can be vague for sure.

A hotkey.

Do we have any free? Is there a list of unused keys somewhere?

Not a list that I know of, and if it exists it is most likely outdated. Tracking "Default.cfg" do give us a list of all used hotkeys.

[elexis]

Would you like to link to where he expressed his concern so I may read it myself and so that we have all information pertinent to this ticket contained within or linked to this ticket?

You might recall ​http://irclogs.wildfiregames.com/2016-09-05-QuakeNet-%230ad-dev.log

We could keep the barter UI and show them if and only if we have 4 or less resource types available. Thus players won't miss the dialog and we have fallback support for mods with more than 4 resources.

How feasible from a maintainability perspective do you think that suggestion is?

Seems maintainable since people care about the public mod and in case we have more than 4 resources, we don't show the panel, perhaps that one button. The trade dialog itself can stay as is, only talking about the removal of the selection panel UI.

[s0600204]

Tbh I dont see any caps only globals in that file [menu.js]

I count nine caps-only global consts and one g_FooBar style global const in menu.js; not counting the ones added by relocating the code from the panel to the dialog.

I can be vague for sure.

I had noticed :P

Is there a list of unused keys somewhere?

Not a list that I know of, and if it exists it is most likely outdated. Tracking "Default.cfg" do give us a list of all used hotkeys.

I was sure someone copy-pasted a list on IRC some months ago. After a bit looking: ​http://irclogs.wildfiregames.com/2016-08-27-QuakeNet-%230ad-dev.log

16:13 < Imarok> elexis: wrt back-to-work: You have the choice between Y, O and L 16:34 < Imarok> fatherbushido: wrt back-to-work: You have the choice between Y, O and L

So, as Y was chosen for back-to-work: O or L. What is most suitable?

You might recall ​​http://irclogs.wildfiregames.com/2016-09-05-QuakeNet-%230ad-dev.log

Thank you for the link to the log of the relevant discussion conducted almost four months ago.

[...] The trade dialog itself can stay as is, only talking about the removal of the selection panel UI.

Ah. Was under the impression that you meant <5: barter panel only; >4: trade dialog only.

Personally I would prefer moving the barter icons to the trade dialog for good. A lot simpler implementation.

[elexis]

Hotkeys -> Pick an arbitrary free key or UNUSED. Perhaps a key-kombination might also work.

[s0600204]

Restore the Barter Panel, but only display it when there are four resources or fewer. Also add a hotkey for the Barter/Trade Dialog (Ctrl+B)

[elexis]

Thanks for keeping the barter panel. I tested the patch and besides one or two things, it is ready to go.

• Since r19028, the barter hotkeys are updated when pressing / releasing the shiftkey. Guess your new dialog should be updated as well if it's currently only onSimulationUpdate.
• Indeed I changed a number of FOO constants to g_Foo in gui/. There is no reason to shout in the code and often constants become variables. Agree that this should be done in an orthogonal patch.
• Didn't see a space after color: before, but idc.
• The hotkey was added to the overlay hotkeys, but the trade dialog isn't an overlay. Also it's a hotkey that is only valid in the session, so hotkey.session.trade or hotkey.session.barter prefix would adequate
• Not introduced by your patch, but selec has the let, the function refers to it before the variable is defined, ​hoisting let variables isn't really supported and wouldn't be surprised if that bugs with some Spider Monkey upgrade. Can we make that a direct argument of the function or do we get ​Closure issues? (Shouldn't as the onpress calls are executed inside a function already for that reason). Also the variable name selec is bad, better selected, selectedRes or selectedResource.
• Some onpress, new onPress. The GUI is case insensitive. (In case someone wants to do some more cleanup and add more surface for merge conflicts)
• Making STEP a new numerical option seems more useful to me than the bartering constants, but not exactly the scope of this ticket either.

(Strategically avoiding the review keyword, so that I can commit the patch without breaking the guidelines, in case someone else supplements the keyword.)

[bb]

Replying to s0600204:

Tbh I dont see any caps only globals in that file [menu.js]

I count nine caps-only global consts and one g_FooBar style global const in menu.js; not counting the ones added by relocating the code from the panel to the dialog.

I have to admit, I didn't look at the file itself, but only at the patch. When digging up the file itself, I indeed see 9 Caps only and 6 g_Foo's. So we already are messing in that file, so I suggest doing it correctly (g_Foo) at once and so avoiding changing the line twice but indeed it can be changed in a cleanup patch too.

Aren't we duplicating a ton of code in the last patch?

[s0600204]

The hotkey was added to the overlay hotkeys, but the trade dialog isn't an overlay.

I placed it next to the hotkey for bringing down the in-game menu (which isn't an overlay either). I've relocated it - see if you like the new change better.

Some onpress, new onPress.

Running ack across the gui's js files reveals that onPress is used more commonly than onpress. Changed patch to reflect that.

Making STEP a new numerical option seems more useful to me than the bartering constants, but not exactly the scope of this ticket either.

Maybe, and agreed. I've started putting together a second patch (that relies on the one in this ticket) that contains the changes proposed in this ticket that are outside the scope of this ticket. Once we're done here, I'll create a new ticket for the patch.

Aren't we duplicating a ton of code in the last patch?

Duplicating logic, certainly. But it's code that effects specific gui elements so there are (perhaps) sufficient differences.

That said, it might be possible to combine the two, but that would present a few problems. Not least because the dialog code is split across two functions (one that only runs on dialog open, and a second one that runs everywhenever and keeps the dialog up-to-date), whilst the panel code is an all-in-one function that repeatedly redraws the entire panel (hardly efficient tbh, but that's the gui for you).

There's also the problem that combining the code would mean that the same code is trying to draw both the panel and the dialog at the same time, even when one is not actually visible. Unless you added copious checks for visibility. Anyhow, it'd most likely get messy, fast.

[s0600204]

Updated patch, please see comment #15 for details.

[s0600204]

Reduce repeated code.

[elexis]

Thanks for addressing the code duplication, that would have had to be removed afterwards otherwise.

Don't see the point in having GetBarterPrices exposed in three places in the GUIInterface. The new function GetBarterPrices is not needed since we can access that directly in session/ via GetSimulationState().barterPrices (and that is cached and updated once per turn regardless, so it's actually a performance improvement removing that function). The other dupe barterMarket in GetExtendedEntityState can be deleted for good since it's not used anymore, right? (And if we want something like a 'supermarket' with different prices, that should be in the patch implementing that)

PlayerCanBarter is also a property that should be moved GetSimulationState.

Notice we don't have any duplicate XML code since the dialog buttons are explicitly specified while the selection panel buttons are dynamically filled.

[s0600204]

Updated patch

[s0600204]

Patch ported to Phabricator: ​D88

[elexis]

In 19354:

Add barter buttons to the trade window for quicker access and to support mods with more than four resource types.
The dialog is resized automatically and can be opened with a new hotkey.

Patch By: s0600204
Differential Revision: ​https://code.wildfiregames.com/D88
Fixes #4366
Refs #3934

[elexis]

Thanks for the feature! That hotkey will make a lot of players happy and that GUI support will make some modders happy!

[elexis]

In 23072:

Refactor trade dialog and barter panel buttons to use object orientation, refs #23, #2311, #4366, #5387, rP10588, rP14417, rP17553, rP19354.

Differential Revision: ​https://code.wildfiregames.com/D2369