Feature/improve desktop integration dialog #400
Conversation
|
This is the first PR of many, the main focus will be to improve the integration dialog, please review. |
|
Any updates? |
There was a problem hiding this comment.
Sorry it took so long, but I only got to having a look at your PR now. I like the change overall, but found a few things I'd like to be changed, mostly minor stuff.
I'd highly appreciate an example screenshot in UI-related PRs by the way. I also try to always provide screenshots in projects I contribute to.
|
Your screenshot looks pretty good, although the window should probably expand vertically, like the old dialog did. |
We fear that it may cause errors somewhere else as the documentation says nothing about if it's safe or not.
| // cannot use messageBox.exec(), will produce SEGFAULTS as QCoreApplications can't show message boxes | ||
| messageBox->show(); | ||
| // As the integration dialog is the only window in our application we can safely use its exec method | ||
| integrationDialog->exec(); |
There was a problem hiding this comment.
Now I'm curious: why are you calling the dialog's exec() now? I'm pretty sure you used to call QApplication::exec().
There was a problem hiding this comment.
In this scenario the QDialog is the only window and the only part of the code that relies on the QEventsLoop. Which meas that once the dialog exits we will not required the events loop enymore.
The QDialog::exec method starts an evenstloop for its sole use, therefore is a good fit. We would have to use the QAppliaction::exec if there were other events (signals/slots) to be processed after the dialog is closed.
There was a problem hiding this comment.
Exactly what I was thinking, too. Thanks for clarifying. We can always fix/improve if we get rid of the solely synchronous workflow (although I'm sure there's higher priorities).
|
Thanks again! |
Replace the inline "Desktop Integration" dialog with a class. This will allow us to customize the dialog in the future.