Discovery: Add branding option to disable default sync of 'M' directo… #5340

ogoffart · 2016-11-29T09:39:41Z

…ries

Issue #5331 and https://github.com/owncloud/enterprise/issues/1594

mention-bot · 2016-11-29T09:39:43Z

@ogoffart, thanks for your PR! By analyzing the history of the files in this pull request, we identified @dragotin, @guruz and @cryptomilk to be potential reviewers.

ogoffart · 2016-11-29T09:40:03Z

(untested)

rperezb · 2016-11-29T11:31:00Z

@SamuAlfageme calling you! 📲

guruz · 2016-11-29T16:03:38Z

👎 doesn't work, debugging

guruz · 2016-12-02T14:21:24Z

@SamuAlfageme You can test this by having an external storage configured in your oC server (files_external). I tried with a system wide mount but you could try with a user mount.
It should then show up unsynced by default in selective sync

SamuAlfageme · 2016-12-12T10:12:41Z

@ogoffart @guruz let me get this straight: bool dontSyncMountedStorageByDefault() const { return true; } should not mount external storage by default? Cause if that's the case, trying with 3 different external storage opts. configured (Dropbox, Drive & Local), all of them are selected in the selective sync view by default.

SamuAlfageme · 2016-12-12T12:08:21Z

So far what I found:

Both account setup wizard options (sync everything and choose what to sync) are still selecting by default all the folders, including the ones from external storage.
When deleting .csync_journal.db, 3 actions are displayed and the folders unchecked, however none of those are erasing the contents from the local sync folder (plus the message shown is addressing size problems, instead of external-storage reasons):

michaelstingl · 2016-12-22T09:59:02Z

@SamuAlfageme What is the exact test procedure?

SamuAlfageme · 2016-12-22T10:04:41Z

@michaelstingl you need to modify the theme to set dontSyncMountedStorageByDefault() const { return true; } and build with the issue_5331 branch.

From there, I can provide you with a test server with 2/3 different external storage options configured and running. And you can see how the auto-deselect doesn't work neither on the create account wizard nor when deleting (to force restore) the .csync_journal.db on the root folder sync connection.

Second scenario unchecks the external storage folders but does not remove them from the local filesystem when selecting any of those 3 options.

ckamm · 2017-01-04T13:12:47Z

One of @SamuAlfageme's issues is that the initial folder setup doesn't check for 'M' directories. In a way this is similar to what I addressed in #5426. It'd be nice to share the "dubious folder" logic between discovery and initial setup so we don't have to worry about it in the future.

SamuAlfageme · 2017-01-10T15:56:49Z

🔝 Cherrypicking the changes in #5426 for testing purposes...

ckamm · 2017-01-11T12:56:34Z

@SamuAlfageme @ogoffart I've addressed the inconsistency between setup and later synchronization in ckamm@39d048a - shall I include that patch here?

SamuAlfageme · 2017-01-11T12:58:58Z

@ckamm sure! will re-test as soon as it's ready.

ckamm · 2017-01-11T13:00:56Z

@SamuAlfageme I've put it into this PR for now, but it's pending @ogoffart's opinion.

ogoffart · 2017-01-11T16:24:26Z

I added some fixup commit.

The remaining issues i could think of with this patch:

The rewording, as i wrote on IRC: the reason it was phrased like (with double negative) that is because it is a black list. So it is "unselected item will not be synced" rather than "selected items will be synced". The difference is weak, but it is to make sure people don't complain that they select "foo/bar", but not "foo", and yet, the file within foo are sync'ed.
I don't have a strong opinion. And if you think the new wording is easier to understand, let's just change it.
The 'M' detection will only work for top level directories. We don't check for sub directories. I guess that's fine because mount point are always top level, are they not?

SamuAlfageme · 2017-01-11T17:42:29Z

New tests after these changes are looking good so far: 🎉

There's also this one conceptual thing we need to address: #5421 (comment) because Sync everything + do not sync ext.sto. will result in syncing ext.sto.

cc/ @michaelstingl

SamuAlfageme · 2017-01-12T09:07:10Z

After including the message pointing the cause of folders being unchecked there should be the same for newly created storages. cc/ @ckamm

Answering to @ogoffart's #5340 (comment)

The 'M' detection will only work for top level directories. We don't check for sub directories. I guess that's fine because mount point are always top level, are they not?

