Vfs: Clear up relationship between _type and pin state

The pin state is a per-item attribute that has an effect on _type:
AlwaysLocal dehydrated files will be marked for hydration and OnlineOnly
hydrated files will be marked for dehydration.

Where exactly this effect materializes depends on how the pin states are
stored. If they're stored in the db (suffix) the dbEntry._type is
changed during the discovery.

If the pin state is stored in the filesystem, the localEntry._type must
be adjusted by the plugin's stat callback.

This patch makes pin states behave more consistently between plugins.
Previously with suffix-vfs pin states only had an effect on new remote
files. Now the effect of pinning or unpinning files or directories is as
documented and similar to other plugins.

10 files changed, 138 insertions, 81 deletions

diff --git a/src/common/pinstate.h b/src/common/pinstate.h
index 5fb267168..053e7062f 100644
--- a/src/common/pinstate.h
+++ b/src/common/pinstate.h
@@ -58,7 +58,9 @@ enum class PinState {
      * Also known as "unpinned". Unpinned hydrated files shall be dehydrated
      * as soon as possible.
      *
-     * If a unpinned file becomes hydrated its pin state changes to unspecified.
+     * If a unpinned file becomes hydrated (such as due to an implicit hydration
+     * where the user requested access to the file's data) its pin state changes
+     * to Unspecified.
      */
     OnlineOnly = 2,
 
diff --git a/src/csync/csync.h b/src/csync/csync.h
index 6952ca967..27f8aaff0 100644
--- a/src/csync/csync.h
+++ b/src/csync/csync.h
@@ -144,16 +144,26 @@ enum ItemType {
 
     /** A ItemTypeVirtualFile that wants to be hydrated.
      *
-     * Actions may put this in the db as a request to a future sync.
+     * Actions may put this in the db as a request to a future sync, such as
+     * implicit hydration (when the user wants to access file data) when using
+     * suffix vfs. For pin-state driven hydrations changing the database is
+     * not necessary.
+     *
      * For some vfs plugins the placeholder files on disk may be marked for
-     * dehydration (like with a file attribute) and then the local discovery
+     * (de-)hydration (like with a file attribute) and then the local discovery
      * will return this item type.
+     *
+     * The discovery will also use this item type to mark entries for hydration
+     * if an item's pin state mandates it, such as when encountering a AlwaysLocal
+     * file that is dehydrated.
      */
     ItemTypeVirtualFileDownload = 5,
 
     /** A ItemTypeFile that wants to be dehydrated.
      *
-     * May exist in db or local files, similar to ItemTypeVirtualFileDownload.
+     * Similar to ItemTypeVirtualFileDownload, but there's currently no situation
+     * where it's stored in the database since there is no action that triggers a
+     * file dehydration without changing the pin state.
      */
     ItemTypeVirtualFileDehydration = 6,
 };
diff --git a/src/gui/accountsettings.cpp b/src/gui/accountsettings.cpp
index ce68159b3..0188332d3 100644
--- a/src/gui/accountsettings.cpp
+++ b/src/gui/accountsettings.cpp
@@ -656,14 +656,9 @@ void AccountSettings::slotSetCurrentFolderAvailability(PinState state)
     if (!selected.isValid() || !folder)
         return;
 
-    // similar to socket api: set pin state, wipe sub pin-states and sync
+    // similar to socket api: sets pin state recursively and sync
     folder->setNewFilesAreVirtual(state == PinState::OnlineOnly);
-
-    if (state == PinState::AlwaysLocal) {
-        folder->downloadVirtualFile("");
-    } else {
-        folder->dehydrateFile("");
-    }
+    folder->scheduleThisFolderSoon();
 }
 
 void AccountSettings::showConnectionLabel(const QString &message, QStringList errors)
diff --git a/src/gui/application.cpp b/src/gui/application.cpp
index f5106a28b..bb3031ea9 100644
--- a/src/gui/application.cpp
+++ b/src/gui/application.cpp
@@ -755,7 +755,7 @@ void Application::openVirtualFile(const QString &filename)
         // TODO: show a QMessageBox for errors
         return;
     }
