Skip to content

Qt: Remove a few more instances of QDialog::exec()#3623

Merged
stenzek merged 1 commit intostenzek:masterfrom
Pesa:qdialog-exec
Nov 15, 2025
Merged

Qt: Remove a few more instances of QDialog::exec()#3623
stenzek merged 1 commit intostenzek:masterfrom
Pesa:qdialog-exec

Conversation

@Pesa
Copy link
Contributor

@Pesa Pesa commented Nov 10, 2025

Need to do some more testing.

@stenzek
Copy link
Owner

stenzek commented Nov 10, 2025

FYI, it's only needed when the dialog is window modal. Application modal is fine. Hence why I didn't bother replacing these.

@Pesa Pesa force-pushed the qdialog-exec branch 2 times, most recently from 8f1656a to 84d0b16 Compare November 12, 2025 05:01
@Pesa
Copy link
Contributor Author

Pesa commented Nov 12, 2025

Yeah, this PR isn't really meant to fix anything (nothing is broken here). I just feel it's cleaner to avoid the nested event loops started by exec() as much as possible. They're fragile and error prone, and Qt's documentation recommends using open() or show() instead. But I can abandon the PR if you're not sold on the idea, not a big deal.

@Pesa
Copy link
Contributor Author

Pesa commented Nov 14, 2025

Sorry for the large-ish commit. To summarize:

  • Extracted three two new classes: AudioStretchSettingsDialog, MultipleDeviceAutobindDialog and TextureReplacementSettingsDialog for better readability and separation of responsibilities. MultipleDeviceAutobindDialog is also used by the setup wizard so it must be exposed in a header, the other two are is hidden in an unnamed namespace. The diff in these three two cases looks large but it's mostly just code moved around with trivial renames.
  • Changed a few dialogs to window modal: "add breakpoint", "advanced SDL settings", "LED settings", "new/edit game cheat code", "rename memory card file". Didn't see a good reason for them to be app modal. Moreover, these are now shown as sheets on macOS, which looks nicer.
  • Four dialogs (AchievementLoginDialog, InputBindingDialog, PostProcessingSelectShaderDialog, and the "compatibility report") were already window modal, I just changed show() to open() for consistency. No functional change.
  • The "About" dialog and the "Third-Party Notices" dialog are changed from app modal to window modal as well. I don't think these should be modal at all ("About Qt" isn't), so I might send another PR later on to make them non-modal.
  • Centralized enabling WA_DeleteOnClose on message boxes. No functional change.
  • Removed redundant modal / windowModality properties from .ui files. No functional change.

@Pesa Pesa marked this pull request as ready for review November 14, 2025 03:20
@stenzek
Copy link
Owner

stenzek commented Nov 14, 2025

I'm not really sure what the benefit to creating the new classes is - they're not moc-invokable, and there's no point keeping the pointers to the widgets around since they're not accessed afterwards?

@Pesa
Copy link
Contributor Author

Pesa commented Nov 14, 2025

Good question. I was mainly following the pattern that when you have a .ui file you create a corresponding class with Ui::Foo as data member. Otherwise you have to allocate that Ui::Foo manually and keep track of it. In particular, I'm not sure when/where it can be safely deleted.

@stenzek
Copy link
Owner

stenzek commented Nov 14, 2025

You shouldn't need to keep the Ui::Whatever instance live. It allocates all the widgets on the heap and parents them to the widget that's passed in.


QMessageBox* const mb = QtUtils::NewMessageBox(QMessageBox::Warning, tr("CPU Overclocking Warning"), message,
QMessageBox::NoButton, QMessageBox::NoButton, Qt::WindowModal, this);
mb->setAttribute(Qt::WA_DeleteOnClose, true);
Copy link
Owner

Choose a reason for hiding this comment

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

This should not be removed. Might be easier to just merge and re-add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just moved it to QtUtils::NewMessageBox()

@stenzek stenzek merged commit 4850471 into stenzek:master Nov 15, 2025
9 checks passed
@Pesa Pesa deleted the qdialog-exec branch November 15, 2025 17:53
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.

2 participants