Improve user interface of manage definitions dialog.

Task-number: QTCREATORBUG-1886

src/plugins/texteditor/generichighlighter/definitiondownloader.cpp

@@ -32,6 +32,7 @@
 #include <QtCore/QLatin1Char>
 #include <QtCore/QEventLoop>
 #include <QtCore/QFile>
+#include <QtCore/QScopedPointer>
 #include <QtNetwork/QNetworkAccessManager>
 #include <QtNetwork/QNetworkRequest>
 #include <QtNetwork/QNetworkReply>
@@ -40,33 +41,31 @@ using namespace TextEditor;
 using namespace Internal;
 
 DefinitionDownloader::DefinitionDownloader(const QUrl &url, const QString &localPath) :
-    m_url(url), m_localPath(localPath)
+    m_url(url), m_localPath(localPath), m_status(Unknown)
 {}
 
-void DefinitionDownloader::start()
+void DefinitionDownloader::run()
 {
-    QNetworkReply *reply;
     QNetworkAccessManager manager;
 
     int currentAttempt = 0;
     const int maxAttempts = 5;
     while (currentAttempt < maxAttempts) {
-        reply = getData(&manager);
-        if (reply->error() != QNetworkReply::NoError)
-            break;
+        QScopedPointer<QNetworkReply> reply(getData(&manager));
+        if (reply->error() != QNetworkReply::NoError) {
+            m_status = NetworkError;
+            return;
+        }
 
         ++currentAttempt;
         QVariant variant = reply->attribute(QNetworkRequest::RedirectionTargetAttribute);
         if (variant.isValid() && currentAttempt < maxAttempts) {
             m_url = variant.toUrl();
-            delete reply;
         } else if (!variant.isValid()) {
-            saveData(reply);
-            break;
+            saveData(reply.data());
+            return;
         }
     }
-
-    delete reply;
 }
 
 QNetworkReply *DefinitionDownloader::getData(QNetworkAccessManager *manager) const
@@ -81,14 +80,20 @@ QNetworkReply *DefinitionDownloader::getData(QNetworkAccessManager *manager) con
     return reply;
 }
 
-void DefinitionDownloader::saveData(QNetworkReply *reply) const
+void DefinitionDownloader::saveData(QNetworkReply *reply)
 {
     const QString &urlPath = m_url.path();
     const QString &fileName =
         urlPath.right(urlPath.length() - urlPath.lastIndexOf(QLatin1Char('/')) - 1);
     QFile file(m_localPath + fileName);
-    if (!file.open(QIODevice::Text | QIODevice::WriteOnly))
-        return;
-    file.write(reply->readAll());
-    file.close();
+    if (file.open(QIODevice::Text | QIODevice::WriteOnly)) {
+        file.write(reply->readAll());
+        file.close();
+        m_status = Ok;
+    } else {
+        m_status = WriteError;
+    }
 }
+
+DefinitionDownloader::Status DefinitionDownloader::status() const
+{ return m_status; }

src/plugins/texteditor/generichighlighter/definitiondownloader.h

@@ -48,25 +48,34 @@ class DefinitionDownloader : public QObject
 public:
     DefinitionDownloader(const QUrl &url, const QString &localPath);
 
-    void start();
+    enum Status {
+        NetworkError,
+        WriteError,
+        Ok,
+        Unknown
+    };
+
+    void run();
+    Status status() const;
 
 private:
     QNetworkReply *getData(QNetworkAccessManager *manager) const;
-    void saveData(QNetworkReply *reply) const;
+    void saveData(QNetworkReply *reply);
 
     QUrl m_url;
     QString m_localPath;
+    Status m_status;
 };
 
 // Currently QtConcurrent::map does not support passing member functions for sequence of pointers
 // (only works for operator.*) which is the case for the downloaders held by the manager. Then the
 // reason for the following functor. If something is implemented (for example a type traits) to
 // handle operator->* in QtConcurrent::map this functor will not be necessary since it would be
-// possible to directly pass DefinitionDownloader::start.
+// possible to directly pass DefinitionDownloader::run.
 struct DownloaderStarter
 {
     void operator()(DefinitionDownloader *downloader)
-    { downloader->start(); }
+    { downloader->run(); }
 };
 
 } // namespace Internal

src/plugins/texteditor/generichighlighter/manager.cpp

@@ -58,6 +58,7 @@
 #include <QtCore/QtConcurrentMap>
 #include <QtCore/QUrl>
 #include <QtGui/QDesktopServices>
+#include <QtGui/QMessageBox>
 #include <QtXml/QXmlSimpleReader>
 #include <QtXml/QXmlInputSource>
 #include <QtXml/QXmlStreamReader>
@@ -289,6 +290,14 @@ void Manager::downloadDefinitions(const QList<QUrl> &urls)
         TextEditorSettings::instance()->highlighterSettings().definitionFilesPath() +
         QLatin1Char('/');
 
+    QDir saveDir(savePath);
+    if (!saveDir.exists()) {
+        QMessageBox::critical(0,
+                              tr("Error"),
+                              tr("Please make sure the destination directory exists."));
+        return;
+    }
+
     m_downloaders.clear();
     foreach (const QUrl &url, urls)
         m_downloaders.append(new DefinitionDownloader(url, savePath));
@@ -303,8 +312,28 @@ void Manager::downloadDefinitions(const QList<QUrl> &urls)
 
 void Manager::downloadDefinitionsFinished()
 {
-    foreach (DefinitionDownloader *downloader, m_downloaders)
+    int errors = 0;
+    bool writeError = false;
+    foreach (DefinitionDownloader *downloader, m_downloaders) {
+        DefinitionDownloader::Status status = downloader->status();
+        if (status != DefinitionDownloader::Ok) {
+            ++errors;
+            if (status == DefinitionDownloader::WriteError && !writeError)
+                writeError = true;
+        }
         delete downloader;
+    }
+
+    if (errors > 0) {
+        QString text;
+        if (errors == m_downloaders.size())
+            text = tr("Error downloading selected definition(s).");
+        else
+            text = tr("Error downloading one or more definitions.");
+        if (writeError)
+            text.append(tr("\nPlease check the directory's access rights."));
+        QMessageBox::critical(0, tr("Download Error"), text);
+    }
 
     registerMimeTypes();
     m_downloadingDefinitions = false;