Page MenuHomeWildfire Games

remove unnecessary selection panels object
Needs ReviewPublic

Authored by Nescio on Sat, Jul 6, 5:26 PM.

Details

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

Currently the mimimap and selection panels are wrapped inside a “bottom panel” object. This is unnecessary, because it's centred on the centre of the bottom of the screen, and the selection panels' positions are already relative to that.
More importantly, removing it makes positioning the minimap more intuitive, because it's no longer relative to the bottom panels, making it easier for anyone to reposition the minimap in a mod (e.g. to the bottom left corner of the screen rather than attached to the left of the selection panels): without this patch they need to change both session.xml and minimap_panel.xml, with this patch they can ignore the former and only have to look at the latter.

Test Plan

Launch a game without this patch, then launch one with this, and observe effectively nothing is changed.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8224
Build 13407: Vulcan BuildJenkins
Build 13406: arc lint + arc unit

Event Timeline

Nescio created this revision.Sat, Jul 6, 5:26 PM
Owners added a subscriber: Restricted Owners Package.Sat, Jul 6, 5:26 PM
Vulcan added a comment.Sat, Jul 6, 5:28 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/1929/display/redirect

Nescio edited the summary of this revision. (Show Details)Sat, Jul 6, 5:29 PM
Nescio edited the summary of this revision. (Show Details)
bb added a subscriber: bb.Thu, Jul 11, 3:09 PM

I absolutely like to see that you want to expand your knowledge from just templates to more areas of the code such as the gui, however it seems like you are fixing a non-existing problem here: The GUI objects incorporate their size from their parent (just like templates incorporate values from their parent). So if you have something like:

<object size="30, 30, 100, 100">
  <object size "0 0 100% 100% "/>
</object>

The inner object will be a square with sides of 70 pixels.

In the code of this patch it is true that everything is given from the center of the screen, but actually the 50%'s in the inner objects refer to the 50% of the object starting in L117 (<object size="50%-512 0 50%+512 100%">). Since this extends equally far to both sides, the 50% in the inner object indeed correspond to the center of the screen. But one could change the size of the parent a bit and see that the inner objects will move with it.

More importantly, removing it makes positioning the minimap more intuitive, because it's no longer relative to the bottom panels, making it easier for anyone to reposition the minimap in a mod (e.g. to the bottom left corner of the screen rather than attached to the left of the selection panels): without this patch they need to change both session.xml and minimap_panel.xml, with this patch they can ignore the former and only have to look at the latter.

I don't think patch makes life easier here, actually I think it makes it harder. In current svn one could just move the <include file="gui/session/minimap_panel.xml"/> a few lines up (such that it is out of his direct parent) and then it would be located at the bottom left corner, so that part isn't improved in the patch. However with this patch if someone wants to add the minimap in two places, one would need to change the session.xml while that could be totally unrelated. In the current svn one just has to add the minimap in a appropriately sized object.

binaries/data/mods/public/gui/session/session.xml
116–117

There is a reason for this object, namely the comment

however it seems like you are fixing a non-existing problem here

Possibly. I fully admit this patch isn't of vital importance, but I do believe it's a minor improvement; otherwise I wouldn't have bothered proposing it.

The GUI objects incorporate their size from their parent [...]
actually the 50%'s in the inner objects refer to the 50% of the object starting in L117

Yes, I fully understand that. However, *effectively*

<object size="50%-512 0 50%+512 100%">
  <object size="50%-304 100%-170 50%-110 100%"/>
  <object size="50%-114 100%-205 50%+114 100%"/>
  <object size="50%+110 100%-170 50%+512 100%"/>
</object>

is equivalent to

<object size="50%-304 100%-170 50%-110 100%"/>
<object size="50%-114 100%-205 50%+114 100%"/>
<object size="50%+110 100%-170 50%+512 100%"/>

One could wrap it into as many objects one likes, e.g.

<object size="50%-128 0 50%+128 100%">
  <object size="50%-256 0 50%+256 100%">
    <object size="50%-512 0 50%+512 100%">
      <object size="50%-304 100%-170 50%-110 100%"/>
      <object size="50%-114 100%-205 50%+114 100%"/>
      <object size="50%+110 100%-170 50%+512 100%"/>
    </object>
  </object>
</object>

Yet all that does is adding unnecessary layers of complexity. In my opinion it's best to keep things as simple as possible.

However with this patch if someone wants to add the minimap in two places, one would need to change the session.xml while that could be totally unrelated. In the current svn one just has to add the minimap in a appropriately sized object.

Not sure why anyone would ever want to display the minimap twice, but if so, the way to do is the same with and without this patch.

Moreover, defining the minimap's position only in the minimap_panel.xml file, and not in session.xml, would also improve consistency; see e.g. diplomacy_window.xml or trade_window.xml.