Validate all preferences when reading global_prefs and global_prefs_override

[Vulpine05]

Fixes #4916

Description of the Change Provides a separate function to check values passed from global_prefs and global_prefs_override. Some values were checked in prefs.cpp, but not all. This provides a space in the Client to check all values, and removes the checks in prefs.cpp.

Alternate Designs My original intent was to keep all checks in prefs.cpp, but as discussed here, it was requested to be in cs_prefs.cpp.

Release Notes [client] Added validation to all variables from global preferences.

[Vulpine05]

@AenBleidd, @davidpanderson, I need to do a more thorough review of my work, but the overall intent of what I would like to do is here. Could you take a quick look and let me know if you feel I'm on the right track?

Some questions and things I need to complete before I mark this as ready for a complete review:

• [x] There is one downside to this PR that I see. The global_prefs will be updated by parsing global_prefs.xml, and then it will be updated again by parsing global_prefs_override.xml. I am concerned that the client can read the GLOBAL_PREFS structure either between the two reads, or worse, before an invalid preference is adjusted (a negative percentage, for example). I don't know how likely that is, please let me know if that is a concern. I could always create a second Global_prefs structure, parse data to that structure, and then copy it to the global_prefs that the Client uses.
• [x] I need to review thoroughly parsing of any date/time fields.
• [x] I have not added any messages to print to the log/notices if a preference is invalid/adjusted.

[codecov[bot]]

Codecov Report

Merging #5050 (a0a6eb7) into master (71e5fd0) will decrease coverage by 0.01%. Report is 19 commits behind head on master. The diff coverage is 0.00%.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #5050      +/-   ##
============================================
- Coverage     10.86%   10.86%   -0.01%     
  Complexity     1068     1068              
============================================
  Files           279      279              
  Lines         36073    36094      +21     
  Branches       8339     8328      -11     
============================================
  Hits           3920     3920              
- Misses        31759    31780      +21     
  Partials        394      394              

Files	Coverage Δ
lib/prefs.cpp	0.00% <0.00%> (ø)

... and 3 files with indirect coverage changes

[AenBleidd]

Hey, @Vulpine05, any plans to continue working on this PR?

[Vulpine05]

@AenBleidd, maybe. This is related to my comment here.

[AenBleidd]

@Vulpine05, from my point of view the behavior should be next:

• function that reads the preferences from the file (and does actual raw data parsing) should validate that data is technically correct: double is double, integer is integer, etc
• function (class function to be more precise) that uses the data that was read before should do logical validation, print the error to the user and use the default value instead of logically incorrect one.

In this way we should eliminate the case when the same preferences are checked for the correct logic when read from the different sources. Also please keep in mind, that sometimes every single source of preferences might contain only the part of them, and thus before starting data validation you should gather them in one place (this is already done, just validation is missed, I suppose).

I hope I've answered your question. If no - don't hesitate to ping me again. Thank you in advance.

[Vulpine05]

@AenBleidd that answers my question, thank you. I think I can wrap this up as I have time. I do have one more noob question. I plan on having the alert message code look like this: msg_printf(0, MSG_USER_ALERT, "'suspend_cpu_usage' setting is <0. Setting changed to 0.");

Is it okay to hard code in the variable name in as text? The reason I ask is I don't know if that would accidentally be translated with Transifex. It seems easier write the alert message as above than taking the variable name, converting it to text, and then writing the alert message, such as: msg_printf(0, MSG_USER_ALERT, "'%s' setting is <0. Setting changed to 0.", PrefsVariable);

[AenBleidd]

@Vulpine05, in order to get the text translated, you need to use the special macro:

_("TEXT TO BE TRANSLATED")

In you particular case I suggest doing like that:

msg_printf(0, MSG_USER_ALERT, "'suspend_cpu_usage' %s <0. %s 0.", _("setting is"), _("Setting changed to"));

[Vulpine05]

Got it, thanks for the tip. I think "setting is" and "Setting changed to" are going to be used quite a bit, so I think I'll just create two strings at the beginning of the function and use those.

[Vulpine05]

@AenBleidd, this PR is ready for review.

[Vulpine05]

Some screen captures of the messages that could appear. Note that under the notices tab some are not appearing, but they do show up in the event log. When there are one or two validation alerts, this doesn't appear to be a problem. I suspect this may be an unrelated issue.

[davidpanderson]

Text shown to users shouldn't include variable names like 'suspend_cpu_usage'. It should use names from the prefs UI.

These names are pretty much consistent between the web prefs interface and the Manager's interface. However, many of them are different in Android. I suggest that we fix this in Android, and then return to this PR.

[CharlieFenton]

Are some of these checks not also made in the preferences dialog? If so, they should also be done there and the dialog should refuse to be saved until they are corrected.

It does make sense to also do the checks here when reading from the prefs files, since the user could manually edit them.

[Vulpine05]

Text shown to users shouldn't include variable names like 'suspend_cpu_usage'. It should use names from the prefs UI.

These names are pretty much consistent between the web prefs interface and the Manager's interface. However, many of them are different in Android. I suggest that we fix this in Android, and then return to this PR.

The intent is to alert the user that their is a mistake in the global_prefs(_override) file. Although the preference can be fixed via the Manager, I decided to list the preference name from the xml so it could be easily referenced there. I think this isn't too different when the Manager notifies the user of an error in a app_config file for a project.

Additionally, I am not sure how to easily describe when variable to fix via the Manager without being verbose.

[Vulpine05]

Are some of these checks not also made in the preferences dialog? This already happens in the Manager but that is independent of this PR. This PR validates variables in the Client.

For example, if you accidentally type -1 for suspend when computer is in use in global_prefs, then work will be suspended indefinitely.