Vfs adjustments for deeper shell integration #7017

ckamm · 2019-02-06T11:14:03Z

This is

some code organization changes (moves to common/) to make classes accessible in the plugin
significant pin-state updates to improve behavior and make it compatible among plugins
discovery change to allow vfs-actions originating from local files
a propagatedownload attribute propagation fix
vfs interface additions and changes
proper vfs unregistering on folder removal

ogoffart · 2019-02-08T11:53:33Z

src/csync/std/c_private.h

@@ -42,8 +42,12 @@
 #include <errno.h>

 #ifdef __MINGW32__
+#ifndef EDQUOT


Where is that even used? perhaps this can simply be removed?

Seems like it, will push fixup commits and fold in later

ogoffart · 2019-02-08T11:57:12Z

src/common/common.cmake

@@ -11,6 +11,7 @@ set(common_SOURCES
    ${CMAKE_CURRENT_LIST_DIR}/remotepermissions.cpp
    ${CMAKE_CURRENT_LIST_DIR}/vfs.cpp
    ${CMAKE_CURRENT_LIST_DIR}/plugin.cpp
+    ${CMAKE_CURRENT_LIST_DIR}/syncfilestatus.cpp


Have you considered moving whatever you need from common to libsync instead?

Since there is no more of csync left, we could move everything together.

The reason I moved it was because the Vfs declarations are in common as they're still used from csync/vio

ogoffart · 2019-02-08T11:58:25Z

src/libsync/syncfileitem.cpp

 {
    SyncJournalFileRecord rec;
-    rec._path = _file.toUtf8();
+    rec._path = destination().toUtf8();


Could there be a test for that?

Yes, would make sense to add db-checks to the SyncMove testsuites. Will do.

ogoffart · 2019-02-08T12:00:28Z

src/libsync/owncloudpropagator.cpp

-            SyncJournalFileRecord record = _item->toSyncJournalFileRecordWithInode(propagator()->_localDir + _item->_file);
-            bool ok = propagator()->_journal->setFileRecord(record);
-            if (!ok) {
+            if (!propagator()->updateMetadata(*_item)) {
                status = _item->_status = SyncFileItem::FatalError;


Some of the error handling could also be moved to the helper
Edit: Maybe not

ogoffart · 2019-02-08T12:03:04Z

src/libsync/owncloudpropagator.cpp

            qCWarning(lcCleanupPolls) << "database error";
            job->_item->_status = SyncFileItem::FatalError;
            job->_item->_errorString = tr("Error writing metadata to the database");
            emit aborted(job->_item->_errorString);
            deleteLater();
            return;
        }
+        // TODO: Is syncfilestatustracker notified somehow?


This is old unused code.
It is going to be updated by #6614

But anyway, no need to update the syncfilestatustracker because this is not about files from this sync, this happen before the discovery.

I was wondering whether SyncFileStatusTracker::slotItemCompleted would ever be emitted for a file that is finished only after polling completes. You're saying that's working fine and the comment can be removed?

ogoffart · 2019-02-08T12:09:08Z

src/common/vfs.h

@@ -31,7 +31,6 @@ typedef QSharedPointer<Account> AccountPtr;
 class SyncJournalDb;
 class VfsPrivate;
 class SyncFileItem;
-typedef QSharedPointer<SyncFileItem> SyncFileItemPtr;


(Putting a message on the first line of the commit, but i want to comment on the whole commit)

I do not understand the relation between the commit message and this commit.
It does not seem to touch directory, it is just some refactoring, is it not?

The significant change that happens in "Vfs: Make files that end up in db placeholders" is that convertToPlaceholder() is called for each updateMetadata(). Previously it was called much more sparingly and that would cause trouble down the line.

ogoffart · 2019-02-08T12:18:39Z

src/libsync/propagatedownload.cpp

@@ -972,8 +965,15 @@ void PropagateDownloadFile::downloadFinished()
    // Apply the remote permissions
    FileSystem::setFileReadOnlyWeak(_tmpFile.fileName(), !_item->_remotePerm.isNull() && !_item->_remotePerm.hasPermission(RemotePermissions::CanWrite));

-    // Make the file a hydrated placeholder if possible
-    propagator()->syncOptions()._vfs->convertToPlaceholder(_tmpFile.fileName(), *_item, fn);
+    bool isConflict = _item->_instruction == CSYNC_INSTRUCTION_CONFLICT


Could there be a test for this?

It'll be needed in vfs plugins so they can connect to the data coming out of SyncFileStatusTracker.

Similar steps were done in many propagation jobs. This also updates the db entry to always have the item.destination() as file path.

Since 'placeholder' just means that it's an item of the special type that the vfs plugin can deal with - no matter whether hydrated or dehydrated - all done items should become placeholders. Even directories. Now every file that passes through updateMetadata() will be converted to a placeholder if necessary.

This allows SyncFileStatusTracker to also know about these. After all its information is used to provide icons for them too.

This will be used in conjunction with vfs plugins that detect whether a file has a pending hydration/dehydration through independent means and communicate that to the discovery through local file type.

Possibly the behavior should actually change and the function should de-placeholder all items, not just dehydrated ones.

Needs to be disabled for tests in some cases.

- unspecified and inherited are different - move enum to header in common/ - access through Vfs instead of directly in Journal

Any folder with a (potentially deeply) contained error will have StatusWarning. StatusExcluded marks exclusions. The difference is useful to know for VFS.

There were cases where the "/" exception wasn't handled correctly and there'd be extra slashes in generated paths.

- SyncJournalDB functions now behind internalPinStates() to avoid accidental usage, when nearly everyone should go through Vfs. - Rename Vfs::getPinState() to Vfs::pinState()

Previously they would be discarded since the file's mtime or size hadn't changed.

Allows for better attribute preservation. Also add verifyFileUnchanged() call before dehydration to avoid data loss when discovery takes a while.

The block of code that propagated attributes etc from the previously existing file was placed *after* the block that renamed the previously existing file to a conflict name. That meant the propagation didn't work in the conflict case.

Because some plugins provide alternative ui.

That just complicated things. It's ok if Vfs is not a fully abstract interface class. The pinstate-in-db methods are instead provided directly on Vfs and VfsSuffix and VfsOff use them to implement pin states. The start() method is simply non-virtual and calls into startImpl() for the plugin-specific startup code.

In tests an un-started Vfs instance is sometimes passed to SyncEngine via SyncOptions.

Fixes a bug introduced while moving the attribute propagation before the conflict-renaming.

That change will be useful for the notifications. Previously the dehydrated files were reported as "newly downloaded", now they're reported as "updated".

Creating a new virtual file and replacing a file with a virtual one now have their own text in the protocol, not just "Downloaded". To do this, the SyncFileItem type is kept as ItemTypeVirtualFileDehydration for these actions. Added new code to ensure the type isn't written to the database. While looking at this, I've also added documentation on SyncFileItem's _file, _renameTarget, _originalFile and destination() because some of the semantics weren't clear.

For moves it's relevant that the old db entry is removed and the new one is created at the right location.

Also covering propagation to the downloaded file when a conflict-rename is done at the same time, which used to not work.

ckamm added the feature:vfs native virtual files and placeholder implementation label Feb 6, 2019

ckamm added this to the 2.6.0 milestone Feb 6, 2019

ckamm self-assigned this Feb 6, 2019

ckamm requested review from dschmidt and ogoffart February 6, 2019 11:14

ogoffart approved these changes Feb 8, 2019

View reviewed changes

ckamm added the DO NOT MERGE YET label Feb 9, 2019

ogoffart approved these changes Feb 11, 2019

View reviewed changes

ckamm added 22 commits February 11, 2019 13:33

Build fix for mingw64 7.3 (Qt 5.12.0)

ced060a

Move SyncFileStatus to libcommon

204ccde

It'll be needed in vfs plugins so they can connect to the data coming out of SyncFileStatusTracker.

Propagator: Helper for updating db

ae53082

Similar steps were done in many propagation jobs. This also updates the db entry to always have the item.destination() as file path.

Vfs: Send SyncFileStatusTracker data to vfs plugins

997f3f7

Discovery: Add signal for silentlyExcluded files

97b77f9

This allows SyncFileStatusTracker to also know about these. After all its information is used to provide icons for them too.

Discovery: Files can have dehydrate/download actions

ae2c9b3

This will be used in conjunction with vfs plugins that detect whether a file has a pending hydration/dehydration through independent means and communicate that to the discovery through local file type.

Vfs: Clarify SyncEngine::wipeVirtualFiles()

deda083

Possibly the behavior should actually change and the function should de-placeholder all items, not just dehydrated ones.

Vfs: Shell integration is enabled optionally

3ecef7f

Needs to be disabled for tests in some cases.

Pin state updates

35aeb17

- unspecified and inherited are different - move enum to header in common/ - access through Vfs instead of directly in Journal

SyncFileStatusTracker: Distinguish Warning and Excluded

ce95d42

Any folder with a (potentially deeply) contained error will have StatusWarning. StatusExcluded marks exclusions. The difference is useful to know for VFS.

Vfs: Enable propagating attributes on download

3cb4129

Vfs: Call unregisterFolder() when folder is removed

0f25732

Folder: Add remoteFolderTrailingSlash()

04bc36c

There were cases where the "/" exception wasn't handled correctly and there'd be extra slashes in generated paths.

PinStates cleanup

4900bf9

- SyncJournalDB functions now behind internalPinStates() to avoid accidental usage, when nearly everyone should go through Vfs. - Rename Vfs::getPinState() to Vfs::pinState()

File watcher: Pin state attribute changes are valid notifications

c67de7d

Previously they would be discarded since the file's mtime or size hadn't changed.

Vfs: dehydration is separate action

92bd69d

Allows for better attribute preservation. Also add verifyFileUnchanged() call before dehydration to avoid data loss when discovery takes a while.

PropagateDownload: Conflict-rename later

df8580a

The block of code that propagated attributes etc from the previously existing file was placed *after* the block that renamed the previously existing file to a conflict name. That meant the propagation didn't work in the conflict case.

Vfs: Add option to hide socketapi pin actions

7198bf4

Because some plugins provide alternative ui.

Vfs: Ensure VfsOff works without start() being called

152285c

In tests an un-started Vfs instance is sometimes passed to SyncEngine via SyncOptions.

Vfs suffix: Fix dehydration creating the wrong db entry

144da99

ckamm added 5 commits February 11, 2019 13:33

PropagateDownload: Create conflict even if local file changed

d838807

Fixes a bug introduced while moving the attribute propagation before the conflict-renaming.

Vfs: Make dehydration a SYNC and not a NEW action

f8857b7

That change will be useful for the notifications. Previously the dehydrated files were reported as "newly downloaded", now they're reported as "updated".

Tests: Add db checks to rename/move tests

3abe488

For moves it's relevant that the old db entry is removed and the new one is created at the right location.

Test: Add check for permission propagation

fd8dd79

Also covering propagation to the downloaded file when a conflict-rename is done at the same time, which used to not work.

ckamm force-pushed the vfs-winsyncprovider branch from 8b43d18 to fd8dd79 Compare February 11, 2019 12:34

ckamm removed the DO NOT MERGE YET label Feb 11, 2019

ckamm merged commit 5673879 into master Feb 11, 2019

delete-merged-branch bot deleted the vfs-winsyncprovider branch February 11, 2019 12:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vfs adjustments for deeper shell integration #7017

Vfs adjustments for deeper shell integration #7017

ckamm commented Feb 6, 2019

ogoffart Feb 8, 2019

ckamm Feb 9, 2019

ogoffart Feb 8, 2019

ckamm Feb 9, 2019

ogoffart Feb 8, 2019

ckamm Feb 9, 2019

ogoffart Feb 8, 2019

ogoffart Feb 8, 2019

ckamm Feb 9, 2019

ogoffart Feb 8, 2019

ckamm Feb 9, 2019

ogoffart Feb 8, 2019

ckamm Feb 11, 2019

Vfs adjustments for deeper shell integration #7017

Vfs adjustments for deeper shell integration #7017

Conversation

ckamm commented Feb 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment