Page MenuHomeWildfire Games

Fix line misaligment in gamesetup settings.

Authored by nani on Jan 20 2019, 6:57 PM.


Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
rP22111: Align the settingspanel with its background, fixing te misalignment itroduced…

Chat and settings GUI are only updated when distributeSettings is called but distributeSettings only is called when the windows resizes. Call it once at init so resizes to the correct size at the beginning.

Test Plan

Chat and panels looking aligned.

Diff Detail

rP 0 A.D. Public Repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

nani created this revision.Jan 20 2019, 6:57 PM
Stan added reviewers: Restricted Owners Package, elexis, bb.Jan 20 2019, 6:58 PM

Was it really a wrong size and not just one of the panels being visible when it should be hidden? Which GUI object is that?

smiley requested changes to this revision.EditedJan 21 2019, 8:50 AM
smiley added a subscriber: smiley.

Why this does not led to the issue before the commit that has the concern raised?
Previously, the sizes of the two panel would be adjusted onTick in updateSettingsPanelPosition.

let settingsBackground = Engine.GetGUIObjectByName("settingsBackground");
let backgroundSize = settingsBackground.size;
backgroundSize.left = settingsPanelSize.left;
settingsBackground.size = backgroundSize;

But now, if the offset is 0, that code would not be reached.

Therefore, IMO, this is a workaround. But if someone thinks otherwise, feel free to remove the status.

This revision now requires changes to proceed.Jan 21 2019, 8:50 AM

Adding a single function call to update more than strictily necessary in the GUI is a common simplification to avoid complex logic checking which things have to be updated.
Should still check whether this is the best place to add the line and the smallest code tree to call necessary.
For example initGUIObjects would be less unspecific than init(. Many people wanted to and have added many things in int, perhaps we should add a comment that people should stop addng things there.)
initSettingsTabButtons() would even be more specific than initGUIObjects.
Perhaps instead of distributeSettings, one could call updateSettingsPanelPosition() (IIRC nani already asked about 0 vs. undefined, now I know why. The answer to that can be found in the JS documentation).

nani updated this revision to Diff 7399.Jan 26 2019, 11:17 PM

Seems the real culprit was in the xml file. Some value was greater than expected making that the initial condition always return 0.

smiley added inline comments.Jan 27 2019, 5:02 AM
146 ↗(On Diff #7399)

Offset by 5 like the others?

nani updated this revision to Diff 7401.Jan 27 2019, 1:02 PM

Add the offset of 5.

Does look correct now apart from this one thing.
(Would accept the revision but that would be pretty meaningless.)

146 ↗(On Diff #7401)

Actually, the above comment is wrong. I wrongly compared x1 with y2. As you can see, x2 does not have any offset either. And also from the looks of the thing it seems like it's only needed for y.

nani updated this revision to Diff 7405.Jan 27 2019, 7:16 PM

Back to how it was.

From the original diff I gather that the size of this object is computed by JS? (Then it could be cleaner to not duplicate that information and remove the size from XML altogether and compute it once upon start. Then there aren't two places in the code that have to be kep in sync with people having to do math and comparisons.)

nani added a comment.Feb 23 2019, 10:05 PM

Will it get commited or do I need to change/do something? I made the change that broke it so I feel responsible :S

bb accepted this revision.Mar 16 2019, 2:32 PM

The horizontal size indeed is computed in js, since that size can change when you click on another tab, however the vertical size is the same always so coded in the xml. What went wrong in the initial slide panel commit (so it's not you who broke things nani, I did) is that the xml value or the background and front panel got different value. This was then masked by the ontick, but when that was removed, the real bug came out. However notice that the bug only happens when the panel width is maximal: in a small window everything is just aligned (Probably the reason I didn't find it....).

To the patch: the idea is good, however this patch results in an initial slide for all screensizes, instead the background's left size should be reduced to 100%-785. Change is trivial =>accept.

One could wonder why the background is even there (and I did while writing this up), that is since the settingspanel slides under the tab buttons and thus drags his borders with him, which looks ugly, the background masks this.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 16 2019, 3:15 PM
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Mar 16 2019, 3:15 PM