Opened 3 years ago

Closed 3 years ago

Last modified 5 days ago

#4366 closed enhancement (fixed)

[PATCH] Move barter UI to trade panel

Reported by: elexis Owned by: elexis
Priority: Nice to Have Milestone: Alpha 22
Component: UI & Simulation Keywords: patch
Cc: s0600204 Patch:

Description (last modified by s0600204)

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: http://trac.wildfiregames.com/raw-attachment/ticket/4366/new_barter.jpg 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.

Attachments (7)

new_barter.jpg (61.4 KB) - added by elexis 3 years ago.
4366_barterUI.patch (21.2 KB) - added by s0600204 3 years ago.
Changes in one single patch, recently rebased (r19015, 36247e)
4366_barterUI_v1-1.patch (22.1 KB) - added by s0600204 3 years ago.
Updated patch
4366_barterUI_v1-2.patch (20.6 KB) - added by s0600204 3 years ago.
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)
4366_barterUI_v1-3.patch (23.2 KB) - added by s0600204 3 years ago.
Updated patch, please see comment #15 for details.
4366_barterUI_v1-4.patch (26.0 KB) - added by s0600204 3 years ago.
Reduce repeated code.
4366_barterUI_v1-5.patch (28.7 KB) - added by s0600204 3 years ago.
Updated patch

Download all attachments as: .zip

Change History (28)

Changed 3 years ago by elexis

Attachment: new_barter.jpg added

comment:1 Changed 3 years ago by elexis

Description: modified (diff)

Changed 3 years ago by s0600204

Attachment: 4366_barterUI.patch added

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

comment:2 Changed 3 years ago by 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?

comment:3 Changed 3 years ago by stanislas69

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

comment:4 in reply to:  3 Changed 3 years ago by 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.

comment:5 Changed 3 years ago by 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.

Changed 3 years ago by s0600204

Attachment: 4366_barterUI_v1-1.patch added

Updated patch

comment:6 Changed 3 years ago by 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.

comment:7 Changed 3 years ago by 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.

comment:8 Changed 3 years ago by 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?

comment:9 in reply to:  8 Changed 3 years ago by 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.

Last edited 3 years ago by bb (previous) (diff)

comment:10 in reply to:  8 Changed 3 years ago by 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.

comment:11 Changed 3 years ago by 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.

comment:12 Changed 3 years ago by elexis

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

Changed 3 years ago by s0600204

Attachment: 4366_barterUI_v1-2.patch added

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)

comment:13 Changed 3 years ago by 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.)

comment:14 in reply to:  11 Changed 3 years ago by 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?

Last edited 3 years ago by bb (previous) (diff)

comment:15 Changed 3 years ago by 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.

Changed 3 years ago by s0600204

Attachment: 4366_barterUI_v1-3.patch added

Updated patch, please see comment #15 for details.

Changed 3 years ago by s0600204

Attachment: 4366_barterUI_v1-4.patch added

Reduce repeated code.

comment:16 Changed 3 years ago by 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.

Changed 3 years ago by s0600204

Attachment: 4366_barterUI_v1-5.patch added

Updated patch

comment:17 Changed 3 years ago by s0600204

Description: modified (diff)

Patch ported to Phabricator: D88

comment:18 Changed 3 years ago by elexis

Keywords: rfc removed

comment:19 Changed 3 years ago by elexis

Owner: set to elexis
Resolution: fixed
Status: newclosed

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

comment:20 Changed 3 years ago by elexis

Milestone: Work In ProgressAlpha 22

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

comment:21 Changed 5 days ago by 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

Note: See TracTickets for help on using tickets.