Added blacklisting of files for uploads

[newhinton]

trafficstars

Closes #3418

[newhinton]

This is a basic implementation for a filter mechanism which prevents the (auto)upload of a given file if the name matches a pattern. Currently there are following problems:

-Where should this get it's place in the ui? We could do the same as the desktop-client does. -Where should i store the list of excluded patterns? -I am using a snippet from stackoverflow to convert a glob style pattern to a regex one. I provided a link to the original in a comment over the snippet. Is that okay to do, license-wise? -Which RemoteOperationResult should i use? Currently im using unautorized, but that results in a wrong notification, but a blacklisted file should not create a notification at all

I'd appreciate any help :)

[codecov[bot]]

Codecov Report

Merging #3500 into master will increase coverage by 0.04%. The diff coverage is 60.86%.

@@             Coverage Diff             @@
##             master   #3500      +/-   ##
===========================================
+ Coverage      6.12%   6.16%   +0.04%     
  Complexity        1       1              
===========================================
  Files           316     316              
  Lines         30626   30649      +23     
  Branches       4384    4389       +5     
===========================================
+ Hits           1876    1890      +14     
- Misses        28468   28473       +5     
- Partials        282     286       +4

Impacted Files	Coverage Δ	Complexity Δ
...ncloud/android/operations/UploadFileOperation.java	25.54% <60.86%> (+1.3%)	0 <0> (ø)	:arrow_down:
.../third_parties/daveKoeller/AlphanumComparator.java	82.14% <0%> (ø)	0% <0%> (ø)	:arrow_down:

[codecov[bot]]

Codecov Report

Merging #3500 into master will increase coverage by 0.05%. The diff coverage is 19.41%.

@@             Coverage Diff              @@
##             master    #3500      +/-   ##
============================================
+ Coverage     17.65%   17.71%   +0.05%     
  Complexity        3        3              
============================================
  Files           392      394       +2     
  Lines         33134    33223      +89     
  Branches       4655     4662       +7     
============================================
+ Hits           5849     5884      +35     
- Misses        26330    26379      +49     
- Partials        955      960       +5

Impacted Files	Coverage Δ	Complexity Δ
...m/nextcloud/client/preferences/AppPreferences.java	0% <ø> (ø)	0 <0> (ø)	:arrow_down:
...java/com/nextcloud/client/di/ComponentsModule.java	0% <ø> (ø)	0 <0> (ø)	:arrow_down:
...owncloud/android/ui/activity/SettingsActivity.java	39.34% <0%> (-0.23%)	0 <0> (ø)
...oud/android/ui/activity/SyncedFoldersActivity.java	25.49% <0%> (-0.32%)	0 <0> (ø)
...alog/PatternBlacklistEditorDialogListFragment.java	0% <0%> (ø)	0 <0> (?)
...i/dialog/PatternBlacklistEditorDialogFragment.java	0% <0%> (ø)	0 <0> (?)
...xtcloud/client/preferences/AppPreferencesImpl.java	53.64% <57.14%> (+2.83%)	0 <0> (ø)	:arrow_down:
...ncloud/android/operations/UploadFileOperation.java	26.54% <64%> (+1.66%)	0 <0> (ø)	:arrow_down:
...in/java/com/owncloud/android/datamodel/OCFile.java	65.92% <0%> (-1.77%)	0% <0%> (ø)
...owncloud/android/ui/adapter/OCFileListAdapter.java	28.28% <0%> (-0.37%)	0% <0%> (ø)
... and 15 more

[newhinton]

I have added a new DialogFragment, which contains a list with all the patterns.

I still dont know on how to persist the list, any suggestions are appreciated.

[AndyScherzinger]

Hi @newhinton :) I'll try to mention the afaik right people to answer the questions:

-Where should this get it's place in the ui? We could do the same as the desktop-client does.

likely a general question for @nextcloud/designers and @tobiasKaminsky if/how exclude patterns for the auto upload should be added to the UI. I am not sure if they should be added (I understand the desktop client has such a feature) or if we should rather have a fixed list of patterns (like implemented within this PR, to not upload .thumbdata files). So any feedback is appreciated and if added, also if this is a glbal setting (for all auto upload folders, just custom auto uploads (I guess so), per account?, 1 setting for any *)

-Where should i store the list of excluded patterns?

Highly depends on the discussion and decision on the first question in my opinion. @tobiasKaminsky

