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

BASE: GUI: Add option to always return to launcher instead of closing ScummVM #2621

Open
wants to merge 6 commits into
base: master
from

Conversation

@lotharsm
Copy link
Member

@lotharsm lotharsm commented Nov 14, 2020

As proposed in https://bugs.scummvm.org/ticket/11951#comment:2 and https://forums.scummvm.org/viewtopic.php?p=94390#p94390, this patch adds a GUI option to always return to the ScummVM launcher when leaving a game or even closing the application window instead of closing ScummVM.

This is done by intercepting the EVENT_QUIT event and overriding it with EVENT_RETURN_TO_LAUNCHER when the option is enabled in the GUI.

The new option is disabled by default.

Copy link
Member

@criezy criezy left a comment

I think this is a good idea, but a few things need to be changed.
See the comments I left.

In addition, whenever you add elements to the theme files, you also need to increase the theme version in all the THEMERC files and in gui/ThemeEngine.h. This way ScummVM can detect when an old theme file is in the path and avoid crashing when trying to use it.

And while you are at it, maybe you could also add in the GUI a checkbox to expose the confirm_exit config option? Currently it needs to be added manually to the config file if we want to enable it.

backends/events/default/default-events.cpp Outdated Show resolved Hide resolved
gui/options.cpp Outdated Show resolved Hide resolved
@@ -2069,13 +2070,26 @@ void GlobalOptionsDialog::addMiscControls(GuiObject *boss, const Common::String
_autosavePeriodPopUp->appendEntry(_(savePeriodLabels[i]), savePeriodValues[i]);
}

_guiReturnToLauncherAtExit = new CheckboxWidget(boss, prefix + "ReturnToLauncherAtExit",

This comment has been minimized.

@criezy

criezy Nov 14, 2020
Member

Please add a check on the kFeatureNoQuit here. If it is on, this option makes no sense so we should probably not add it to the GUI. I though about it, but forgot to comment during my initial review.

This comment has been minimized.

@lotharsm

lotharsm Nov 14, 2020
Author Member

Ah, right - I absolutely missed that too. Thanks, fix incoming!

@deepcode-ci-bot
Copy link

@deepcode-ci-bot deepcode-ci-bot bot commented Nov 14, 2020

DeepCode's analysis on #edcf55 found:

  • ⚠️ 1 warning 👇

Top issues

Description Example fixes
Potential nullptr dereference. Null flows from nullptr literal. Consider adding a check. Occurrences: 🔧 Example fixes

👉 View analysis in DeepCode’s Dashboard | Configure the bot

@criezy
Copy link
Member

@criezy criezy commented Nov 14, 2020

The commit history is not ideal. With commit a7eb84f you increase the theme version bit did not regenerate the theme files. So trying to run this specific commit will likely fail to load the theme. This could be an issue when doing git bisect. I think the best here would be to squash a18c482 and a7eb84f and also includes updated themes zips with the new version (8.40, not 8.41 that was changed later) in that squashed commit.

@lotharsm lotharsm force-pushed the lotharsm:return-to-launcher-at-exit branch from 8ad89f4 to 8eab93c Nov 14, 2020
@lotharsm
Copy link
Member Author

@lotharsm lotharsm commented Nov 14, 2020

Welp, rebasing went horribly wrong - while trying to inject a commit with theme files for 8.40, my branch somewhat disintegrated. Might be possible that I need to squash everything into one single commit.

@lotharsm lotharsm force-pushed the lotharsm:return-to-launcher-at-exit branch 2 times, most recently from 90c1415 to 8eab93c Nov 14, 2020
@lotharsm lotharsm force-pushed the lotharsm:return-to-launcher-at-exit branch from 8eab93c to edcf559 Nov 14, 2020
@lotharsm
Copy link
Member Author

@lotharsm lotharsm commented Nov 14, 2020

Rebasing done - injecting the binary stuff was quite a nightmare. Note to myself: Be careful before pushing.

@criezy
criezy approved these changes Nov 15, 2020
Copy link
Member

@criezy criezy left a comment

It looks good to me now.

@macca8
Copy link

@macca8 macca8 commented Nov 16, 2020

With respect to the 'confirm_exit' option, I appreciate that this PR is to include the option in the GUI , rather than its performance. However, would it be feasible as a follow up to this PR to improve the messaging of its dialog?
As it stands, "Do you really want to (exit)?" doesn't offer a reason for why a user is being asked to confirm their exit. May I suggest that the addition of a second line - e.g. "Any unsaved changes will be lost." - would clarify that position, and would be appropriate regardless of whether or not the user may have already saved.

It would also function as a handy fallback position until such time as a proper prompt-to-save feature can be added to ScummVM's core code, which is apparently a major undertaking.

@lotharsm
Copy link
Member Author

@lotharsm lotharsm commented Nov 16, 2020

With respect to the 'confirm_exit' option, I appreciate that this PR is to include the option in the GUI , rather than its performance. However, would it be feasible as a follow up to this PR to improve the messaging of its dialog?
As it stands, "Do you really want to (exit)?" doesn't offer a reason for why a user is being asked to confirm their exit. May I suggest that the addition of a second line - e.g. "Any unsaved changes will be lost." - would clarify that position, and would be appropriate regardless of whether or not the user may have already saved.

That's a really great idea! I consider this as being in the scope of this PR, so I'll update it soon. I still want to wait until #2591 is merged since I need to update the theme files again and fix the numbering. Since my branch is already quite "hacky", it might be better if I squash all theming related commits so I can bump the THEMERC version properly.

It would also function as a handy fallback position until such time as a proper prompt-to-save feature can be added to ScummVM's core code, which is apparently a major undertaking.

Very nice idea too - this will maybe be my next adventure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.