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

Qt: Replace some QApplication::processEvents() and QDialog::exec() #13876

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

elad335
Copy link
Contributor

@elad335 elad335 commented May 17, 2023

Both functions are discouraged by Qt for various reasons, it is of course unrealistic to replace them all.
Didn't test. (yet)


vfs_dialog_usb_input* input_dialog = new vfs_dialog_usb_input(device_name, default_info, &info, m_gui_settings, this);
if (input_dialog->exec() == QDialog::Accepted)
vfs_dialog_usb_input* input_dialog = new vfs_dialog_usb_input(device_name, *default_info, info.get(), m_gui_settings, this);
Copy link
Contributor

@Megamouse Megamouse May 17, 2023

Choose a reason for hiding this comment

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

this is horrible.
you're passing raw pointers here which are captured inside the dialog.
Either make the constuctor arguments shared or use const refs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Their ownership is captured in the lambda below.

Copy link
Contributor

Choose a reason for hiding this comment

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

that's still horrible software design.
the code should be consistent in its parts, and if you just look at the tab code it makes no sense at all and will cause some regression in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Parent events are still executing even from QDialog::exec(), if som code does not currently doesn't abide it then it's a bug.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're completely missing the point.

for (const auto& serial : serials)
{
if (pdlg->wasCanceled())
BatchActionBySerials(pdlg, serials, tr("%0/%1 custom pad configurations cleared"),
Copy link
Contributor

@Megamouse Megamouse May 17, 2023

Choose a reason for hiding this comment

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

I think this is all way too complex and detached from the reading flow.

Just create a (new QFutureWatcher(this)) and connect its resultReady signal to the pdlg::setValue.
Then connect the finish signal to deleteLater of QFutureWatcher and stuff.

This would keep the code contained in this function without too much overhead instead of having one huge complex thing a couple of hundred lines above

@Megamouse Megamouse added GUI Refactoring Refactors or simplifies existing code labels May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GUI Refactoring Refactors or simplifies existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants