Page MenuHomeWildfire Games

Hide "Control all units" checkbox in observer mode
Needs RevisionPublic

Authored by wowka on May 19 2020, 2:58 PM.

Details

Reviewers
Silier
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Trac Tickets
#5670
Summary

Developer tools disabled checkboxes are now filtered out during instantiation of DeveloperOverlay

Test Plan

Manually compared developer tools checkboxes

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

wowka created this revision.May 19 2020, 2:58 PM
wowka added a reviewer: Restricted Owners Package.May 19 2020, 3:06 PM

First off, thank you for the patch! Don't hesitate to drop by on IRC if you have any questions.

As noted in the patch it would be good to only hide the checkbox, not the entire line.

binaries/data/mods/public/gui/credits/texts/programming.json
249

Please keep alphabetic order (nick if present) :)

binaries/data/mods/public/gui/session/developer_overlay/DeveloperOverlay.js
12

We prefer let over var whenever possible.

17–21

Can be combined.

binaries/data/mods/public/gui/session/developer_overlay/DeveloperOverlayCheckboxes.js
26

Perhaps this could be useful for other functions as well?

elexis added a subscriber: elexis.May 21 2020, 1:26 AM
  • Engine.GetPlayerID() can change during the course of the game using the developer overlay "change player" option.
  • Other hypothetical enabled properties could change arbitrarily.

Therefore all checkboxes must be initialized in the beginning and later when something changes, the checkbox should change accordingly.

So to be more precise:
DeveloperOverlayCheckboxes.prototype.ControlAll.enabled changes if and only if Engine.GetPlayerID() changes, so whenever Engine.GetPlayerID() changes, the enabled part should be refreshed ideally.
This could be done using something like this.registerViewedPlayerChangeHandler(this.rebuild.bind(this)); or by using the patch from #5670.

wowka updated this revision to Diff 11954.May 21 2020, 6:38 PM
wowka added inline comments.May 21 2020, 6:45 PM
binaries/data/mods/public/gui/session/developer_overlay/DeveloperOverlayCheckboxes.js
26

Do you mean adding some implementation of

enabled()

for every checkbox handler class?

Silier added a subscriber: Silier.May 21 2020, 6:46 PM
Silier added inline comments.
binaries/data/mods/public/gui/session/developer_overlay/DeveloperOverlayCheckbox.js
39 ↗(On Diff #11954)

could you please reuse variable above enabled to assign into hidden?
this.handler.enabled() is called twice and it is good for performance to not call things twice as long as not necessary

Thank you

wowka updated this revision to Diff 11956.May 21 2020, 6:56 PM
Silier requested changes to this revision.EditedMay 23 2020, 1:56 PM

With this change, Checkbox is missing but text label is still there.
So it should be or only grayed out so set to enabled = false, or if hidden, text label should be hidden as well and in that case, there should not be blank space visible.
Also Promote selected units should be disabled when player is observer in regards why Control all units is disabled in that case.

This revision now requires changes to proceed.May 23 2020, 1:56 PM
Silier resigned from this revision.Jul 31 2023, 5:26 PM