Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vfs <=> selective sync switching #6954

Merged
merged 7 commits into from
Jan 18, 2019
Merged

Conversation

ckamm
Copy link
Contributor

@ckamm ckamm commented Jan 8, 2019

There is now a local/online availability switch on folder context menus in the settings as well as a menu entry to switch into vfs mode and away from it.

I looked at migrating between the two analogous settings (selective sync excluded <-> vfs online-only) but it's complex and will probably not produce exactly what users want. So instead I just start with blank vfs and selective sync settings after switching.

Details: When switching on vfs it seems natual to make all excluded paths online-only, the rest available-locally. But then switching on vfs has no visible effect for users that didn't use selective sync unless they manually also switch folders to online-only.

Migrating from vfs to selective sync is just hard since vfs can have arbitrary switches back and forth between online/local folders. Also, online-only folders can totally have locally available files - so setting up selective sync options automatically from that is likely not worth it.

@michaelstingl FYI

@ckamm ckamm added the feature:vfs native virtual files and placeholder implementation label Jan 8, 2019
@ckamm ckamm added this to the 2.6.0 milestone Jan 8, 2019
@ckamm ckamm self-assigned this Jan 8, 2019
@ckamm ckamm requested a review from ogoffart January 8, 2019 15:00
@ogoffart
Copy link
Contributor

ogoffart commented Jan 8, 2019

optional: data on heap, unions don't like non-trival dtors

Why?
Normally this should work if we call the destructor manyally, which Error does.


// Wipe pin states to be sure
folder->journalDb()->wipePinStateForPathAndBelow("");
folder->journalDb()->setPinStateForPath("", PinState::OnlineOnly);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we not set the oldBlackList as Online Only ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By setting the root as OnlineOnly everything will be online-only - there's no attempt to migrate selective sync settings. If we had the root as AlwaysLocal, users who switch to vfs but have no selective sync configured would see no difference. See PR text above.

QMessageBox::Question,
tr("Disable virtual file support?"),
tr("This action will disable virtual file support. As a consequence contents of folders that "
"are currently marked as 'available online only' will either be downloaded or excluded "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe they will all be downloaded

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, old text from when I migrated.

It has a destructor and these operations make sense. Particularly the
move is important for code like:

Result<x, y> foo() { Result<x, y> v; return v; }

because the move-ctor will not autogenerate if x or y are not trivially
destructible.
This allows enabling and disabling vfs.

To distinguish this operation from setting the root pin state, the
availability setting is adjusted as well to be similar to the
menu that shows in the shell extensions.
@ckamm
Copy link
Contributor Author

ckamm commented Jan 10, 2019

@ogoffart Please rereview

@ogoffart ogoffart merged commit ad81454 into master Jan 18, 2019
@delete-merged-branch delete-merged-branch bot deleted the vfs-selective-sync-migration branch January 18, 2019 09:59
@ogoffart
Copy link
Contributor

I've merged this since other pr depends on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature:vfs native virtual files and placeholder implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants