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.
Details
- Reviewers
elexis - Group Reviewers
Restricted Owners Package (Owns No Changed Paths)
Test or improve fix.
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Event Timeline
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 | ||
---|---|---|
498 | 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. |
Rebased.
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 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.
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).
(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.)
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.