Create QMessageBox on heap and continue running the app when update is ready. #3566
Conversation
allexzander
force-pushed
the
bugfix/desktop-client-high-cpu-usage-on-auto-update
branch
2 times, most recently
from
46f9062 to
34eeb5e
Compare
| QMessageBox::Ok)) { | ||
| const auto messageBoxStartInstaller = new QMessageBox(QMessageBox::Information, | ||
| tr("New %1 Update Ready").arg(Theme::instance()->appNameGUI()), | ||
| tr("A new update for %1 is about to be installed. The updater may ask\n" |
There was a problem hiding this comment.
Do we really need that new line character? I think the gui should figure out line breaks on it own. If it's just because you don't want to have a long string in the source code, you can use C++ multiline strings :)
There was a problem hiding this comment.
@FlexW Not sure. I haven't checked if that newline character is not needed. Just copy-pasted that code.
src/gui/updater/ocupdater.cpp
Outdated
| .arg(name), | ||
| QMessageBox::Ok)) { | ||
| const auto messageBoxStartInstaller = new QMessageBox(QMessageBox::Information, | ||
| tr("New %1 Update Ready").arg(Theme::instance()->appNameGUI()), |
There was a problem hiding this comment.
@jancborchardt told me once we never use capitalization in titles. So should be "New %1 update ready"
There was a problem hiding this comment.
@FlexW I don't want to change this now. This will require translation. So, let's use the old text for now. Or, should we ask Jan's opinion? @jancborchardt Do you think I should change existing strings?
There was a problem hiding this comment.
I get your point, but why not change it now. At some point, we need to change it. If we do not do it now, it may never happen.
There was a problem hiding this comment.
Yeah, agree with @FlexW here – better to change things when we find them. :) (Also cause there’s probably not so many wrong anyway.)
| } | ||
| }); | ||
| connect(messageBoxStartInstaller, &QMessageBox::rejected, this, [this] { | ||
| slotStartInstaller(); |
There was a problem hiding this comment.
I am confused about this one, is this not a behavior change from before ?
why do you need it ?
There was a problem hiding this comment.
@mgallien Yes. I need to change behavior. Otherwise, I can not fix the issue, because, it is caused by incorrect behavior. We must not return true. Otherwise, QCoreApplication would never enter the event loop, because, this code:
if (updater && updater->handleStartup()) {
return 1;
}
in src/gui/main.cpp.
That's why, we must not return true when displaying the message box. Also, we should be able to call qApp->quit() when closing this message box. Otherwise, if we call it right away when displaying the message box, the QCoreApplication hasn't entered the event loop yet. This leads to calling qApp->quit() having no effect because the event loop hasn't started yet. According to Qt documentation.
There was a problem hiding this comment.
in that case, you may use QDialog::finished such that you avoid needing to connect to two signals
There was a problem hiding this comment.
@mgallien At first, I wanted to use it, but then, I've decided to make this code more deterministic so I only execute that line on accepting or rejecting and no other reason for dialog finish.
Will that QDialog::finished be triggered only when I close the dialog via the X button (ESC on keyboard) and via OK button (ENTER on keyboard) ? If that is the case, I am going to use QDialog::finished.
There was a problem hiding this comment.
finished should happen only for accepted or rejected in our case
it is always possible to call the done slot with another return code but the user can not cause that by closing the dialog
you should be safe using it
allexzander
force-pushed
the
bugfix/desktop-client-high-cpu-usage-on-auto-update
branch
2 times, most recently
from
52f7c4f to
9350e63
Compare
…ue from OCUpdater to allow for a proper app::quit Signed-off-by: allexzander <[email protected]>
allexzander
force-pushed
the
bugfix/desktop-client-high-cpu-usage-on-auto-update
branch
from
9350e63 to
801e4ad
Compare
allexzander
deleted the
bugfix/desktop-client-high-cpu-usage-on-auto-update
branch
|
AppImage file: Nextcloud-PR-3566-9350e636b2009bffac77c2cdb9589f8f98023906-x86_64.AppImage |
allexzander
changed the title
Signed-off-by: allexzander [email protected]
This should fix #2379 and #3538