Page MenuHomeWildfire Games

Move GUIUtil SetSetting to IGUIObject
ClosedPublic

Authored by elexis on Aug 27 2019, 2:45 AM.

Details

Summary

See documentation in D2230.

See Philip comments about GUIutil.h being ugly and should be rewritten in rP22789#36910.
See rP22790 noticed when writing this commit.

  • Move static GUI<>::SetSetting operating on IGUIObject to IGUIObject::SetSetting member function, near SettingExists, GetSetting, AddSetting.
  • Stop using PSRETURN codes for these functions, since there is no benefit over if-SettingExists checks or std::map exceptions, while they made the code uglier.
  • Remove SetSettingWrap std::function argument by removing the need for it (removing the exception code inside it). Rename to SetSettingChanged.
  • Rename existing SetSetting to SetSettingFromString. That there are two functions called SetSetting shows yet again that the previous split of setting functions was wrong.
  • Change SkipMessage to SendMessage, so that a positive value relates to a positive action.
  • Remove default values, see D2211/rP22785 and other historic commits.
  • Add SendMessage parameter to CGUISetting functions.

14:20 <@Philip-> which is why it's in GUIutil.h
14:21 <@Philip-> (I think it's fairly ugly)
14:22 <@Philip-> (but not enough to motivate me to rewrite it)

Test Plan

Read the patch after having slept, not before.
Make sure that those bool arguments are correct.
Let further refactoring be done independently.
Find more related revision IDs to point out.

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, 2:45 AM

Build failure - The Moirai have given mortals hearts that can endure.

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

elexis updated this revision to Diff 9524.Aug 27 2019, 12:25 PM

Missing latenight semicolon

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

Linter detected issues:
Executing section Source...

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

source/gui/GUIutil.h
|  24| 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/503/display/redirect

elexis added inline comments.Aug 27 2019, 5:01 PM
source/gui/CInput.cpp
80 ↗(On Diff #9524)

For example if someone (absolutely not me) was to use a wrong boolean here, you'd get an infinite loop segfault.

source/gui/GUITooltip.cpp
149 ↗(On Diff #9523)

the debug_warn come from rP1540, there were many more of them back then, apparently got removed elsewhere too.

I suppose a LOGERROR is sufficient, dont have to crash the entire UI if someone set a wrong XML object, one that doesn't have _mousepos, i.e. CTooltip.

It's not like the tooltip code isn't weird (perhaps virtual inheritance would be more consistent with the rest, dunno).
Nor is it like _mousepos isn't actually a setting, but should be a member.

source/gui/IGUIObject.cpp
312 ↗(On Diff #9524)

if the SetSetting fails, don't care.

Comment from rP1234.

I suppose I'll add a safeguard then instead of a segfault as in this version of the patch.

There are two kinds of SetSetting calls.
One of them is when reading the XML files, which means XML editors are in charge of what values SetSetting receives. Thus those need safeguards.

The other kind of SetSetting calls are the ones that happen in the same GUIObject file after the AddSetting calls and thus are the responsibility of the C++ author and there we should definitely not silently ignore errors but explicitly add safeguards if the Setting is optional.

elexis updated this revision to Diff 9527.Aug 27 2019, 6:15 PM

Rebase following rP22792 and some SetSetting fixes.

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

Linter detected issues:
Executing section Source...

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

source/gui/GUIutil.h
|  24| 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/505/display/redirect

elexis added a comment.EditedAug 27 2019, 7:38 PM

One of the reasons why I decided to add the default argument in this patch is because this patch already touches all the 133 SetSetting calls. So I rather have one patch with 133 lines that changes it to the better form than two patches with 133 lines, or one patch not using the potential of doing the better,
even if it adds the danger of passing a wrong bool somewhere and crashing the thing.
It however means that one has to review every line. Not even once or twice. I did that. Not even three or four times. :-/

The other thing that has to be checked for every line is that every SetSetting call is either safeguarded or operates on something it can legitimately trigger an uncaught exception if it the setting would not exist. That should only be the case if AddSetting is in the same class or inherited class, but not for example for GUI tooltips where the XML author specifies the input and can pass arbitrary objects.

Yet I still wonder if eventually the Setting Management (TM) logic shouldn't be moved to a separate file (rather than in GUIUtil nor IGUIObject).
IGUIObject could inherit yet another class IGUISettingsManagement for instance.
If Im not mistaken, such a similar construct had existed long time ago even.

However after this patch, GUIUtil.cpp will only contain CGUISetting.
Since that is only 80 lines, it's mostly a matter of what C++ template specialization features will allow for to move that also to IGUIObject and nuke the file (from orbit).
I don't see the benefit of having a Settings class when the code is only 80 lines and still operates on the IGUIObject; unless C++ wants that so.

So in case new evidence shows that AddSetting, SettingExists, SetSetting(FromString), GetSetting, SetSetting should not be in IGUIObject but a separate file, then the move in this commit wouldn't be the shortest path until there.
However the GUI<> class (that probably should have been a namespace) with static members is definitely wrong as is, and the patch removes legacy and ugly code and replaces it with better code, and the evidence for the own file is not there, and also it might be easier to refactor this into a separate class after this patch, (because changing the classname actually isn't the hard part of the diff, the default argument and the exception handling now is, from the audit pov).

source/gui/CCheckBox.cpp
91 ↗(On Diff #9527)

This setting is defined in this file, so this call may rightfully assume that the setting exists and even segfault if someone removes the "checked" property from the checkbox and presses it.

source/gui/CDropDown.cpp
58 ↗(On Diff #9527)

"scrollbar" is defined in IGUIScrollBarOwner, so ok to assume its defined IMO

elexis added inline comments.Aug 28 2019, 1:14 PM
source/gui/GUITooltip.cpp
154 ↗(On Diff #9527)

It's also in the XML authors control to set an object that doesn't have a "caption", so there must be a safeguard here too.
From recollection of the 133 SetSetting calls I think most if not all the other calls, at least in the other files, are ensured by the class or inherited class that the setting exists.

"hidden" exists for every IGUIObject.

It seems it would be more solid to static_cast<CTooltip> instead of testing if a setting exists.

One can reproduce this XML induced segfault by setting use_object="playerAssignmentsPanel" in gui/gamesetup/setup.xml.

Aside from that, the entire file looks like a bit weird. There shouldnt be a thing such as private settings shared across files, since they are still exposed, both to JS and C++. Should rather use member variables or pass as argument in C++.

....

Can't use static_cast because of virtual inheritance, needs dynamic_cast, but dynamic_cast is wicked.

So I can only add a bunch of SettingExists and LOGERROR, which is better than the previous debug_warn if an XML author set some wrong gui object tooltip setting.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 28 2019, 1:21 PM
This revision was automatically updated to reflect the committed changes.

However after this patch, GUIUtil.cpp will only contain CGUISetting.
Since that is only 80 lines, it's mostly a matter of what C++ template specialization features will allow for to move that also to IGUIObject and nuke the file (from orbit).
I don't see the benefit of having a Settings class when the code is only 80 lines and still operates on the IGUIObject; unless C++ wants that so.

I tried moving it following the commit, but the template deducation doesn't allow us to, for seveal reasons:

  • When loading XML settings, the type T is not known. The trick from the commit introducing CGUISetting was to transfer the T information add AddSetting time. If we remove the class, we lose this information and would have to know the information of the type from the XML file. This would be the previous approach where every type is linked to the enum, but that's worse than having the CGUISetting class with 80 lines.
  • When deleting the settings, the type needs to be known as well, and can't be inferred from anything. But the type is known for the CGUISetting class.
  • When setting a setting from JS, via JSInterface_IGUIObject, CGUISetting::FromJSVal, then the type is also not knowable from JS, so it cannot be inferred. But it works well with CGUISetting approach.

Take a look at the impossible diff yourself here:

So CGUISetting will have to remain, and it means GUIUtil.h can become CGUISetting.h, only remaining weird function is ParseString, and that seems to be a CGUI one, which only needs something for tests to be moved over, or remain static and moved over.

elexis updated the Trac tickets for this revision.Aug 29 2019, 11:24 AM