[trojita] src/Gui: GUI: Reduce indirect QDialog::exec calls

Jan Kundrát jkt at kde.org
Mon Sep 26 08:48:58 UTC 2016


Git commit f3373660e48ffd38c123afddaabe6b7a7e733aaa by Jan Kundrát.
Committed on 21/09/2016 at 00:09.
Pushed by gerrit into branch 'master'.

GUI: Reduce indirect QDialog::exec calls

QDialog::exec is dangerous because it re-enters the event loop. We've
got a report of a crash that can be traced back to
MainWindow::cacheError which led to further event processing up to
Gui::MainWindow::slotPluginsChanged which tried to manipulate QActins
which weren't really instantiated at that time.

Reported on Haiku (!) by Zoltán Mizsei ("miqlas" on IRC).

Change-Id: Ibd24040c67341961b69cd058253fceadf5a6ea2a

M  +3    -2    src/Gui/ComposeWidget.cpp
M  +3    -3    src/Gui/SettingsDialog.cpp
M  +5    -2    src/Gui/TagListWidget.cpp
M  +23   -0    src/Gui/Util.cpp
M  +3    -0    src/Gui/Util.h
M  +21   -21   src/Gui/Window.cpp

http://commits.kde.org/trojita/f3373660e48ffd38c123afddaabe6b7a7e733aaa

diff --git a/src/Gui/ComposeWidget.cpp b/src/Gui/ComposeWidget.cpp
index b84449d..153c2a0 100644
--- a/src/Gui/ComposeWidget.cpp
+++ b/src/Gui/ComposeWidget.cpp
@@ -439,8 +439,9 @@ ComposeWidget *ComposeWidget::createReply(MainWindow *mainWindow, const Composer
                          "You might want to use the \"Reply All\" function and trim the list of addresses manually.");
             break;
         }
-        if (!err.isEmpty())
-            QMessageBox::warning(w, tr("Cannot Determine Recipients"), err);
+        if (!err.isEmpty()) {
+            Gui::Util::messageBoxWarning(w, tr("Cannot Determine Recipients"), err);
+        }
     }
     w->placeOnMainWindow();
     w->show();
diff --git a/src/Gui/SettingsDialog.cpp b/src/Gui/SettingsDialog.cpp
index 5e6ccf0..ae5e913 100644
--- a/src/Gui/SettingsDialog.cpp
+++ b/src/Gui/SettingsDialog.cpp
@@ -211,9 +211,9 @@ void SettingsDialog::slotAccept()
         }
     }
     if (!passwordFailures.isEmpty()) {
-        QMessageBox::warning(this, tr("Saving passwords failed"),
-                             tr("<p>Couldn't save passwords. These were the error messages:</p>\n<p>%1</p>")
-                                .arg(passwordFailures.join(QStringLiteral("<br/>"))));
+        Gui::Util::messageBoxWarning(this, tr("Saving passwords failed"),
+                                     tr("<p>Couldn't save passwords. These were the error messages:</p>\n<p>%1</p>")
+                                     .arg(passwordFailures.join(QStringLiteral("<br/>"))));
     }
 
     buttons->setEnabled(true);
diff --git a/src/Gui/TagListWidget.cpp b/src/Gui/TagListWidget.cpp
index fa2df43..ed71cbb 100644
--- a/src/Gui/TagListWidget.cpp
+++ b/src/Gui/TagListWidget.cpp
@@ -32,6 +32,7 @@
 #include "TagListWidget.h"
 #include "FlowLayout.h"
 #include "TagWidget.h"
+#include "Gui/Util.h"
 #include "Imap/Model/SpecialFlagNames.h"
 
 namespace Gui
@@ -103,8 +104,10 @@ void TagListWidget::newTagsRequested()
         }
     }
     if (!reservedTagsList.isEmpty()) {
-        QMessageBox::warning(this, tr("Disallowed tag value"),
-                             tr("No tags were set because the following given tag(s) are reserved and have been disallowed from being set in this way: %1.").arg(reservedTagsList.join(QStringLiteral(", "))));
+        Gui::Util::messageBoxWarning(this, tr("Disallowed tag value"),
+                                     tr("No tags were set because the following given tag(s) are reserved "
+                                        "and have been disallowed from being set in this way: %1.")
+                                     .arg(reservedTagsList.join(QStringLiteral(", "))));
         return;
     }
 
