Page MenuHomeWildfire Games

Tributes break in diplomacy dialog if more than one resource is clicked while holding shift.
AbandonedPublic

Authored by mapkoc on Jan 2 2018, 11:30 PM.

Details

Reviewers
elexis
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Summary

The actions changed while holding shift remain uncleaned for all resources except the last when you release. (multiplier, tooltip and amounts get dirty)
Scope for those variables was upgraded so each can see the rest and the one that applies resets values.

Test Plan

Test or improve fix.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 9658
Build 16205: arc lint + arc unit

Event Timeline

mapkoc created this revision.Jan 2 2018, 11:30 PM
elexis awarded a token.Jan 3 2018, 1:52 AM

binaries/data/mods/public/gui/credits/texts/programming.json entry missing (you get that if you want or not :P)

The proposed patch fixes the bug that if one holds shift, clicks on one resource in the tribute dialog, then on another resource and leaves shift, that the first resource isn't reset to 100.

However I noticed a similar bug, it should already be reset when the mouse button is unpressed. With the patch applied, it still shows 500 for the original resource while holding shift.

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

Oh I get it, this is a trick to store a variable without using a global and without saving it directly in a button property.
This is what you must have meant with "static".

elexis added a reviewer: Restricted Owners Package.Jan 4 2019, 2:31 PM
Freagarach updated this revision to Diff 10013.Sep 29 2019, 4:28 PM
Freagarach retitled this revision from Tributes break in diplomacy dialog if more than one resource is clicked while holding shift to Tributes break in diplomacy dialog if more than one resource is clicked while holding shift..
Freagarach edited the test plan for this revision. (Show Details)
Freagarach added a subscriber: Freagarach.

Rebased.

In D1191#50897, @elexis wrote:

However I noticed a similar bug, it should already be reset when the mouse button is unpressed. With the patch applied, it still shows 500 for the original resource while holding shift.

I think this is the intended behaviour of the author. It lets one tribute more than one resource at a time (what the function already supports).

Owners added a subscriber: Restricted Owners Package.Sep 29 2019, 4:28 PM
Stan added a subscriber: Stan.Sep 29 2019, 4:32 PM
Stan added inline comments.
binaries/data/mods/public/gui/session/menu.js
492

brace on newline.

495

Not sure initialization is needed.

517

Comment is a bit weird, maybe it could be improved.

535

brace on new line.

I think this is the intended behaviour of the author. It lets one tribute more than one resource at a time (what the function already supports).

I cant fully reproduce the premise (code supports it) nor the conclusion (hence its the intention of the author).
If it was the intention of the author, then he would have checked that this feature was actually supported, but this feature is not supported correctly unless its patched this way.
To me it seems more like not having considered or tested for the possibility of pressing on other resource types without having released the tribute previously.

However I do agree that it is not wrong to support tributing resources of different types this way.

However 2, I do not agree with the way how this patch implements it, since it does so by adding a local function variable that is hoisted into a the local function that is stored globally.
So this state is defined in a function that doesnt use it, while the state is used in a local function of the function and globally.

So to me, this patch implementation makes the code transparency worse, but only consequentially so due to the existing code being this way already:

				// This is in a closure so that we have access to `player`, `amounts`, and `multiplier` without some
				// evil global variable hackery.
				g_FlushTributing = function() {

I don't really understand the comment, because the function is stored in a global variable that I can only call evil hackery as well, even worse than usual global variables.
See also http://trac.wildfiregames.com/wiki/Coding_Conventions

While I did this observation everytime I looked at this according tribute button code, no matter whether it was for this patch or the the first looked and refactored it in 2015,
I did not know how to improve it systematically until I rewrote it in the course of #5387 at D2365.
The INPUT_MASSTRIBUTING global is also replaceable by handling a hotkey-release event.
So this feature will be implemented if D2365 is implemented.

The code supports it.

The PostNetworkCommand sends amounts not amount, which would, for me, point strongly towards: "Hey, lets make sure we can tribute more than one resource in one go!" (Okay, that was probably because GetNeededResources needs an object.)
(If I see it correcty, rP17820 changed it so that only one resource was sent at a time?) While I agree it remains elusive whether or not it was ever intentional.

Either way, I think it would be nice to allow multiple types of resources to be tributed in one go.

If I see it correcty, rP17820 changed it so that only one resource was sent at a time?)

I don't see a behavior change in that commit, there are all resource types sent prior and afterwards, and prior and afterwards all of them are 0 or multiplier * 100.
Notice that sending resources this way still isnt ideal in D2365, as it sends one command per resource type in that case (which could be changed by storing the amounts in the manager instead of the button if one wanted to).

Ah, I see indeed. Never mind then :)

(And when speaking in general about this tribute button, there should be a way to abort the tribute. I remember a suggestion on irc at the time of that commit about making that a confirmation box, which has advantages (cancelability) and disadvantages (more user interaction if only wanting to send few amounts).
Also the amounts to be sent should be displayed on the buttons, right now theres only the tooltip which is shown for one of the resources to be tributed.)

elexis abandoned this revision.EditedOct 11 2019, 2:30 PM

Thanks for bringing the issue to my attention and going the extra mile to writing a proposal too.
Issue fixed in rP23065.
If you want to improve the presentation of the button feel free to create an independent proposal or pick this up.