-I am using a snippet from stackoverflow to convert a glob style pattern to a regex one. I provided a link to the original in a comment over the snippet. Is that okay to do, license-wise?

@schiessle can you answer this? :)

-Which RemoteOperationResult should i use? Currently im using unautorized, but that results in a wrong notification, but a blacklisted file should not create a notification at all

@tobiasKaminsky can you answer this? :)

[newhinton]

the thing with fixed lists are, well, they are fixed ^^ Imho we should keep this as close as possible to the desktop-client, so that a user should have no trouble at all to understand how those patterns work. Also the user cannot remove excluded pattern if he needs to, which can be a problem if he needs excluded files

[tobiasKaminsky]

My opinion

• we try to have as few options as possible, but instead work with sane defaults
• an exclude list should be globally for any auto-upload folder, otherwise this will be totally confusing
• configurable list seems to be very low demanded; I cannot recall any other request like this on gplay comments, nor somewhere else
• existing issues point to .thumbnail file only

So my idea is to first have a simple hardcoded blacklisted pattern list to exclude.

[tobiasKaminsky]

After deciding how this feature shall look like, we can discuss implementation detail (like licence, ResultCode, etc.)

[newhinton]

I understand and agree withyour point regarding adding as few settings as possible, but i think excluding files without the ability to disable this behaviour may also be confusing/really bad. I tried to keep this function as close as possible to the desktop-client, so users dont have to deal with multiple different-working sync-features

So, when we work with a hardcoded list, we need to give the user the ability to find out what is blacklisted, and an ability to disable blacklisting completely

[newhinton]

Also a good reason to allow customizing: custom auto uploads can be from a gamefolder or a folder which creates a lot of tempfiles or so on, where users could want to prevent uploading of those files

Also, while the ui-discussion is important, i think it does not prevent us from deciding on how to do the rest, especially the stackoverflow stuff :D if i need to reimplement this or find a different solution, i can do it while we are discussing the ui ;)

[newhinton]

@nextcloud/designers What do you think about this?

Besides the obvious shortfalls of my current design (wrong buttons, listlayout broken, etc) i think this would serve just right and behaves like the desktop version

[newhinton]

@AndyScherzinger @tobiasKaminsky Also i have a different problem: my build wont compile anymore. Master works fine, but this branch wont compile because it cannot find ProgressiveDataTransferer. (The same on my old repo, and a fresh copy from git) Basically com.owncloud.android.lib.* does not resolve anymore except on the master branch. Any ideas why that is? Or how i can solve this?

[tobiasKaminsky]

try to rebase your branch on latest master, there were some name changes…

[AndyScherzinger]

@newhinton I did a rebase for you ❗️ Should now work fine again.

[newhinton]

@AndyScherzinger That did it, thanks!

[AndyScherzinger]

@newhinton you are very welcome ❤️

[Siggi0904]

when'll be these branch merged into master?

I need these very nice feature. :-)

[newhinton]

@Siggi0904 This is currently in development. While i have figured out the basics, putting it together takes some additional time, and after that we may have to discuss changes. I hope this wont take too long, but this is by no means ready to release ;)

[Siggi0904]

ok, if you need a tester, write me. :-)

[newhinton]

@AndyScherzinger @tobiasKaminsky This basically works now. Except for custom uploads, which do not seem to work at all. (They don't trigger. I curently have set up the Downloads-Folder as a custom one, and an auto-detected folder, but only the auto-detected works. @Siggi0904 Can you confirm this behaviour with this branch?)

Besides that: there are a few minor ui-issues: -The fragment is to small in size for my taste, i'd like to have it the same size as the "set up custom upload" fragment -The 'save'&'close' buttons are the wrong (default-blue) color -The 'add' button is currently a black +. Is there a better icon i should use?

That, and the open questions regarding licensing ,sane defaults and the "error" message need to be discussed.

[jancborchardt]

Only read the beginning of the discussion, here’s my design input – basically I agree with @tobiasKaminsky and especially @AndyScherzinger’s comment here:

I am not sure if they should be added (I understand the desktop client has such a feature) or if we should rather have a fixed list of patterns (like implemented within this PR, to not upload .thumbdata files).

These patterns to exclude thumbnail files and temp folders are just common sense, we don’t need anything in the UI for that. The desktop client is not a good comparison as it is a "we always had it and people will complain" issue rather than a specific design decision we ever made.

[newhinton]

@jancborchardt Basically, i would agree. I am somewhat sure that i wouldn't use that feature to add many patterns. But there is a problem: What are sensible defaults? Do you know every filename? (They may be different between androidforks) What if the thumbnails are timestamped? What if an app adds a new tempfile which we do not cover?

We could find many filenames which would be nessessary to exclude, but adding/removing one would need an app update. Also what if a user wants to keep a temp file and sync it?

I am not sure how we would cover such cases, but i don't think we should ignore them either

[newhinton]

Also, how would a user know that some files are excluded? If there is no userinterface at all, we hide this feature from the user. He would have no way to find it, and he would simply wonder why the heck his custom sync does not work properly because not all files are synced and some are missing. Which basically means that the sync which should sync everything does not work, which reduces the confidence in this app by users who don't know that filtering exists.

If you have ideas regarding this, tell me, i want to know :D But currently i don't see any way how to make this clear, without a list telling the user what is beeing excluded, and if we have this list, why not make it editable in the first place? ;)

(oh, one way to solve those problems is by not including filtering at all ^^ it would be sad, but it would solve the problems ;) )

