Opened 4 years ago

Closed 4 years ago

#3745 closed defect (wontfix)

[PATCH] Back-to-work icon is not shown sometimes

Reported by: svott Owned by: svott
Priority: Must Have Milestone:
Component: UI & Simulation Keywords: patch
Cc: Patch:

Description

The defect exists for a group of selected units. Figuring out the broken case is not that easy

Good case:

  • always when a worked-female worker is in the group
  • always when all selected units are worked-units

defect case:

  • when only archer units are worked-units and all other selected units are not worked-units

-- defect: select a spearman and then add with shift a worked archer -- NO defect: select a cavalry ranged unit and then an archer -> wired

suspicion: defect depends on the selection order and unit type (ranged, ..)

"worked-unit" = has as a back-to-work-attribute in single selection

Attachments (2)

backtowork.patch (3.4 KB) - added by svott 4 years ago.
backtowork.2.patch (3.4 KB) - added by svott 4 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 4 years ago by svott

.. there are also defect cases with worked-female

comment:2 Changed 4 years ago by svott

I fixed it.. quite tricky and I think you will complain about my extra loop. The loop could also be one level above..

What was the problem?

selection_panels.js -> g_SelectionPanels.CommandgetItems? was only called for the first unit of the selected group.

If this unit did not have a "back-to-work" entry, the command was not added to the command list Otherwise, if the first unit had this entry (but maybe other units of the selection not), the command was added to the command list

Workaround: if the given unit has no back-to-work entry, we must also check the other units

Problem: This could also occur for other commands. so my solution is not the best, right? There should be a loop over all units one or two levels earlier.

AND:

The function comments in unit_command.js talk about the first unit which was selected. This is wrong, it is the unit with the lowest id!

Changed 4 years ago by svott

Attachment: backtowork.patch added

Changed 4 years ago by svott

Attachment: backtowork.2.patch added

comment:3 Changed 4 years ago by svott

Keywords: patch review added
Summary: Back-to-work icon is not shown sometimes[PATCH] Back-to-work icon is not shown sometimes

sorry for double upload

comment:4 Changed 4 years ago by Itms

Keywords: review removed

This bug is part of a bigger bug which is that the UI is a bit broken when it comes to groups of units.

Your solution looks nice!

With respect to the style, you should fix your curly braces (we use a new line for the opening one) and remove some tabs in empty lines. Also we have a new way to track contributions for the game credits, you should take a look at ps/trunk/binaries/data/mods/public/gui/credits/texts/programming.json and add yourself if you want to (no obligation of course!)

Thanks for your work!

comment:5 Changed 4 years ago by svott

Thanks for reviewing it. The small style fixes should be a problem. Will fix it later.

Yes, perhaps a bigger workaround is needed to fix the original problem. But, I think this patch a suitable quickfix for the beginning. I played A19 (not SVN) again at the weekend after a couple if weeks and this bug was/is so annoying.

comment:6 Changed 4 years ago by svott

(Where are unnecessary tabs in empty lines? I will regard the curly bracket style next time, but I won't post a patch where just changed that in line 211 and 212)

comment:7 Changed 4 years ago by Itms

Lines 214 and 218.

comment:8 Changed 4 years ago by Itms

Resolution: fixed
Status: newclosed

In 17880:

Workaround for the multiple-selection commands problem in the case of the back-to-work button. In this case a fix is really needed, as long as the system for handling the selection is not improved.
Also fix the wrong documentation for some functions.

Patch by svott, fixes #3745.

comment:9 Changed 4 years ago by Itms

In 17883:

Fix oversight in r17880: GetExtendedEntityState? is far too demanding for performance, and it is not needed.
Refs #3745

comment:10 Changed 4 years ago by Itms

In 17885:

Revert r17880, refs #3745, refs #3823

comment:11 Changed 4 years ago by Itms

Resolution: fixed
Status: closedreopened

After a discussion and on second thought, we shouldn't include this kind of workarounds but just try and fix the big issue (especially here where the issue impacts a lot of different things in the user feeling). I opened #3823 for this issue.

Sorry for the wandering.

comment:12 Changed 4 years ago by Itms

Milestone: Alpha 20
Resolution: wontfix
Status: reopenedclosed
Note: See TracTickets for help on using tickets.