diff --git a/src/Gui/Util.cpp b/src/Gui/Util.cpp
index 2cf7c95..b9b0620 100644
--- a/src/Gui/Util.cpp
+++ b/src/Gui/Util.cpp
@@ -36,6 +36,17 @@
 #include "Util.h"
 #include "Window.h"
 
+namespace {
+
+void messageBoxImpl(QWidget *parent, const QString &title, const QString &message, const QMessageBox::Icon icon)
+{
+    Q_ASSERT(parent);
+    auto box = new QMessageBox(icon, title, message, QMessageBox::Ok, parent);
+    box->open();
+}
+
+}
+
 namespace Gui {
 
 namespace Util {
@@ -94,6 +105,18 @@ QString resizedImageAsDataUrl(const QString &fileName, const int extent)
     return QLatin1String("data:image/png;base64,") + QString::fromUtf8(bdata.toBase64());
 }
 
+/** @short QMessageBox::critical, but without reentering the event loop */
+void messageBoxCritical(QWidget *parent, const QString &title, const QString &message)
+{
+    messageBoxImpl(parent, title, message, QMessageBox::Critical);
+}
+
+/** @short QMessageBox::warning, but without reentering the event loop */
+void messageBoxWarning(QWidget *parent, const QString &title, const QString &message)
+{
+    messageBoxImpl(parent, title, message, QMessageBox::Warning);
+}
+
 } // namespace Util
 
 } // namespace Gui
diff --git a/src/Gui/Util.h b/src/Gui/Util.h
index 9713c3e..09883bc 100644
--- a/src/Gui/Util.h
+++ b/src/Gui/Util.h
@@ -46,6 +46,9 @@ int askForSomethingUnlessTold(const QString &title, const QString &message, cons
 
 QString resizedImageAsDataUrl(const QString &fileName, const int extent);
 
+void messageBoxCritical(QWidget *parent, const QString &title, const QString &message);
+void messageBoxWarning(QWidget *parent, const QString &title, const QString &message);
+
 } // namespace Util
 
 } // namespace Gui
diff --git a/src/Gui/Window.cpp b/src/Gui/Window.cpp
index 4ab6596..24715f5 100644
--- a/src/Gui/Window.cpp
+++ b/src/Gui/Window.cpp
@@ -117,11 +117,11 @@ MainWindow::MainWindow(QSettings *settings): QMainWindow(), m_imapAccess(0), m_m
                                                  Common::SettingsNames::addressbookPlugin, Common::SettingsNames::passwordPlugin);
     connect(m_pluginManager, &Plugins::PluginManager::pluginsChanged, this, &MainWindow::slotPluginsChanged);
     connect(m_pluginManager, &Plugins::PluginManager::pluginError, this, [this](const QString &errorMessage) {
-        QMessageBox::warning(this, tr("Plugin Error"),
-                             //: The %1 placeholder is a full error message as provided by Qt, ready for human consumption.
-                             trUtf8("A plugin failed to load, therefore some functionality might be lost. "
-                                    "You might want to update your system or report a bug to your vendor."
-                                    "\n\n%1").arg(errorMessage));
+        Gui::Util::messageBoxWarning(this, tr("Plugin Error"),
+                                     //: The %1 placeholder is a full error message as provided by Qt, ready for human consumption.
+                                     trUtf8("A plugin failed to load, therefore some functionality might be lost. "
+                                            "You might want to update your system or report a bug to your vendor."
+                                            "\n\n%1").arg(errorMessage));
     });
 #ifdef TROJITA_HAVE_CRYPTO_MESSAGES
     Plugins::PluginManager::MimePartReplacers replacers;
