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
Delete properties dialog on close #8716
Conversation
| @@ -32,6 +32,7 @@ PropertiesDialog::PropertiesDialog(QWidget* parent, const UICommon::GameFile& ga | |||
| QString::fromStdString(game.GetGameID()), | |||
| QString::fromStdString(game.GetLongName()))); | |||
| setWindowFlags(windowFlags() & ~Qt::WindowContextHelpButtonHint); | |||
| setAttribute(Qt::WA_DeleteOnClose, true); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably have a comment accompanying it explaining why this behavior is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, maybe the code would be clear enough on its own without needing a comment if we move this line to where the PropertiesDialog gets allocated? That is how every other usage of Qt::WA_DeleteOnClose in Dolphin seems to be done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the comment, but I think what JosJuice mentions is better; I'll do that now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the comment and moved it to the allocation point. (For reference, the previous comment was The properties dialog is not re-used, and maintains a lock on the game file until it's deleted, preventing the file from being moved or deleted.)
110a7cb
to
03eb887
Compare
Fixes the game file being locked until Dolphin closes.
03eb887
to
e11a2bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I personally don't think the comment is necessary, but I don't mind having it there.
Fixes the game file being locked until Dolphin closes. The properties dialog technically didn't leak before this, since Qt will close it when its parent window closes, but that only happens when dolphin itself exits and that's way longer than it needs to be. (See also: #8088 (comment))