Well, looks like yes, if you specify a path for the mounting point instead of a single, folder name; it's mounted that path (if available; I'm investigating a bug related with this). I don't know if this is the expected behavior though. cc/ @PVince81

Also, again, we have discrepancies between what happens on the wizards and if changes were made in an already created folder sync connection. In the second scenario, folder is unchecked:

In the first, as addressed in 7344571 it isn't. So we should have an uniform way of dealing with this.

@ogoffart agreed on having a different name for the option as current (double negation antipattern) is too confusing (and was source of my question on #5340 (comment))

ogoffart · 2017-01-13T10:06:02Z

@michaelstingl Should the branding option be only for new folder, or also for new sync?

i.e, in the first sync, if the user selected "Sync Everything" in the wizard, should the 'M' folder be sync'ed or not?

ckamm · 2017-01-20T13:16:10Z

@ogoffart How about we merge this to have it in 2.3 and create a new ticket to discuss the implications on the wizards? If merging this soon isn't urgent, we can also keep it like this.

ogoffart · 2017-01-26T09:25:06Z

@SamuAlfageme Thanks for testing! I have fixed most issues. I icreased the default size of the wizard so everything fits.

The checkboxes do not affect the contents of the Choose what to sync... filepicker

That is expected i'd say. (See comment #5340 (comment) )

Group [notifications] in one instead of displaying them in batch after clicking in Connect...

The API for notifications is "fire and forget". So we'd need to accumulate them and only show the notification when discovery is finished. We can do that. But that's a bigger change which i might do later.

some of the folder names are cropped after the line break.

I could not reproduce that. Maybe it's only on Mac?

SamuAlfageme · 2017-01-26T10:39:24Z

@ogoffart awesome, updated #5340 (comment) and will move some of the things that can be improved to individual issues to address them later so we can merge here. Couple of simple things before so,

The wizard window now looks huge:

The new icons introduced in ed15842 are not being used in the "Choose what to sync" view:

SamuAlfageme · 2017-01-26T13:07:05Z

@ogoffart and finally, extracted from #5340 (comment):

[If checkboxes apply to 'Choose what to sync' as well they should probably be placed below, not in between the both options]

Place the the confirmation options below the Sync everything/Choose what to sync radio buttons

ogoffart · 2017-01-26T14:58:08Z

@SamuAlfageme : Do you think the wizard is now too big?

Regarding the choose what to sync, i'll change their icon in a follow up pull request.
But the checkboxes do not apply to the choose what to sync.

guruz · 2017-01-26T15:26:44Z

src/libsync/discoveryphase.h

+struct SyncOptions {
+    /** Maximum size (in Bytes) a folder can have without asking for confirmation.
+     * -1 means infinite */
+    qint64 _newBigFolderSizeLimit = -1;


Just saying in future it would be nice if all variables with time or bytes are called something like ...msec or ..bytes etc. Not important now

guruz · 2017-01-26T15:48:58Z

src/gui/wizard/owncloudadvancedsetuppage.cpp

@@ -200,6 +215,11 @@ QStringList OwncloudAdvancedSetupPage::selectiveSyncBlacklist() const
    return _selectiveSyncBlacklist;
 }

+bool OwncloudAdvancedSetupPage::confirmBigFolder() const


Would be good to have such functions named "is..." or "are..." to see it is a getter

guruz · 2017-01-26T16:00:13Z

src/libsync/theme.cpp

@@ -275,6 +275,15 @@ qint64 Theme::newBigFolderSizeLimit() const
    return 500;
 }

+bool Theme::wizardHideExternalStorageConfirmationCheckbox() const


FYI @rperezb

@guruz thanks!

SamuAlfageme · 2017-01-26T16:20:32Z

@ogoffart

Do you think the wizard is now too big?

Kind of, yeah 😕. Took the whole screen on my Win7 VM, it's bigger than the application main window and there's a lot of unused space on the right.

Regarding the choose what to sync, i'll change their icon in a follow up pull request.

Great! 😉

But the checkboxes do not apply to the choose what to sync.

They do if they're set as global settings, so I would rather enable them for both options and place them under the radio buttons.

guruz · 2017-01-26T16:45:33Z

Code looks OK 👍
I have not tested the actual functionality.

wizard is also too big on my screen, please reduce and merge.

