Show the group name in autocomplete#739
Conversation
but only if there are matching logins from multiple groups Fixes keepassxreboot#466 For users that use groups and have similar names for multiple logins but organized in different groups
varjolintu
left a comment
There was a problem hiding this comment.
Replying through a review:
This should be an optional feature, but enabled by default.
|
I added the option. What do you think? I was only able to translate it to german and spanish, next to english. What about the the other languages? |
|
That option is only in the options page. It's not saved to the extension settings. So the functionality is still missing. Also, languages are translated via Transifex, so there's no need to add any translations here, except for the main English translation file. |
I compared this to other settings and its pretty much the same. I guess there is some generic de/serialization of all fields of the settings page from/to the storage? I also tested it and it does work as expected.
Ok I removed them |
|
Thanks. I'll review this again. |
|
I couldn't get this change to work. The credentials were still shown without the group name when placing credentials for the same site in different groups. |
|
Did you use a build of the keepassxc desktop app that adds the group to the messages for the browser extension (see the PR description)? |
|
Oops!! |
| function getLoginText(credential, withGroup) { | ||
| const group = (withGroup && credential.group) ? `[${credential.group}] ` : ''; | ||
| const visibleLogin = (credential.login.length > 0) ? credential.login : tr('credentialsNoUsername'); | ||
| const text = `${group}${credential.name} (${visibleLogin})`; |
There was a problem hiding this comment.
Was this ordering already fixed? I cannot see @droidmonkey's message about the line anymore.
There was a problem hiding this comment.
I don't remember any comment regarding this, can you add it again please?
There was a problem hiding this comment.
While waiting for the answer, I would actually prefer that the group name would be added last. The username should be always visible as first.
There was a problem hiding this comment.
I recinded my comment, that was basically what it was (preserve the ordering). But after using the new order I actually liked it. Can we make it an option?
There was a problem hiding this comment.
In my opinion I would rather not add settings because already now there are so many and it's hard to understand all of them. They give the impression that KeepassXC is only for tech people but it wouldn't have to be. Also it adds complexity to the code.
Maybe rather come up with a good way to display these 3 informations in a sane way and do not fear the change. Maybe some people need to get used to it but it won't take long to adopt I believe.
But in the end it is your project. You decide and I will change it :)
There was a problem hiding this comment.
I agree that there's too much different options already.
Just a sidenote, but I'm already planning a settings page upgrade and it would definitely need some Advaced settings tab where we could put all extra settings which doesn't concern the normal user.
|
Thanks for merging this. If you can, keepassxreboot/keepassxc#4111 still needs to be merged so the feature can actually be used. |
For users that use groups and have similar names for multiple logins but organized in different groups
The group name is only added if there are matching logins from multiple groups. I read the idea in #466 about providing a way for templating but for now I decided to go a simpler route which maybe is sufficient as well.
Fixes #466
Requires keepassxreboot/keepassxc#4111
I decided to change the order of values inside the string because that was the most intuitive way to display the group name for me. I hope you like it, but I am also open for any better idea.