-    folder->downloadVirtualFile(relativePath);
+    folder->implicitlyHydrateFile(relativePath);
     QString normalName = filename.left(filename.size() - virtualFileExt.size());
     auto con = QSharedPointer<QMetaObject::Connection>::create();
     *con = QObject::connect(folder, &Folder::syncFinished, [con, normalName] {
diff --git a/src/gui/folder.cpp b/src/gui/folder.cpp
index 686b37b9a..5f6e1ef99 100644
--- a/src/gui/folder.cpp
+++ b/src/gui/folder.cpp
@@ -577,58 +577,36 @@ void Folder::slotWatchedPathChanged(const QString &path)
     scheduleThisFolderSoon();
 }
 
-void Folder::downloadVirtualFile(const QString &_relativepath)
+void Folder::implicitlyHydrateFile(const QString &relativepath)
 {
-    qCInfo(lcFolder) << "Download virtual file: " << _relativepath;
-    auto relativepath = _relativepath.toUtf8();
+    qCInfo(lcFolder) << "Implicitly hydrate virtual file:" << relativepath;
 
     // Set in the database that we should download the file
     SyncJournalFileRecord record;
-    _journal.getFileRecord(relativepath, &record);
-    if (!record.isValid() && !relativepath.isEmpty())
+    _journal.getFileRecord(relativepath.toUtf8(), &record);
+    if (!record.isValid()) {
+        qCInfo(lcFolder) << "Did not find file in db";
         return;
-    if (record._type == ItemTypeVirtualFile) {
-        record._type = ItemTypeVirtualFileDownload;
-        _journal.setFileRecord(record);
-        // Make sure we go over that file during the discovery even if
-        // no actual remote discovery would be necessary
-        _journal.schedulePathForRemoteDiscovery(relativepath);
-    } else if (record._type == ItemTypeDirectory || relativepath.isEmpty()) {
-        _journal.markVirtualFileForDownloadRecursively(relativepath);
-    } else {
-        qCWarning(lcFolder) << "Invalid existing record " << record._type << " for file " << _relativepath;
     }
-
-    // Schedule a sync (Folder man will start the sync in a few ms)
-    slotScheduleThisFolder();
-}
-
-void Folder::dehydrateFile(const QString &_relativepath)
-{
-    qCInfo(lcFolder) << "Dehydrating file: " << _relativepath;
-    auto relativepath = _relativepath.toUtf8();
-
-    auto markForDehydration = [&](SyncJournalFileRecord rec) {
-        if (rec._type != ItemTypeFile)
-            return;
-        rec._type = ItemTypeVirtualFileDehydration;
-        _journal.setFileRecord(rec);
-        _localDiscoveryTracker->addTouchedPath(relativepath);
-    };
-
-    SyncJournalFileRecord record;
-    _journal.getFileRecord(relativepath, &record);
-    if (!record.isValid() && !relativepath.isEmpty())
+    if (!record.isVirtualFile()) {
+        qCInfo(lcFolder) << "The file is not virtual";
         return;
-    if (record._type == ItemTypeFile) {
-        markForDehydration(record);
-    } else if (record._type == ItemTypeDirectory || relativepath.isEmpty()) {
-        _journal.getFilesBelowPath(relativepath, markForDehydration);
-    } else {
-        qCWarning(lcFolder) << "Invalid existing record " << record._type << " for file " << _relativepath;
+    }
+    record._type = ItemTypeVirtualFileDownload;
+    _journal.setFileRecord(record);
+
+    // Change the file's pin state if it's contradictory to being hydrated
+    // (suffix-virtual file's pin state is stored at the hydrated path)
+    QString pinPath = relativepath;
+    if (_vfs->mode() == Vfs::WithSuffix && pinPath.endsWith(_vfs->fileSuffix()))
+        pinPath.chop(_vfs->fileSuffix().size());
+    const auto pin = _vfs->pinState(pinPath);
+    if (pin && *pin == PinState::OnlineOnly) {
+        _vfs->setPinState(pinPath, PinState::Unspecified);
     }
 
-    // Schedule a sync (Folder man will start the sync in a few ms)
+    // Add to local discovery
+    schedulePathForLocalDiscovery(relativepath);
     slotScheduleThisFolder();
 }
 
@@ -671,7 +649,12 @@ bool Folder::newFilesAreVirtual() const
 
 void Folder::setNewFilesAreVirtual(bool enabled)
 {
-    _vfs->setPinState(QString(), enabled ? PinState::OnlineOnly : PinState::AlwaysLocal);
+    const auto newPin = enabled ? PinState::OnlineOnly : PinState::AlwaysLocal;
+    _vfs->setPinState(QString(), newPin);
+
+    // We don't actually need discovery, but it's important to recurse
+    // into all folders, so the changes can be applied.
+    slotNextSyncFullLocalDiscovery();
 }
 
 bool Folder::supportsSelectiveSync() const
diff --git a/src/gui/folder.h b/src/gui/folder.h
index 2217e7dfa..f5995ea61 100644
--- a/src/gui/folder.h
+++ b/src/gui/folder.h
@@ -338,21 +338,21 @@ public slots:
     void slotWatchedPathChanged(const QString &path);
 
     /**
-     * Mark a virtual file as being ready for download, and start a sync.
-     * relativepath is the path to the file (including the extension)
-     * Passing a folder means that all contained virtual items shall be downloaded.
-     * A relative path of "" downloads everything.
-     */
-    void downloadVirtualFile(const QString &relativepath);
-
-    /**
-     * Turn a regular file into a dehydrated placeholder.
+     * Mark a virtual file as being requested for download, and start a sync.
+     *
+     * "implicit" here means that this download request comes from the user wanting
+     * to access the file's data. The user did not change the file's pin state.
+     * If the file is currently OnlineOnly its state will change to Unspecified.
+     *
+     * The download request is stored by setting ItemTypeVirtualFileDownload
+     * in the database. This is necessary since the hydration is not driven by
+     * the pin state.
+     *
+     * relativepath is the folder-relative path to the file (including the extension)
      *
-     * relativepath is the path to the file
-     * It's allowed to pass a path to a folder: all contained files will be dehydrated.
-     * A relative path of "" dehydrates everything.
+     * Note, passing directories is not supported. Files only.
      */
-    void dehydrateFile(const QString &relativepath);
+    void implicitlyHydrateFile(const QString &relativepath);
 
     /** Ensures that the next sync performs a full local discovery. */
     void slotNextSyncFullLocalDiscovery();
diff --git a/src/gui/socketapi.cpp b/src/gui/socketapi.cpp
index 3406205ac..092a84d20 100644
--- a/src/gui/socketapi.cpp
+++ b/src/gui/socketapi.cpp
@@ -657,8 +657,9 @@ void SocketApi::command_MAKE_AVAILABLE_LOCALLY(const QString &filesArg, SocketLi
         auto pinPath = data.folderRelativePathNoVfsSuffix();
         data.folder->vfs().setPinState(pinPath, PinState::AlwaysLocal);
 
-        // Trigger the recursive download
-        data.folder->downloadVirtualFile(data.folderRelativePath);
+        // Trigger sync
+        data.folder->schedulePathForLocalDiscovery(data.folderRelativePath);
+        data.folder->scheduleThisFolderSoon();
     }
 }
 
@@ -676,8 +677,9 @@ void SocketApi::command_MAKE_ONLINE_ONLY(const QString &filesArg, SocketListener
         auto pinPath = data.folderRelativePathNoVfsSuffix();
         data.folder->vfs().setPinState(pinPath, PinState::OnlineOnly);
 
-        // Trigger recursive dehydration
-        data.folder->dehydrateFile(data.folderRelativePath);
+        // Trigger sync
+        data.folder->schedulePathForLocalDiscovery(data.folderRelativePath);
+        data.folder->scheduleThisFolderSoon();
     }
 }
 
diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp
index 41ce627cd..13de3edb3 100644
--- a/src/libsync/discovery.cpp
+++ b/src/libsync/discovery.cpp
@@ -88,7 +88,9 @@ void ProcessDirectoryJob::process()
             auto name = pathU8.isEmpty() ? rec._path : QString::fromUtf8(rec._path.constData() + (pathU8.size() + 1));
             if (rec.isVirtualFile() && isVfsWithSuffix())
                 chopVirtualFileSuffix(name);
-            entries[name].dbEntry = rec;
+            auto &dbEntry = entries[name].dbEntry;
+            dbEntry = rec;
+            setupDbPinStateActions(dbEntry);
         })) {
         dbError();
         return;
@@ -1448,4 +1450,30 @@ void ProcessDirectoryJob::computePinState(PinState parentState)
     }
 }
 
