Refactor settings

[superenginegit]

This PR is for issue #5201.

What I have done so far:

• Added file SettingsDescription.h where the Settings are described in structs. It is possible to set the xmlName, default value, an optional comment to go into the settings file and three functions for import, export and validation (I expanded the approach from here)
• Adapted the current Settings to work with the new data structure

What I think could still be done:

• It would be possible to remove the setters and getters from the Settings.h and Settings.cpp files and replace their usage with set<setting-name>(), but this would probably be too much to add it to this PR as it is already a lot of changes

I'd appreciate any feedback on this

[bhennion]

Here are a couple of comments. Mainly use SFINAO to default most members of the Setting<> specializations.

Also, there are quite a bit of clang-format changes in parts that you did not edit. Could you please remove those changes? They clutter the diff making the reviewing process more complicated and increase the number of conflicts you'll get if you ever need to rebase on a more up to date master branch.
Running git clang-format (as opposed to asking your IDE to apply clang-format) does exactly that: only apply clang-format to the chunks you modified.

[superenginegit]

Thanks for your comments, I'll incorporate them.

[bhennion]

That's a lot of work! Thanks!!
Here is a partial review. I'll come back for more (but that's a lot to review).
One question: shouldn't SElement be modified to have the same structure as the main Settings class (i.e. not being a std::map but rathen a template)?

[superenginegit]

One question: shouldn't SElement be modified to have the same structure as the main Settings class (i.e. not being a std::map but rathen a template)?

I considered doing this, but I ultimately decided not to, as SElement is currently a very customizable container. Recreating it as a template would be very complicated, and I don't think it would be the best option for most of the settings currently using it.

I think it would be better to rework those settings and give them their own, unique type. For example lastUsedPageBgColor would work well as a vector I think, The touch nested setting should probably be just a struct (or maybe a class if necessary). Just for tools a nested structure would make sense, but that could be done similarly to how the buttonconfig or deviceclasses are currently implemented.

But I would rather do this in another PR as this one is already really big, and reworking those settings would create a lot of changes in additional files.

For this PR I would keep the SElement, and fix the issue with the const& you mentioned in Settings.h.

[bhennion]

Here are some more comments.

Overall, I'm a bit confused about various declarations/definitions. For instance, the importProperty/exportProperty specializations feels misplaced. There are two kinds of such: "generic" specializations, e.g. importProperty, that are used by numerous settings and whose location is alright, and very specific specializations (e.g. importProperty(xmlNodePtr node, std::map<std::string, std::pair<InputDeviceTypeOption, GdkInputSource>>& var)) which should go directly as IMPORT_FN of the corresponding setting.

TBH, I'm a bit confused at the moment as to why it compiles and links properly while those importProperty<> specializations are only defined in the .cpp file but used in other units.

In addition:

1. There are a couple of

   if (foo)
       bar()
   

   where braces should be added, to match our coding conventions.
2. There are some C-style casts which should be replaced with static_cast.

[superenginegit]

Thanks for the review, I have now addressed all of the points you mentioned.

• Mainly I moved the import and export functions to a file of their own
• Same for the getter_return, comment, etc. structs
• I improved the comment, making it hopefully easier to understand
• I removed the function array and map from the SettingsContainer class as members, the map is still generated in a member function, but is only allocated for the load function, the export functions are in a static loop
• I also replaced c-style casts with static_cast, and I added braces to if statements (hope I didn't miss anything here)

[rolandlo]

@superenginegit Can you please rebase on current master and solve the merge conflicts, so we can see if there are any (new) warnings on different platforms?

[rolandlo]

The clang format should be fixed. I have found the following warning (just one), that should be fixed, too:

2023-11-29T18:31:02.1823240Z /Users/runner/work/1/s/src/core/control/settings/SettingsImExporters.cpp:706:33: warning: implicit conversion changes signedness: 'int' to 'std::array::size_type' (aka 'unsigned long') [-Wsign-conversion]
2023-11-29T18:31:02.1923240Z const auto& cfg = value[i];
2023-11-29T18:31:02.2001240Z ~~~~~ ^
2023-11-29T18:31:02.2101880Z 1 warning generated.

[superenginegit]

@bhennion What do you think about the current status of this? Would you like to see any more changes?

[bhennion]

Sorry for the delay. To be perfectly candid, I've been wondering whether this simplifies maintainability or not. It seems to me that we still need to edit in a lot of places to e.g. add a new setting.
Let me play around with it and I'll get back to you.