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

Adds option to exclude file/folder patterns #224

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

kajgies
Copy link

@kajgies kajgies commented Feb 8, 2024

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

@newhinton
Copy link
Owner

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
Copy link
Owner

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?

@kajgies
Copy link
Author

kajgies commented Feb 10, 2024

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.

@newhinton
Copy link
Owner

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
Copy link
Owner

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

@kajgies
Copy link
Author

kajgies commented Feb 10, 2024

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.

@newhinton
Copy link
Owner

newhinton commented Feb 10, 2024

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

@kajgies
Copy link
Author

kajgies commented Feb 15, 2024

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.

super.onCreate(savedInstanceState)
ActivityHelper.applyTheme(this)
setContentView(R.layout.activity_filter)
val toolbar = findViewById<Toolbar>(R.id.toolbar)
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe a "close" icon in the toolbar would be better instead of the "back"-one that is default. In the future i might change views like this to a Full Screen Dialog

app/src/main/res/layout/activity_filter.xml Outdated Show resolved Hide resolved

</androidx.recyclerview.widget.RecyclerView>

<Button
Copy link
Owner

Choose a reason for hiding this comment

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

Thus button should be on the right, and it needs a bit more padding from the list above it

app/src/main/res/layout/content_task.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/content_task.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/content_task.xml Outdated Show resolved Hide resolved
android:inputType="text"
android:text="Name" />

<ImageButton
Copy link
Owner

Choose a reason for hiding this comment

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

I think a simple trash-button would be more appropriate for a single-menu item

Copy link
Owner

Choose a reason for hiding this comment

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

See above, trash-button

@newhinton
Copy link
Owner

@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 newhinton added this to the 2.5 Milestone milestone Mar 16, 2024
@kajgies
Copy link
Author

kajgies commented May 1, 2024

Hey @newhinton ,

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

Kaj Giesbers added 5 commits May 4, 2024 15:06
Filterentries can be created now within filters
Now supports both include and exclude
Removes tertiary cards on filter entry list
Also fixes a bug where triggers would not be linked to a task anymore after importing if you ever deleted a task with a lower ID
@kajgies
Copy link
Author

kajgies commented May 4, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants