Fix incorrect handling of provider resolution for offline storage

This patch fixes the incorrect handling of the population/repopulation
of OSM offline tiles when the providers are initially populated.
In this way it is now fine if the user selects high dpi tiles, and has
both high dpi and low dpi offline tiles, and the high dpi provider
is unavailable.

Change-Id: I4d7080d6f259663bf6ca7a985fd9784fb81cfec3
Reviewed-by: Alex Blasche <alexander.blasche@qt.io>

2 files changed, 45 insertions, 32 deletions

diff --git a/src/plugins/geoservices/osm/qgeofiletilecacheosm.cpp b/src/plugins/geoservices/osm/qgeofiletilecacheosm.cpp
index a36f15b3..f932d3de 100644
--- a/src/plugins/geoservices/osm/qgeofiletilecacheosm.cpp
+++ b/src/plugins/geoservices/osm/qgeofiletilecacheosm.cpp
@@ -49,13 +49,14 @@ QGeoFileTileCacheOsm::QGeoFileTileCacheOsm(const QVector<QGeoTileProviderOsm *>
                                            const QString &offlineDirectory,
                                            const QString &directory,
                                            QObject *parent)
-:   QGeoFileTileCache(directory, parent), m_offlineDirectory(offlineDirectory), m_requestCancel(0), m_providers(providers)
+:   QGeoFileTileCache(directory, parent), m_offlineDirectory(offlineDirectory), m_providers(providers)
 {
     m_highDpi.resize(providers.size());
     for (int i = 0; i < providers.size(); i++) {
         providers[i]->setParent(this);
         m_highDpi[i] = providers[i]->isHighDpi();
         m_mapIdFutures[providers[i]->mapType().mapId()].isFinished(); // To construct a default future for this mapId
+        m_requestCancel[providers[i]->mapType().mapId()] = 0;
         connect(providers[i], &QGeoTileProviderOsm::resolutionFinished, this, &QGeoFileTileCacheOsm::onProviderResolutionFinished);
         connect(providers[i], &QGeoTileProviderOsm::resolutionError, this, &QGeoFileTileCacheOsm::onProviderResolutionFinished);
     }
@@ -63,10 +64,10 @@ QGeoFileTileCacheOsm::QGeoFileTileCacheOsm(const QVector<QGeoTileProviderOsm *>
 
 QGeoFileTileCacheOsm::~QGeoFileTileCacheOsm()
 {
-    m_requestCancel = 1;
-    m_future.waitForFinished();
-    for (const QGeoTileProviderOsm *p : m_providers)
+    for (const QGeoTileProviderOsm *p : m_providers) {
+        m_requestCancel[p->mapType().mapId()] = 1;
         m_mapIdFutures[p->mapType().mapId()].waitForFinished();
+    }
 }
 
 QSharedPointer<QGeoTileTexture> QGeoFileTileCacheOsm::get(const QGeoTileSpec &spec)
@@ -88,12 +89,20 @@ void QGeoFileTileCacheOsm::onProviderResolutionFinished(const QGeoTileProviderOs
             int mapId = m_providers[i]->mapType().mapId();
             m_highDpi[i] = m_providers[i]->isHighDpi();
 
+            // Terminate initOfflineRegistry future for mapId.
+            if (!m_offlineDirectory.isEmpty()) {
+                m_requestCancel[mapId] = 1;
+                m_mapIdFutures[mapId].waitForFinished();
+                m_requestCancel[mapId] = 0;
+            }
+
             // reload cache for mapId i
             dropTiles(mapId);
             loadTiles(mapId);
 
             // reload offline registry for mapId i
-            m_mapIdFutures[mapId] = QtConcurrent::run(this, &QGeoFileTileCacheOsm::initOfflineRegistry, mapId);
+            if (!m_offlineDirectory.isEmpty())
+                m_mapIdFutures[mapId] = QtConcurrent::run(this, &QGeoFileTileCacheOsm::initOfflineRegistry, mapId);
 
             // send signal to clear scene in all maps created through this provider that use the reloaded tiles
             emit mapDataUpdated(mapId);
@@ -101,7 +110,8 @@ void QGeoFileTileCacheOsm::onProviderResolutionFinished(const QGeoTileProviderOs
     }
 }
 
-// On resolution error the provider is removed ONLY if there is no enabled hardcoded fallback.
+// On resolution error the provider is removed.
+// This happens ONLY if there is no enabled hardcoded fallback for the mapId.
 // Hardcoded fallbacks also have a timestamp, that can get updated with Qt releases.
 void QGeoFileTileCacheOsm::onProviderResolutionError(const QGeoTileProviderOsm *provider, QNetworkReply::NetworkError error)
 {
@@ -109,6 +119,7 @@ void QGeoFileTileCacheOsm::onProviderResolutionError(const QGeoTileProviderOsm *
     clearObsoleteTiles(provider); // this still removes tiles who happen to be older than qgeotileproviderosm.cpp defaultTs
 }
 
+// init() is always called before the provider resolution starts
 void QGeoFileTileCacheOsm::init()
 {
     if (directory_.isEmpty())
@@ -141,11 +152,11 @@ void QGeoFileTileCacheOsm::init()
     // Base class ::init()
     QGeoFileTileCache::init();
 
-    for (QGeoTileProviderOsm * p: m_providers)
+    for (QGeoTileProviderOsm * p: m_providers) {
         clearObsoleteTiles(p);
-
-    if (!m_offlineDirectory.isEmpty())
-        m_future = QtConcurrent::run(this, &QGeoFileTileCacheOsm::initOfflineRegistry, -1);
+        if (!m_offlineDirectory.isEmpty())
+            m_mapIdFutures[p->mapType().mapId()] = QtConcurrent::run(this, &QGeoFileTileCacheOsm::initOfflineRegistry, p->mapType().mapId());
+    }
 }
 
 QSharedPointer<QGeoTileTexture> QGeoFileTileCacheOsm::getFromOfflineStorage(const QGeoTileSpec &spec)
@@ -190,6 +201,11 @@ void QGeoFileTileCacheOsm::dropTiles(int mapId)
     for (const QGeoTileSpec &k : keys)
         if (k.mapId() == mapId)
             diskCache_.remove(k);
+
+    keys = m_tilespecToOfflineFilepath.keys();
+    for (const QGeoTileSpec &k : keys)
+        if (k.mapId() == mapId)
+            m_tilespecToOfflineFilepath.remove(k);
 }
 
 void QGeoFileTileCacheOsm::loadTiles(int mapId)
@@ -211,6 +227,9 @@ void QGeoFileTileCacheOsm::loadTiles(int mapId)
 
 void QGeoFileTileCacheOsm::initOfflineRegistry(int mapId)
 {
+    if (mapId < 1) // map ids in osm start from 1
+        return;
+
     // Dealing with duplicates: picking the newest
     QMap<QString, QPair<QString, QDateTime> > fileDates; // key is filename, value is <filepath, lastmodified>
     QDirIterator it(m_offlineDirectory, QStringList() << "*.*", QDir::Files, QDirIterator::Subdirectories | QDirIterator::FollowSymlinks );
@@ -219,42 +238,37 @@ void QGeoFileTileCacheOsm::initOfflineRegistry(int mapId)
         QFileInfo f(path);
         if (!fileDates.contains(f.fileName()) || fileDates[f.fileName()].second < f.lastModified())
             fileDates[f.fileName()] = QPair<QString, QDateTime>(f.filePath(), f.lastModified());
-        if (m_requestCancel)
+        if (m_requestCancel[mapId])
             return;
     }
 
     // Clear the content of the index. Entirely (at startup), or selectively (when a provider resolution changes the highDpi status).
-    if (mapId < 0) {
-        storageLock.lock();
-        m_tilespecToOfflineFilepath.clear();
-        storageLock.unlock();
-    } else {
-        QList<QGeoTileSpec> toRemove;
-        for (auto i = m_tilespecToOfflineFilepath.constBegin(); i != m_tilespecToOfflineFilepath.constEnd(); ++i) {
-            if (i.key().mapId() == mapId)
-                toRemove.append(i.key());
-        }
-        storageLock.lock();
-        for (const auto &i : toRemove)
-            m_tilespecToOfflineFilepath.remove(i);
-        storageLock.unlock();
+    QList<QGeoTileSpec> toRemove;
+    for (auto i = m_tilespecToOfflineFilepath.constBegin(); i != m_tilespecToOfflineFilepath.constEnd(); ++i) {
+        if (i.key().mapId() == mapId)
+            toRemove.append(i.key());
     }
-    if (m_requestCancel)
+    storageLock.lock();
+    for (const auto &i : toRemove)
+        m_tilespecToOfflineFilepath.remove(i);
+    storageLock.unlock();
+
+    if (m_requestCancel[mapId])
         return;
 
-    // Fill the index entirely or selectively
+    // Fill the index by mapId
     int count = 0;
     for (auto i= fileDates.constBegin(); i != fileDates.constEnd(); ++i) {
         QGeoTileSpec spec = filenameToTileSpec(i.key());
         if (spec.zoom() == -1)
             continue;
-        if (mapId >= 0 && spec.mapId() != mapId) // if mapId != -1, pick up only those files with that mapId.
+        if (spec.mapId() != mapId)
             continue;
         count++;
         storageLock.lock();
         m_tilespecToOfflineFilepath[spec] = i.value().first;
         storageLock.unlock();
-        if (m_requestCancel)
+        if (m_requestCancel[mapId])
             return;
     }
     //qInfo() << "OSM plugin has found and is using "<< count <<" offline tiles";
diff --git a/src/plugins/geoservices/osm/qgeofiletilecacheosm.h b/src/plugins/geoservices/osm/qgeofiletilecacheosm.h
index d26cad4a..f26e7f10 100644
--- a/src/plugins/geoservices/osm/qgeofiletilecacheosm.h
+++ b/src/plugins/geoservices/osm/qgeofiletilecacheosm.h
@@ -72,13 +72,12 @@ protected:
     void dropTiles(int mapId);
     void loadTiles(int mapId);
 
-    void initOfflineRegistry(int mapId = -1);
+    void initOfflineRegistry(int mapId);
     void clearObsoleteTiles(const QGeoTileProviderOsm *p);
 
     QString m_offlineDirectory;
     QHash<QGeoTileSpec, QString> m_tilespecToOfflineFilepath;
-    QAtomicInt m_requestCancel;
-    QFuture<void> m_future;
+    QMap<int, QAtomicInt> m_requestCancel;
     QMap<int, QFuture<void>> m_mapIdFutures;
     QMutex storageLock;
     QVector<QGeoTileProviderOsm *> m_providers;