Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't utf8-check New_Game #470

Merged
merged 2 commits into from
Jun 8, 2021
Merged

Conversation

P-D-E
Copy link
Contributor

@P-D-E P-D-E commented Jun 6, 2021

Thank you for submitting a pull request and becoming a contributor to the Vega Strike Core Engine.

Please answer the following:

Code Changes:

Issues:

Purpose:

  • What is this pull request trying to do? Skipping the utf-check for the New_Game special case as it's not a real file; it makes the Campaign (main menu) and New (while docked in game) buttons work properly
  • What release is this for? 0.8.x
  • Is there a project or milestone we should apply this to? 0.8.x

Don't utf8-check New_Game
@P-D-E P-D-E added this to the 0.8.x milestone Jun 6, 2021
Copy link
Contributor

@stephengtuggy stephengtuggy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good. Just have one question.

@@ -5979,7 +5979,7 @@ bool BaseComputer::actionConfirmedLoadGame()
if (desc) {
std::string tmp = desc->text();
if (tmp.length() > 0) {
if (!isUtf8SaveGame(tmp)) {
if ((tmp != "New_Game") && (!isUtf8SaveGame(tmp))) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this check should be made case insensitive? Using, for example, boost::iequals.

Other than that, looks good.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New_Game is hard-coded, all other case variants are intentionally allowed as legit save names, and only this exact one is barred from manual save, so that the engine knows it's dealing with a new game.
You can see it for instance in the method above this one, actionSaveGame, which I just copied to actionConfirmedLoadGame for coherence: (string( "New_Game" ) != tmp)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@P-D-E is there a string constant that can be used for that? If not, we should add an issue to introduce one.

I'd just hate to see that string modified slightly and have things break.
Also, we probably should prohibit all case variants of that string (if we don't right now, that should be another issue).

Copy link
Contributor Author

@P-D-E P-D-E Jun 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there are quite a few independent instances of the string "New_Game":

engine/datascripts/setup.base.xml
engine/datascripts/021.nsi
engine/src/cmd/basecomputer.cpp
engine/src/options.cpp
engine/launcher/saveinterface.cpp
data/cgi-accountserver/db.py
data/bases/computer_lib.py
data/bases/main_menu.py
data/modules/GUI.py

and they are more than the usages of a convenient UniverseUtil::getNewGameSaveName function in engine/src/universe_util_generic.cpp also made available to Python via the VS module, e.g.

engine/src/cmd/basecomputer.cpp
6010:    desc->setText( UniverseUtil::getNewGameSaveName() );

data/bases/main_menu.py
16:    VS.loadGame(VS.getNewGameSaveName())

We might try replacing all the explicit New_Game mentions with calls to the function, especially where the needed includes are already there. I'd leave that to 0.9.x, though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@P-D-E I'm good with a separate PR for that too; can you please open an issue with your findings? Thanks!

Copy link
Contributor

@ermo ermo Jun 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might try replacing all the explicit New_Game mentions with calls to the function, especially where the needed includes are already there.

That sounds like a good approach to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BenjamenMeyer @ermo Thank you both, here's the issue.

Copy link
Contributor

@stephengtuggy stephengtuggy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. Looks good to me then.

@BenjamenMeyer BenjamenMeyer added this to In progress in 0.9.x Release Jun 7, 2021
Copy link
Member

@BenjamenMeyer BenjamenMeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from my comments related to @stephengtuggy's review, I'm good for this too.

@ermo ermo mentioned this pull request Jun 7, 2021
2 tasks
@BenjamenMeyer BenjamenMeyer merged commit b029cc4 into vegastrike:master Jun 8, 2021
@BenjamenMeyer BenjamenMeyer mentioned this pull request Jun 8, 2021
2 tasks
@stephengtuggy stephengtuggy added this to Done in 0.8.x Release Jul 10, 2021
@stephengtuggy stephengtuggy moved this from In progress to Done in 0.9.x Release Aug 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants