[PATCH] SplashScreenDisable option currently not used

[mimo]

The option window contains a checkbox to enable/disable the splashscreen, but its input is never used. The attached patch fixes that.

[leper]

Why is that code in the Press handler and not in the code for the Ok button? (Changing it and pressing cancel shouldn't result in the setting having been changed)

The text change needs some improvement and should also include that it will be shown once for each update/change. So actually the part about missing updates is wrong.

[mimo]

Thanks for the review

Replying to leper:

Why is that code in the Press handler and not in the code for the Ok button? (Changing it and pressing cancel shouldn't result in the setting having been changed)

You are right! I've moved it.

The text change needs some improvement and should also include that it will be shown once for each update/change. So actually the part about missing updates is wrong.

Done.

[leper]

Looks better, haven't tested it though.

[mimo]

In 19033:

fix splashscreen management, fixes #4391

[mimo]

Thanks for the review

[elexis]

• Grammar: will nevertheless be shown -> will be shown nevertheless
• Misleading description: the welcome screen will nevertheless be shown once when a new version is available
  • It has not been stated whether a new version of the splash screen or the game (release) is meant.
  • It has not been stated whether the splash screen is shown if a new version has been installed locally or has become available remotely (f.e. a new release on play0ad.com).
• (and you can always launch it from the main menu. That might be better as a single sentence.)
• JS code should be moved from XML to JS files where possible, especially when needing CDATA or using more than 3 lines of code.

• Independent from the commit, the filemod date is not a good identifier, because the date can change arbitrarily. Preferably the quick md5 hash of the splash screen file would be used (by adding GetSplashScreenVersion to ScriptFunctions.cpp, loading the file and doing what compare of test_MD5.h does).

[mimo]

Replying to elexis:

• Grammar: will nevertheless be shown -> will be shown nevertheless

I think what is written is correct and for me looks better. We would need an advice from a native english speaker. But as i don't mind, feel free to change it if you prefer.

• Misleading description: the welcome screen will nevertheless be shown once when a new version is available
  • It has not been stated whether a new version of the splash screen or the game (release) is meant.
  • It has not been stated whether the splash screen is shown if a new version has been installed locally or has become available remotely (f.e. a new release on play0ad.com).
• (and you can always launch it from the main menu. That might be better as a single sentence.)

Yes, the text can certainly be improved, and feel free to implement what you think would fit better. Maybe just replacing "once when a new version is available" by "each new version" would be enough.

• JS code should be moved from XML to JS files where possible, especially when needing CDATA or using more than 3 lines of code.

Why this "should". Is it your personnal taste or do you have any argument? While I agree for big pieces of codes, I think simple expressions like these ones make it clearer directly in their onPress part and I was tempted to move the only 2 lines from the js file in a onLoad action and remove completely this nearly-empty js file. But i refrain to do it just to avoid this kind of endless discussion.

• Independent from the commit, the filemod date is not a good identifier, because the date can change arbitrarily. Preferably the quick md5 hash of the splash screen file would be used (by adding GetSplashScreenVersion to ScriptFunctions.cpp, loading the file and doing what compare of test_MD5.h does).

As you said, that's independent of the commit and if you want your remark to have any effect, you'd better open a new ticket about it (or reopen this one) rather than commenting on a closed ticket.

[elexis]

Rephrasing, separation of JS and XML, md5sum hash, GUI element size fix, code duplication removal and saving the version when opening, not when closing offered in #4399.

XML is a markup language, as such not intended to describe code but the design of a document. Splitting the presentation layer (XML) from the controller (JS) matches the ​Model-View-Controller pattern and ​Information hiding pattern. Automated testing with jenkins, manual testing with jshint and syntax highlighting in the editor is unfeasible with JS inside XML. Even small files should (= ought to) start with a structure that will not accumulate disadvantages when increasing the codebase.