@SamuAlfageme Did you also test this with deeper directory structures? Will creating a new directory on the server side deep inside the mounted storage again ask for confirmation?

SamuAlfageme · 2017-01-26T17:00:22Z

@guruz

Did you also test this with deeper directory structures?

Yeah, looks good so far:

Will creating a new directory on the server side deep inside the mounted storage again ask for confirmation?

Tried making superficial changes on the external storage and it was ok, program logic could point otherwise?

The advanced page has become quite complex and does not fit on the screen anymore if the fonts are too big

We need to forward the information that the folder is an external storage for the notification message. Issue: #5340 (comment)

… added The Size limit, or confirmation checkboxes might have changed. We need to guard against saving if the control changes while we are loading Issue: #5340 (comment)

…ill want to confirm big folder Issue: #5340 (comment)

ogoffart · 2017-01-27T08:46:04Z

I resized the wizard again (750 pixels wide)

Added two checkboxes in the Account Wizard in the advanced page to change the first options. Also added a checkbox in the general settings to ask for confirmation for external storages. Theme options allow to hide the checkboxes in the wizard. As described in issue #5340

The sync engine rely on the 'M' in premission to ask for confirmation (As requested in issue #5340) But we only want to ask the premission for the 'root' of the mounting point and not for every subfolders within it. So we change the discovery phase in a way that it does not keep the 'M' for children within the external storage.

Issue: #5340 (comment)

We need to forward the information that the folder is an external storage for the notification message. Issue: #5340 (comment)

… added The Size limit, or confirmation checkboxes might have changed. We need to guard against saving if the control changes while we are loading Issue: #5340 (comment)

guruz · 2017-01-27T15:11:04Z

@ogoffart I accidently merged this to master instead of 2.3.
Do you want to merge to 2.3 manually with git?

guruz added this to the 2.3.0 milestone Nov 29, 2016

guruz added Enhancement blue-ticket labels Nov 29, 2016

guruz added a commit that referenced this pull request Nov 29, 2016

Fix up previous commit #5340

1fba273

guruz added the ReadyToTest QA, please validate the fix/enhancement label Nov 29, 2016

guruz assigned SamuAlfageme Nov 29, 2016

guruz mentioned this pull request Nov 30, 2016

[Placeholder] Implement branding option handling for M permission #5331

Closed

SamuAlfageme removed the ReadyToTest QA, please validate the fix/enhancement label Dec 13, 2016

SamuAlfageme mentioned this pull request Dec 27, 2016

"Ask confirmation before downloading folders larger than X MB" does not work when creating a folder sync connection #5421

Closed

ckamm force-pushed the issue_5331 branch from 39d048a to 99cfd4b Compare January 11, 2017 14:40

SamuAlfageme mentioned this pull request Jan 13, 2017

[OS X] Improve macdeployqt.py #5447

Closed

SamuAlfageme mentioned this pull request Jan 26, 2017

[Notifications] Group similar notifications received together instead of showing them in batch #5493

Open

guruz reviewed Jan 26, 2017

View reviewed changes

ogoffart added 4 commits January 27, 2017 09:43

Wizard: Increase the size

1b9c49e

The advanced page has become quite complex and does not fit on the screen anymore if the fonts are too big

Confirm External Storage: adjust the notification message

dc3acca

We need to forward the information that the folder is an external storage for the notification message. Issue: #5340 (comment)

General Settings: Make sure to reload the settings when an account is…

de53732

… added The Size limit, or confirmation checkboxes might have changed. We need to guard against saving if the control changes while we are loading Issue: #5340 (comment)

Account Wizard: don't whitelist everything on the first sync if we st…

7e63849

…ill want to confirm big folder Issue: #5340 (comment)

ogoffart force-pushed the issue_5331 branch from bf954da to 7e63849 Compare January 27, 2017 08:43

guruz merged commit 65e4afe into master Jan 27, 2017

guruz pushed a commit that referenced this pull request Jan 27, 2017

FolderStatusModel: Different icon for external storages

517623e

Issue: #5340 (comment)

guruz pushed a commit that referenced this pull request Jan 27, 2017

Confirm External Storage: adjust the notification message

38cf459

We need to forward the information that the folder is an external storage for the notification message. Issue: #5340 (comment)

guruz deleted the issue_5331 branch January 27, 2017 15:00

guruz restored the issue_5331 branch January 27, 2017 15:00

guruz deleted the issue_5331 branch July 31, 2017 18:35

SamuAlfageme mentioned this pull request Oct 30, 2017

Identify federated/external shares in the account/selective sync view #6132

Closed

SamuAlfageme mentioned this pull request Jan 10, 2018

[Feature Request] Propagate mount write/update permissions to the client owncloud/core#30070

Closed

SamuAlfageme mentioned this pull request May 17, 2018

Large remote shares get synced by default when its size is "Pending" #6536

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Discovery: Add branding option to disable default sync of 'M' directo… #5340

Discovery: Add branding option to disable default sync of 'M' directo… #5340

ogoffart commented Nov 29, 2016

mention-bot commented Nov 29, 2016

ogoffart commented Nov 29, 2016

rperezb commented Nov 29, 2016

guruz commented Nov 29, 2016

guruz commented Dec 2, 2016

SamuAlfageme commented Dec 12, 2016

SamuAlfageme commented Dec 12, 2016 •

edited

Loading

michaelstingl commented Dec 22, 2016

SamuAlfageme commented Dec 22, 2016

ckamm commented Jan 4, 2017

SamuAlfageme commented Jan 10, 2017 •

edited

Loading

ckamm commented Jan 11, 2017

SamuAlfageme commented Jan 11, 2017

ckamm commented Jan 11, 2017

ogoffart commented Jan 11, 2017

SamuAlfageme commented Jan 11, 2017 •

edited

Loading

SamuAlfageme commented Jan 12, 2017

ogoffart commented Jan 13, 2017

ckamm commented Jan 20, 2017

ogoffart commented Jan 26, 2017

SamuAlfageme commented Jan 26, 2017 •

edited

Loading

SamuAlfageme commented Jan 26, 2017

ogoffart commented Jan 26, 2017

guruz Jan 26, 2017

guruz Jan 26, 2017

guruz Jan 26, 2017

rperezb Jan 27, 2017

SamuAlfageme commented Jan 26, 2017

guruz commented Jan 26, 2017

SamuAlfageme commented Jan 26, 2017

ogoffart commented Jan 27, 2017

guruz commented Jan 27, 2017

Discovery: Add branding option to disable default sync of 'M' directo… #5340

Discovery: Add branding option to disable default sync of 'M' directo… #5340

Conversation

ogoffart commented Nov 29, 2016

mention-bot commented Nov 29, 2016

ogoffart commented Nov 29, 2016

rperezb commented Nov 29, 2016

guruz commented Nov 29, 2016

guruz commented Dec 2, 2016

SamuAlfageme commented Dec 12, 2016

SamuAlfageme commented Dec 12, 2016 • edited Loading

michaelstingl commented Dec 22, 2016

SamuAlfageme commented Dec 22, 2016

ckamm commented Jan 4, 2017

SamuAlfageme commented Jan 10, 2017 • edited Loading

ckamm commented Jan 11, 2017

SamuAlfageme commented Jan 11, 2017

ckamm commented Jan 11, 2017

ogoffart commented Jan 11, 2017

SamuAlfageme commented Jan 11, 2017 • edited Loading

SamuAlfageme commented Jan 12, 2017

ogoffart commented Jan 13, 2017

ckamm commented Jan 20, 2017

ogoffart commented Jan 26, 2017

SamuAlfageme commented Jan 26, 2017 • edited Loading

SamuAlfageme commented Jan 26, 2017

ogoffart commented Jan 26, 2017

guruz Jan 26, 2017

Choose a reason for hiding this comment

guruz Jan 26, 2017

Choose a reason for hiding this comment

guruz Jan 26, 2017

Choose a reason for hiding this comment

rperezb Jan 27, 2017

Choose a reason for hiding this comment

SamuAlfageme commented Jan 26, 2017

guruz commented Jan 26, 2017

SamuAlfageme commented Jan 26, 2017

ogoffart commented Jan 27, 2017

guruz commented Jan 27, 2017

SamuAlfageme commented Dec 12, 2016 •

edited

Loading

SamuAlfageme commented Jan 10, 2017 •

edited

Loading

SamuAlfageme commented Jan 11, 2017 •

edited

Loading

SamuAlfageme commented Jan 26, 2017 •

edited

Loading