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

Fix multiple settings dialog #66

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Significantbits
Copy link

This pull request aims to close issue #59. To get the desired behavior, I made the settings window a member of the MainWindow class. Then I simply check if the dialog is already visible before calling exec_(). Something else I added is that if the settings dialog is already open and the user clicks Edit->Settings, the settings window is brought to the front by calling the raise_() function.

This pull request also fixes the issue that I mentioned in my comment on #59. So, closing the xqemu-manager now also closes the settings window. I accomplished this by adding a closeEvent method to the MainWindow class. I think this should of been there from the beginning. The onExitClicked code is only called from the File->Exit button and not when the window is closed.

A downside to closing the settings window when the main window is closed is that currently the code doesn't save your settings before closing the window. So, the user's settings could be lost. I think this can be fixed with issue #15. Maybe we can detect if the settings have changed without being saved then on exit ask the user if they would like to save them before closing.

Please reply if I've done anything wrong or if this should be split up into two separate issues.

@JayFoxRox
Copy link
Member

Thanks!

Please clean up your git history (no line wrapping, no merge commits).
Code looks good but I didn't try it yet.

A downside to closing the settings window when the main window is closed is that currently the code doesn't save your settings before closing the window. So, the user's settings could be lost.

That would be an issue that should be fixed first.

Is this still an issue in this PR? I'm not too familiar with XQEMU-Manager, but you seem to call the close() on the settings window before exiting - I'd assume that to trigger a save? (Doesn't even editing a text field trigger a save?)

Maybe we can detect if the settings have changed without being saved then on exit ask the user if they would like to save them before closing.

Yes. If we add a save button this would be expected.


A small heads up: @mborgerson is currently working on a new XQEMU GUI (which lives in the actual QEMU). However, there is no ETA or public code for it yet.
So while we still care about XQEMU-Manager temporarily I'd expect it to be dead soon unless @mborgerson stops working on his GUI.

@Significantbits
Copy link
Author

Editing a text field doesn't trigger a save. The settings are saved to a json file which only gets written to in the save method of the SettingsManager. This only gets called in the onSettingsClicked event. I can call that method in closeEvent. However, I personally feel that if a user closes the main window after making changes to their settings, it is unclear what their intentions are. They may expect the settings to be forgotten, so I didn't add it. Should I add code that pops up a dialog box and asks the user if they want their settings changed to this pull request, or should that be apart of issue #15?

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

Successfully merging this pull request may close these issues.

None yet

2 participants