[jancborchardt]

Also, how would a user know that some files are excluded? If there is no userinterface at all, we hide this feature from the user. He would have no way to find it, and he would simply wonder why the heck his custom sync does not work properly because not all files are synced and some are missing.

Even if we have an interface for it, regular people should not need to look through that. So we need sane defaults anyway. :)

What sane defaults actually are, I’ll leave up to the Android experts. On desktop there are certain filetypes and folders which do not have user data in them, like .DS_Store and such. Image thumbnails also belong in this category – there would be no expectation of sync since they do not show up anywhere.

What are sensible defaults? Do you know every filename? (They may be different between androidforks) What if the thumbnails are timestamped? What if an app adds a new tempfile which we do not cover?

As usual, the perfect is the enemy of the good. ;) People don’t want to manage exclude rules. They just want their stuff safe. So we simply start with a set of rules and can always expand on them. If there’s timestamps, there’s checks to ignore files starting with a pattern etc.

[newhinton]

Yeah, don't get me wrong, i am not arguing against sane defaults, i have the same opinion like you regarding that ;) sane defaults are good and important

What i think is problematic is not providing an ui for users who want/nees the customizability and to inform them about the filtering

[jancborchardt]

What i think is problematic is not providing an ui for users who want/nees the customizability and to inform them about the filtering

I do see your point, but the issue is that everything has to be built and maintained. :) Especially these specific settings only accounting for a tiny fraction of users (if at all – many times people only think they need these things) are always very involved.

There are plenty of complicated open source sync apps which are customizable to the last level. Nextcloud aims to be an alternative for people who are not routinely fiddling with settings. And with the limited resources we have, we need to focus on what brings value to the majority of people using Nextcloud.

[newhinton]

What i mean is: we sync everything, except e.g. *.temp because it is a temp file.

User is using a custom (or even autoupload) to sync his images from SuperPhotoApp. Then we update nextcloud, and suddently, .temp files aren't synced anymore, but user wants/needs those files for whatever reason. He does not know that we are filtering, so he thinks nextcloud is broken (which would be the best case here)

We need to at least inform the user about the filtering, and preferably what is beeing filtered. We cannot simply not sync some files and not tell the user

[jancborchardt]

Right, but let’s start with not hypotheticals but actual cases. :)

You opened the original issue at https://github.com/nextcloud/android/issues/3418, saying:

Filter Uploads to exclude files, quiet like the desktop app. That would enable the user to exclude files like the .thumbdataXX files android creates for dcim-folders.

So .thumbdata* would then be the first exclude rule as it’s a specific case you brought up.

.temp files aren't synced anymore, but user wants/needs those files for whatever reason.

This is speculation, and we can’t design properly on that. :) Like: .temp files are temporary, so why would they need to be synced? Or we could ask the person why they need the files. If that user ever happens to exist.

But we should not take imaginary scenarios as a justification to introduce a management interface for what should just work in the background. (As said – the desktop app is not a good benchmark for this.)

[newhinton]

tbh, if i get my tempfiles included in this pr, im fine with not including any explanation, but i don't think that is the way we should go :D

[newhinton]

its not quiet about the ability to manage it, its about filtering at all? shouldnt the user know that we are filtering at all?

something like "hey, some files were excluded in the upload because they are tempfiles"