Round-Sync icon indicating copy to clipboard operation
Round-Sync copied to clipboard

Adds option to exclude file/folder patterns

Open kajgies opened this issue 1 year ago • 14 comments

Adds option to exclude certain file patterns. UI inspired by RClone Browser. Could fix at least part of this issue 79 and provide a workaround for issue 126

Reason for this request

I use this app to backup all files on my phone, since the Android folder is protected, I am getting errors and this prevents files from being deleted on the remote.

I read the discussion on issue 79 and understand you would rather have some file/folder multi-select option instead for a better user experience. Since that issue is already almost a year ago, I hope you can consider this in the mean time.

Please let me know if I can improve the in-app explanation text or the code.

Screenshot

Screenshot

kajgies avatar Feb 08 '24 17:02 kajgies

On a first glance, this looks promising.

My intuition says that i would like to use a different table with each row only containing the value, possibly the options for that filter. And the task contains a list of the id's for the filters. My thinking is that this could allow later reuse of filters, global filters, etc.

Would this allow us to silmultaneous implement include/exclude/filter?

Regardless of above ideas, i would prefer to not use a textfield, but a recyclerview with each filter it's own row, and an add-button. This should be way more nice to use, i feel textfields are often a bit clunky to use.

newhinton avatar Feb 08 '24 19:02 newhinton

Okay, i made a decision. I am okay with the current database where you add a field. But i really would like to see a non-textfield-based ui for adding filters. It just is the better UX, and it doesn't require changes to the backend.

Though i was reading this page: Rclone Filtering Maybe it would be a good idea to extend your PR now to include a field that determines whether or not your list is exclude, include or filter, so that we can update the ui to support all of them without making any changes to the database.

What do you think, could you extend your PR to accomodate this?

newhinton avatar Feb 09 '24 14:02 newhinton

I will look into it. I'll try making the filters a separate database because I agree it will have value to reuse them. Instead of --exclude we can use --filter to support both ways of filtering. So you would have a Filter consisting of multiple "FilterEntry"s in a RecyclerView. Every FilterEntry consists of a filter and a dropdown to select whether it's an include of an exclude.

I'll add some screenshots soon to explain it better.

kajgies avatar Feb 10 '24 11:02 kajgies

Every FilterEntry consists of a filter and a dropdown to select whether it's an include of an exclude.

I dont think that is nessessary, or a good idea. As the rclone docs state, it is usually not a good idea to combine includes/excludes, and by only allowing a task-specific option to select wheter or not its an include or exclude, we prevent errors there. It technically limits what you can do with this app, but i think its worthwile to keep the UI not too complicated.

It will also limit the complexity of the filter-database. Instead of using multiple rows for a specific filter, we can use a single row with a singe field for the filter, as your current proposal does.

newhinton avatar Feb 10 '24 13:02 newhinton

I am uncertain about the choice of a seperate database for the filters. On one hand, that technically allows more flexibility, but it increases the complexity quite a bit. I currently dont see me using that flexibility, so i think it would be better to keep it contained to the task-database for now. This way we dont need to introduce "massive" changes to the database, that also need to reflect in the model, the import/export and the ui for only theoretical gains.

It should also make your work easier. Though if you implement something different, i will certainly review it

newhinton avatar Feb 10 '24 13:02 newhinton

I have been looking at the documentation for rclone filters. It is a bit confusing but they only advice to not combine the include, exclude and filter flags. The --filter flag actually has support for both includes and excludes. If there is an --include or --include-from flag specified, rclone implies a - ** rule which it adds to the bottom of the internal rule list. Specifying a + rule with a --filter... flag does not imply that rule.

That is why I suggested allowing include & exclude per filter, it will allow users to set all options. Everything is applied using the --filter flag, the exclude/include flags are then unnecessary. To prevent needing multiple rows per filter, I just serialize the list of filterentries into a string. So a Filter consists of (id, title, serializedFilterEntries)

I did end up making a separate table for the filters, partially because I was already halfway on it when I read your reply, and partially because I think there are use cases for it, such as being able to apply a /.thumbnails/ rule to multiple tasks. It could also be used for global filters as you suggested.

If you like these changes I could also look into fixing the export/import functionality to also save the filters.

Here are some screenshots of what I ended up with:

screenshot1

There is a spinner that allows you to select a filter. The first option in the spinner is "No Filter".

These are the options you get in the menu next to the filter:

screenshot2

Edit and Delete options are hidden when you have no filter selected. Then when you click "Edit" or "Add a Filter" you get the following screen: screenshot3

The option menu next to the filter entry just has an option to delete it.

Let me know what you think.

kajgies avatar Feb 10 '24 21:02 kajgies

If you like these changes I could also look into fixing the export/import functionality to also save the filters.

Yes please, that would be a hard requirement. Though, from what i can see you only need to make the FilterEntry serializable, as you did for the Filter itself already, and add that to the Importer/Exporter. Most of the work is done by the serializer, so that is nearly finished already.

From your screenshots: It already looks good. There are a few minor things, like the card-on-card* in the filter ui, and a bit of padding here and there, but i will mark them in an in-depth review of the code.

*We either need to add the tertiary-card here or use a different element. Maybe we can take something from here: https://m3.material.io/components/lists/specs

newhinton avatar Feb 10 '24 21:02 newhinton

Made some progress, I think it's almost finished.

  • I removed the tertiary cards, a list might not be necessary, it looks good to me as is
  • Added support for importing/exporting
  • I also fixed a bug with importing. The current importing system doesn't import the object's id. This causes issues with relationships in other tasks. For example, if you create 3 tasks, then delete the second one, they will have ID 1 & 3 in the database, if you now link a Trigger to the task with ID 3, then export config, then import config, the trigger will still be linked to ID 3, but the tasks will now actually be imported with ID's 1 & 2. I fixed this here
  • Also fixed a small issue with the create filter button not turning into a spinner immediately after creating your first filter

There was no need to serialize the FilterEntry's because they are already serialized as a string in the class/table. Only the getter/setter actually converts it between a serialized string and a list of FilterEntry's. This is done for ease of use without over complicating the database setup.

Please let me know if there's anything else to fix.

kajgies avatar Feb 15 '24 17:02 kajgies

@kajgies I think i would like to have the FilterEntry in a seperate table instead of as a string in the filter itself. For one, this allows us to get rid of the string operations in the getFilters-method, but the main reason is that with android room, it will be nearly trivial to manage those objects. (I have not yet switched to android room, but i think that is next on my todo.)

Are you okay with me changing that?

Nice catch with the importer!

If you want, i can make the changes myself, including the filter-entry database. In that case, we can just merge this now, and i'll do the rest one by one.

newhinton avatar Mar 16 '24 14:03 newhinton

Hey @newhinton ,

Thanks for your response. Please feel free to merge and make the changes you feel are necessary.

kajgies avatar May 01 '24 16:05 kajgies

Rebased it onto master to get the latest version, fix conflicts and make the future merge easier.

kajgies avatar May 04 '24 13:05 kajgies

Hello, any news on this? thanks

Odeen avatar Jun 20 '24 08:06 Odeen

Hi @newhinton, any update on this? I have been using my test build with these changes in it for months and it had been very stable to me so I think it can be merged. What do you think?

kajgies avatar Sep 09 '24 17:09 kajgies

Sorry, i was/am very busy. I will look at it this weekend!

newhinton avatar Sep 09 '24 17:09 newhinton