search

xqemu / xqemu-manager

Simple graphical user interface to manage XQEMU
http://xqemu.com
GNU General Public License v2.0
34 stars 10 forks source link

Fix multiple settings dialog #66

Open Significantbits opened 4 years ago

Significantbits commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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?