Page MenuHomeWildfire Games

Report and delete wrong style settings
ClosedPublic

Authored by elexis on Aug 27 2019, 5:38 PM.

Details

Summary

The LoadStyle function silently ignores if a Style specifies a Setting that doesn't exist on the IGUIObject type it is assigned to.
By adding a harmless LOGWARNING, the machine can help authors to identify such entries.
There is a code comment in LoadStyle stating that it is a beauty to silently ignore, but it was not specified why this is a beauty.
If one applies the warning, we see a lot of warnings that arise from the "default" template, so it seems originally it was used for the default template.
If exempting the default style from the LOGWARNING, we dont get any false positives, indicating that there is no real use of this in other places.
So this warning identified 27 true positives and 0 false positives.
The next question is whether there could be any beauty (use case) in this at all.
If anything, it would be inheritance.
There is inheritance for IGUIObjects already, for example COList inherits CList inherits IGUIObject.

There are two ways for the inheritance:

  1. We observe defining a style that works for a base class will after this patch still apply without showing warnings. So "base styles" are a use case and still supported.
  2. We observe that a style that specializes for a child class will specify wrong settings for the base class.

The first effect of this is that authors are invited to specify settings (most importantly via copypasta) that don't ever have an effect without them realizing that.
The second observation for this case is that style authors will specify Settings that suit one child class (for example COList inheriting CList),
but this style will have Settings specified that
(a) don't exist in the other child class
(b) exist in the other child class but not this one
(c) specify a setting that should have a different value for both child classes, but the same Setting name!

The default style is most exposed to 2(c), because it applies to literaly every object, but specifying default values that apply to literally everything is not really possible.

So the conceivable use cases are better served by implementing style inheritance specifically if this is important.
Then one could specify a style for CList and then inherit this style in the style for COList and CDropdown. (That could be called beauty perhaps.)

The next question is whether virtual style inheritance is important enough to spend time on implementing.
If we look at the codebase, we see that other than the default style, nothing uses this as a feature.
For example Dropdowns and OLists (tables) specify their own style.
It's simple to perform.
The only disadvantage of not having style inheritance is more copypasta.

So for this patch and end of august 2019, I would suggest to add the warning and guide people to specify one style per IGUIObject rather than guiding them to specify settings that never apply.

This patch also unifies the two functions and removes one unneeded CGUI argument that is always same to the member.

The patch arose as a preparation of D2231.

Test Plan
  1. try { Follow the argument; } except (e) { http.post(e.toString()); }

Code verification:

  1. Apply only the C++ part of the diff, open every GUI page, notice that it bugs for only these XML files, or one did't really open every GUI page, or the styles are completely unused.
  2. Remove the "default" check to see what happens with that style.

Example messages of the LOGWARNING:

WARNING: GUI object has no setting "scrollbar", but the style "TreeDisplay" defines it
WARNING: GUI object has no setting "scrollbar_style", but the style "TreeDisplay" defines it

Diff Detail

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

Event Timeline

elexis created this revision.Aug 27 2019, 5:38 PM

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

Linter detected issues:
Executing section Source...

source/gui/IGUIObject.h
|  44| template·<typename·T>·class·GUI;
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'template<...' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/504/display/redirect

elexis edited the test plan for this revision. (Show Details)Aug 27 2019, 5:44 PM
elexis added inline comments.Aug 27 2019, 5:53 PM
source/gui/IGUIObject.cpp
275 ↗(On Diff #9525)

It would be good to remove it for default as well, but that file is already duplicated, probably needs to be moved too, so I suggest to investigate this in a separate diff, otherwide I'm in a dependency infinite loop, as this patch is also a preparation for other diffs (except for the LOGWARNING line)

This revision was not accepted when it landed; it landed in state Needs Review.Aug 27 2019, 6:03 PM
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Aug 27 2019, 6:03 PM
elexis edited the summary of this revision. (Show Details)Aug 27 2019, 6:05 PM