@@ -1199,12 +1199,12 @@ void MainWindow::slotResyncMbox()
 void MainWindow::alertReceived(const QString &message)
 {
     //: "ALERT" is a special warning which we're required to show to the user
-    QMessageBox::warning(this, tr("IMAP Alert"), message);
+    Gui::Util::messageBoxWarning(this, tr("IMAP Alert"), message);
 }
 
 void MainWindow::imapError(const QString &message)
 {
-    QMessageBox::critical(this, tr("IMAP Protocol Error"), message);
+    Gui::Util::messageBoxCritical(this, tr("IMAP Protocol Error"), message);
     // Show the IMAP logger -- maybe some user will take that as a hint that they shall include it in the bug report.
     // </joke>
     showImapLogger->setChecked(true);
@@ -1233,10 +1233,10 @@ void MainWindow::networkError(const QString &message)
 
 void MainWindow::cacheError(const QString &message)
 {
-    QMessageBox::critical(this, tr("IMAP Cache Error"),
-                          tr("The caching subsystem managing a cache of the data already "
-                             "downloaded from the IMAP server is having troubles. "
-                             "All caching will be disabled.\n\n%1").arg(message));
+    Gui::Util::messageBoxCritical(this, tr("IMAP Cache Error"),
+                                  tr("The caching subsystem managing a cache of the data already "
+                                     "downloaded from the IMAP server is having troubles. "
+                                     "All caching will be disabled.\n\n%1").arg(message));
 }
 
 void MainWindow::networkPolicyOffline()
@@ -1291,8 +1291,8 @@ void MainWindow::slotShowSettings()
     QString method = m_settings->value(Common::SettingsNames::imapMethodKey).toString();
     if (method != Common::SettingsNames::methodTCP && method != Common::SettingsNames::methodSSL &&
             method != Common::SettingsNames::methodProcess ) {
-        QMessageBox::critical(this, tr("No Configuration"),
-                              trUtf8("No IMAP account is configured. Trojitá cannot do much without one."));
+        Gui::Util::messageBoxCritical(this, tr("No Configuration"),
+                                      trUtf8("No IMAP account is configured. Trojitá cannot do much without one."));
     }
     applySizesAndState();
 }
@@ -1808,20 +1808,20 @@ MSA::MSAFactory *MainWindow::msaFactory()
 
 void MainWindow::slotMailboxDeleteFailed(const QString &mailbox, const QString &msg)
 {
-    QMessageBox::warning(this, tr("Can't delete mailbox"),
-                         tr("Deleting mailbox \"%1\" failed with the following message:\n%2").arg(mailbox, msg));
+    Gui::Util::messageBoxWarning(this, tr("Can't delete mailbox"),
+                                 tr("Deleting mailbox \"%1\" failed with the following message:\n%2").arg(mailbox, msg));
 }
 
 void MainWindow::slotMailboxCreateFailed(const QString &mailbox, const QString &msg)
 {
-    QMessageBox::warning(this, tr("Can't create mailbox"),
-                         tr("Creating mailbox \"%1\" failed with the following message:\n%2").arg(mailbox, msg));
+    Gui::Util::messageBoxWarning(this, tr("Can't create mailbox"),
+                                 tr("Creating mailbox \"%1\" failed with the following message:\n%2").arg(mailbox, msg));
 }
 
 void MainWindow::slotMailboxSyncFailed(const QString &mailbox, const QString &msg)
 {
-    QMessageBox::warning(this, tr("Can't open mailbox"),
-                         tr("Opening mailbox \"%1\" failed with the following message:\n%2").arg(mailbox, msg));
+    Gui::Util::messageBoxWarning(this, tr("Can't open mailbox"),
+                                 tr("Opening mailbox \"%1\" failed with the following message:\n%2").arg(mailbox, msg));
 }
 
 void MainWindow::slotMailboxChanged(const QModelIndex &mailbox)
@@ -1969,8 +1969,8 @@ void MainWindow::slotSaveCurrentMessageBody()
 
 void MainWindow::slotDownloadTransferError(const QString &errorString)
 {
-    QMessageBox::critical(this, tr("Can't save into file"),
-                          tr("Unable to save into file. Error:\n%1").arg(errorString));
+    Gui::Util::messageBoxCritical(this, tr("Can't save into file"),
+                                  tr("Unable to save into file. Error:\n%1").arg(errorString));
 }
 
 void MainWindow::slotDownloadMessageFileNameRequested(QString *fileName)


More information about the kde-doc-english mailing list