+void ProcessDirectoryJob::setupDbPinStateActions(SyncJournalFileRecord &record)
+{
+    // Only suffix-vfs uses the db for pin states.
+    // Other plugins will set localEntry._type according to the file's pin state.
+    if (!isVfsWithSuffix())
+        return;
+
+    QByteArray pinPath = record._path;
+    if (record.isVirtualFile()) {
+        const auto suffix = _discoveryData->_syncOptions._vfs->fileSuffix().toUtf8();
+        if (pinPath.endsWith(suffix))
+            pinPath.chop(suffix.size());
+    }
+    auto pin = _discoveryData->_statedb->internalPinStates().rawForPath(pinPath);
+    if (!pin || *pin == PinState::Inherited)
+        pin = _pinState;
+
+    // OnlineOnly hydrated files want to be dehydrated
+    if (record._type == ItemTypeFile && *pin == PinState::OnlineOnly)
+        record._type = ItemTypeVirtualFileDehydration;
+
+    // AlwaysLocal dehydrated files want to be hydrated
+    if (record._type == ItemTypeVirtualFile && *pin == PinState::AlwaysLocal)
+        record._type = ItemTypeVirtualFileDownload;
+}
+
 }
diff --git a/src/libsync/discovery.h b/src/libsync/discovery.h
index 6cc059bbd..20d27b73d 100644
--- a/src/libsync/discovery.h
+++ b/src/libsync/discovery.h
@@ -198,13 +198,25 @@ private:
       */
     bool runLocalQuery();
 
