Add support for category renaming

[Dioxo]

📢 Type of change

• Bugfix
• New feature
• Enhancement
• Refactoring
• Dependency updates

📜 Description

Continuing with my last PR, now we can edit the category name when selecting desired folders and then edit. It will ask for a new category name for each folder.

💚 How did you test it?

I tried to rename different categories and it works... :)

📝 Checklist

• I formatted the code with the IDE's reformat action (Ctrl + Shift + L/Cmd + Shift + L)
• I reviewed submitted code
• I added a CHANGELOG entry if applicable

[msfjarvis]

Didn't get notified for this for some reason. I'm personally not a fan of prompting a new category for each password, or the stack-based code there. I refactored it away in #855 to address #853. Would you mind reworking this to match the new behaviour from #855?

[msfjarvis]

790e5e4 is essentially how you want the final code to behave.

[Dioxo]

@msfjarvis I'll change it to match with #855 once it's merged , i would like to ask you how can I edit the categories if the user selects more than 1 at the time and without a prompt for each category to change, that's why I prompt for a new category in each folder the user wants to change and had selected.

[msfjarvis]

@msfjarvis I'll change it to match with #855 once it's merged , i would like to ask you how can I edit the categories if the user selects more than 1 at the time and without a prompt for each category to change, that's why I prompt for a new category in each folder the user wants to change and had selected.

I understand the situation you're proposing but the situation I'm looking at makes prompting for each password rather problematic.

Imagine you have a main category called 'stuff' that contains subfolders for 50 or 100 websites. Now consider that you decide to rename 'stuff' to 'personal'. If I was to be prompted for each file in this situation, I'd be inputting into a 100 dialog boxes when all I wanted to do was rename a single folder.

In this situation, simply asking once would work better. For changing the category of multiple files or folders, it will simply suffice to create the destination category if it doesn't already exist, then move those files or folders inside the destination directory.

[Dioxo]

I think we are talking about different situations... I'm talking about the folder, not the files it contains, how it is structured right now is that if you want to rename "stuff" to "personal", it will be only 1 prompt. It doesn't matter if there are 100 files inside.

Now, imagine you have 3 folders, "stuff", "games" and "other", the situation that I'm currently telling you is, what happens if you want to rename the 3 folders because user selected the 3 at the same time? Right now it ask for each folder (ask for the new category for "stuff", then "games" and so on...).

[msfjarvis]

I think we are talking about different situations... I'm talking about the folder, not the files it contains, how it is structured right now is that if you want to rename "stuff" to "personal", it will be only 1 prompt. It doesn't matter if there are 100 files inside.

Now, imagine you have 3 folders, "stuff", "games" and "other", the situation that I'm currently telling you is, what happens if you want to rename the 3 folders because user selected the 3 at the same time? Right now it ask for each folder (ask for the new category for "stuff", then "games" and so on...).

Ah, I see now. That's what I get for rushing things :( Guess this is fine to go as-is then. I'll review this now and merge after testing tomorrow.

[Dioxo]

Yeah, inside renameCategory you can see that I only take into consideration the folders to change the name. Would it be also right to change the password file's name in the same function? To not discriminate only folders in this feature... Or should I leave it to only folders?

Because currently if you select "stuff" as a folder and "test" as Password, it will edit only the name for the folder.

[msfjarvis]

Yeah, inside renameCategory you can see that I only take into consideration the folders to change the name. Would it be also right to change the password file's name in the same function? To not discriminate only folders in this feature... Or should I leave it to only folders?

Because currently if you select "stuff" as a folder and "test" as Password, it will edit only the name for the folder.

That's fine, once my comments are addressed then you won't be able to edit if a password is selected.

[msfjarvis]

@Dioxo I've addressed my comments and fixed the merge conflict, there's still a bug where the dialog accepts empty category names that I've left for you to resolve.

[Dioxo]

I'll see that later :)

[msfjarvis]

I'll see that later :)

Absolutely. If you need extra time I'm more than happy to move this PR to the 1.10.0 milestone for July's release, since 1.9.0 is scheduled for June 22nd.

[Dioxo]

I think I can work on this tomorrow, but if you don't see any response from me then we can move this to the 1.10 milestone.

[Dioxo]

Well, i had some free time, I wanted to ask you @msfjarvis , when user writes a category name with "/" for exemple "games" => "foo/games", right now APS moves the category "games" inside "foo", I don't see this as a good behavior because maybe the destination folder doesn't exist (sauf if we want to confirm that the path exist to put the category inside... but even so, there's the option to manually move the category) but i can't find a way to escape newCategoryEditText.text , do you know how i can do this?

[msfjarvis]

Well, i had some free time, I wanted to ask you @msfjarvis , when user writes a category name with "/" for exemple "games" => "foo/games", right now APS moves the category "games" inside "foo", I don't see this as a good behavior because maybe the destination folder doesn't exist (sauf if we want to confirm that the path exist to put the category inside... but even so, there's the option to manually move the category) but i can't find a way to escape newCategoryEditText.text , do you know how i can do this?

Not sure what you mean by 'escape newCategoryEditText.text', can you elaborate on that? About moving to non-existent parent folders, I'm fine with us creating the path before moving.

[Dioxo]

Not sure what you mean by 'escape newCategoryEditText.text', can you elaborate on that?

Well, it was a stupid question, don't pay attention to it...
I think now it's good the PR. tell me what you think :)

[msfjarvis]

LGTM sans minor nits, will test it tomorrow and wait for Fabian to have a look as well.

[fmeum]

@Dioxo I added proper Autofill match updates by reusing (and renaming) the existing movePasswords function. I tested this a bit and found it to work well, but please do another round of testing yourself.

The stacking of dialogs works fine, but I noticed two effects that may warrant attention:

Cancel only cancels the renaming for a single file since the other dialogs are already shown. Should we relabel it as Skip?

All but the first dialogs do not have their text field focused when they become visible since there onShow event has already triggered by then. There is no proper fix for this short of using suspendCoroutine, but it's also not a blocker.

[Dioxo]

Cancel only cancels the renaming for a single file since the other dialogs are already shown. Should we relabel it as Skip?

I'll change it.

All but the first dialogs do not have their text field focused when they become visible since there onShow event has already triggered by then. There is no proper fix for this short of using suspendCoroutine, but it's also not a blocker.

I don't really know what to do to change this... And i tried your modifications and it still works with the different test that i tried.

[Dioxo]

@msfjarvis , @FabianHenneke, I just found a bug when renaming a file or category, as we can use / to rename things, It's also possible to user ../ to go outside of a folder, for exemple, if in the root (for us is "store"), i have the password "github", if I edit the category to "../". It's possible to put outside the folder the password and now APS doesn't display it anymore because it searchs from "store" but the file is now at the same level that "store". This bug appears in all the forms to rename a file/folder. I'll search how to fix it by seeing the new destination if it's inside "store" otherwise it's not a valid operation.

[Dioxo]

Using canonical path we can confirm that the new file is inside our repository. Now I have a question if we should show a different error message to the user for each error case, right now we have 3 cases where the rename can fail. Or i just add complementary text to display error like "Category name can't be empty or category already exists, and the destination must be within the repository"

[msfjarvis]

Using canonical path we can confirm that the new file is inside our repository. Now I have a question if we should show a different error message to the user for each error case, right now we have 3 cases where the rename can fail. Or i just add complementary text to display error like "Category name can't be empty or category already exists, and the destination must be within the repository"

I'd recommend taking the approach from here and replacing the boolean with a result enum.

[fmeum]

LGTM, thanks a lot for the stream of improvements!