-    /** Sets _pinState
+    /** Sets _pinState, the directory's pin state
      *
      * If the folder exists locally its state is retrieved, otherwise the
      * parent's pin state is inherited.
      */
     void computePinState(PinState parentState);
 
+    /** Adjust record._type if the db pin state suggests it.
+     *
+     * If the pin state is stored in the database (suffix vfs only right now)
+     * its effects won't be seen in localEntry._type. Instead the effects
+     * should materialize in dbEntry._type.
+     *
+     * This function checks whether the combination of file type and pin
+     * state suggests a hydration or dehydration action and changes the
+     * _type field accordingly.
+     */
+    void setupDbPinStateActions(SyncJournalFileRecord &record);
+
     QueryMode _queryServer = QueryMode::NormalQuery;
     QueryMode _queryLocal = QueryMode::NormalQuery;
 
@@ -244,7 +256,7 @@ private:
     PathTuple _currentFolder;
     bool _childModified = false; // the directory contains modified item what would prevent deletion
     bool _childIgnored = false; // The directory contains ignored item that would prevent deletion
-    PinState _pinState = PinState::Unspecified; // The directory's pin-state, see setParentPinState()
+    PinState _pinState = PinState::Unspecified; // The directory's pin-state, see computePinState()
 
 signals:
     void finished();
diff --git a/test/testsyncvirtualfiles.cpp b/test/testsyncvirtualfiles.cpp
index 765d55ec4..dbb0beb27 100644
--- a/test/testsyncvirtualfiles.cpp
+++ b/test/testsyncvirtualfiles.cpp
@@ -64,8 +64,9 @@ QSharedPointer<Vfs> setupVfs(FakeFolder &folder)
     auto suffixVfs = QSharedPointer<Vfs>(createVfsFromPlugin(Vfs::WithSuffix).release());
     folder.switchToVfs(suffixVfs);
 
-    // Using this directly doesn't recursively unpin everything
-    folder.syncJournal().internalPinStates().setForPath("", PinState::OnlineOnly);
+    // Using this directly doesn't recursively unpin everything and instead leaves
+    // the files in the hydration that that they start with
+    folder.syncJournal().internalPinStates().setForPath("", PinState::Unspecified);
 
     return suffixVfs;
 }
@@ -923,7 +924,7 @@ private slots:
         setPin("online", PinState::OnlineOnly);
         setPin("unspec", PinState::Unspecified);
 
-        // Test 1: root is OnlineOnly
+        // Test 1: root is Unspecified
         fakeFolder.remoteModifier().insert("file1");
         fakeFolder.remoteModifier().insert("online/file1");
         fakeFolder.remoteModifier().insert("local/file1");
@@ -935,7 +936,7 @@ private slots:
         QVERIFY(fakeFolder.currentLocalState().find("local/file1"));
         QVERIFY(fakeFolder.currentLocalState().find("unspec/file1.owncloud"));
 
-        // Test 2: root is AlwaysLocal
+        // Test 2: change root to AlwaysLocal
         setPin("", PinState::AlwaysLocal);
 
         fakeFolder.remoteModifier().insert("file2");
@@ -949,8 +950,32 @@ private slots:
         QVERIFY(fakeFolder.currentLocalState().find("local/file2"));
         QVERIFY(fakeFolder.currentLocalState().find("unspec/file2.owncloud"));
 
-        // file1 is unchanged
+        // root file1 was hydrated due to its new pin state
+        QVERIFY(fakeFolder.currentLocalState().find("file1"));
+
+        // file1 is unchanged in the explicitly pinned subfolders
+        QVERIFY(fakeFolder.currentLocalState().find("online/file1.owncloud"));
+        QVERIFY(fakeFolder.currentLocalState().find("local/file1"));
+        QVERIFY(fakeFolder.currentLocalState().find("unspec/file1.owncloud"));
+
+        // Test 3: change root to OnlineOnly
+        setPin("", PinState::OnlineOnly);
+
+        fakeFolder.remoteModifier().insert("file3");
+        fakeFolder.remoteModifier().insert("online/file3");
+        fakeFolder.remoteModifier().insert("local/file3");
+        fakeFolder.remoteModifier().insert("unspec/file3");
+        QVERIFY(fakeFolder.syncOnce());
+
+        QVERIFY(fakeFolder.currentLocalState().find("file3.owncloud"));
+        QVERIFY(fakeFolder.currentLocalState().find("online/file3.owncloud"));
+        QVERIFY(fakeFolder.currentLocalState().find("local/file3"));
+        QVERIFY(fakeFolder.currentLocalState().find("unspec/file3.owncloud"));
+
+        // root file1 was dehydrated due to its new pin state
         QVERIFY(fakeFolder.currentLocalState().find("file1.owncloud"));
+
+        // file1 is unchanged in the explicitly pinned subfolders
         QVERIFY(fakeFolder.currentLocalState().find("online/file1.owncloud"));
         QVERIFY(fakeFolder.currentLocalState().find("local/file1"));
         QVERIFY(fakeFolder.currentLocalState().find("unspec/file1.owncloud"));