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

Add tag functionality (Issue #508) #6487

Merged
merged 1 commit into from
Jan 28, 2022
Merged

Add tag functionality (Issue #508) #6487

merged 1 commit into from
Jan 28, 2022

Conversation

xvallspl
Copy link
Contributor

@xvallspl xvallspl commented May 5, 2021

Very early state, just testing the waters to gather feedback.

Add tag functionality (addresses issue #508)

Work on this PR:

  • Display tags in entry preview
  • Allow modification of tags on entry edit
  • Introduce search term "tag:"
  • Implement new side panel, similar to the groups one, with the list of available tags. When clicking on one of those tags, the entry view widget should filter all entries that don't contain the selected tag.
  • Display tags as "pills"
  • Make tag editing an autocomplete dropdown
  • Adapt the styles, make everything beautiful (did the best I could)
  • Test

Work for subsequent PRs:

  • Update the menus with tag actions (still thinking which ones and what actions)
  • Delete a tag from the database from the side panel
  • When selecting a tag from the side panel filter groups and display only the ones containing entries with the tag
  • Check feature/modifications suggestions in comments and Issue Add Tag Feature  #508

Screenshots

Screenshot 2021-11-24 at 19 57 50

Screenshot 2021-11-24 at 19 54 16

TagsBox2

Testing strategy

Manually for the fast prototype, will come up with tests next.

Type of change

  • ✅ New feature (change that adds functionality)
  • ✅ Documentation (non-code change)

@xvallspl xvallspl changed the title Adds tags functionality (Issue #508) Add tag functionality (Issue #508) May 5, 2021
@xvallspl xvallspl requested review from droidmonkey and phoerious May 5, 2021 06:17
@michaelk83
Copy link

Figure out a way to improve tag editing and display

  1. Tags edit box should be an auto-complete drop-down. IOW, you can:
    1. Click drop-down arrow to get a list of existing tags, select from the list to add a tag. Maybe with checkboxes to add/remove multiple?
    2. Type partial tag name to filter the drop-down list.
    3. If entered text does not exactly match an existing tag, list shows an additional entry: "New: [entered text]". Click that or press Enter to create a new tag with that name and add it to the entry.
    4. If entered text matches an existing tag, click that or press Enter to add the existing tag.
    5. Added tags become pills in the edit box. You can type the next tag after the pills.
    6. X on the right end of each tag pill allows to remove that tag.
  2. In entry preview, tags show as pills near the bottom (no X's if the preview is not editable).
  3. In tags side panel, can delete the selected tag with Delete key (with confirmation), or via menu. This only removes the tag from the entries and from the known tags list. The entries themselves remain.

You can have a look at GMail's labels UI for more ideas. There is probably a ready UI component for that autocomplete drop-down with pills. No need to reinvent the wheel here.

@xvallspl
Copy link
Contributor Author

xvallspl commented May 6, 2021

Added a side panel listing the tags in the active group (see last gif in the Screenshots section). Double clicking on one of them filters by that tag in the entry view.

Doubts/TODOs:

  • Not sure double clicking is better than clicking [SOLVED]
  • No way to undo filtering yet [SOLVED]
  • Do we only want to show the tags from the current group or all of them?

@michaelk83
Copy link

My personal opinions:

Not sure double clicking is better than clicking

It should behave the same as group selection.

No way to undo filtering yet

Add an (all) meta tag at the top of the list in the side panel?

Do we only want to show the tags from the current group or all of them?

I'd say, filter the groups to show only those that have matching entries, and in the preview panel list the entries in the selected group out the filtered ones.

@xvallspl
Copy link
Contributor Author

xvallspl commented May 7, 2021

My personal opinions:

Not sure double clicking is better than clicking

It should behave the same as group selection.

No way to undo filtering yet

Add an (all) meta tag at the top of the list in the side panel?

Do we only want to show the tags from the current group or all of them?

I'd say, filter the groups to show only those that have matching entries, and in the preview panel list the entries in the selected group out the filtered ones.

This PR can get too big with all this functionalities introduced at the same time. I will not touch what I already produced and I will continue implementing the most basic features, but I will keep certain features for subsequent PRs (like allowing to delete a tag in all entries of the database by clicking delete on the side panel).

Thanks! this is all very useful

@droidmonkey
Copy link
Member

Concur with that, MVP for this PR

@droidmonkey droidmonkey added this to the v2.7.0 milestone May 8, 2021
@xvallspl
Copy link
Contributor Author

xvallspl commented May 17, 2021

Added a couple more features and improvements: improved side panel, restore side panel selection after entry editing, tag pills view and autocomplete in Edit entry view.

The Tag Widget I used for the last two, https://github.com/nicktrandafil/tags, pulls code from QtGui/private, which either we add as a dependency or I can try to reimplement the functions we use. For the moment it makes the tests fail.

The formatting mistakes are introduced by make format. Which feedback should I pay attention to?

Updated the screenshots.

@michaelk83
Copy link

Looks like the auto-complete delay is pretty high. Is that a setting or a limitation of the code?
I was thinking it should show and update the auto-complete list while you type.

@michaelk83
Copy link

There's a more official (?) fork at https://github.com/Qt-Widgets/tags . It says "3 commits ahead, 4 commits behind nicktrandafil:master". It might have fewer build issues?

@xvallspl
Copy link
Contributor Author

xvallspl commented May 17, 2021

There's a more official (?) fork at https://github.com/Qt-Widgets/tags . It says "3 commits ahead, 4 commits behind nicktrandafil:master". It might have fewer build issues?

It has the same problem: https://github.com/Qt-Widgets/tags/blob/e520e47e40538b7d9c418776413fe4c302470c16/src/tags.cpp#L35

@xvallspl
Copy link
Contributor Author

Looks like the auto-complete delay is pretty high. Is that a setting or a limitation of the code?
I was thinking it should show and update the auto-complete list while you type.

There's no delay, it just autocompletes (doesn't suggest by default) and my choice in the video was starting with a letter (case sensitive) that no other tag started with yet.

@droidmonkey
Copy link
Member

Damn @xvallspl that is some great progress! The tag widget works well, but I agree it really shouldn't be using private functions.

@xvallspl xvallspl linked an issue May 26, 2021 that may be closed by this pull request
@droidmonkey
Copy link
Member

I'm pretty damn excited about this feature

@xvallspl
Copy link
Contributor Author

@droidmonkey TeamCity is complaining about features I'm using from C++17 and C++14. Should we configure it for at least C++17? Should I stick to earlier standards?

@droidmonkey
Copy link
Member

We are currently on C++ 14 standard. I think we can move to 17 though because xenial is out. @phoerious ?

@phoerious
Copy link
Member

I guess.

@droidmonkey
Copy link
Member

Although I noticed you are using std functions instead doing Qt functions to do some stuff with lists / sets. Favor Qt where possible.

@droidmonkey droidmonkey mentioned this pull request Sep 8, 2021
@xvallspl xvallspl force-pushed the feature/tags branch 2 times, most recently from 5b21b31 to 92eb42e Compare November 25, 2021 18:46
@xvallspl xvallspl force-pushed the feature/tags branch 2 times, most recently from 34d81b5 to f1c9ae1 Compare December 14, 2021 20:22
@droidmonkey
Copy link
Member

@xvallspl this is awesome!

@droidmonkey
Copy link
Member

droidmonkey commented Dec 16, 2021

One bit a feedback, it looks like the tag list filters based on the current group. That doesn't really make much sense since tags are supposed to be sort of meta groups that cross-cut your database. Prefer to have all known tags in the database be visible in the pick list at all times.

Another good thing to do would be to actually just populate the search box with the "tags:<clicked tag>" and let the search happen naturally from there. This has the benefit of showing the user that tags == search and it also lets the user narrow down the search directly while retaining the tag portion.

@xvallspl
Copy link
Contributor Author

I was thinking of tags as confined to a group, just another way to group your entries within the group. I'll take a look at it as soon as I get out of the git applying hell I'm on.

@acolomb
Copy link

acolomb commented Dec 17, 2021

I strongly agree that tags should be global, not confined to a group. The purpose is to have a second grouping scheme based on tags, orthogonal to the groups scheme.

@codecov-commenter
Copy link

codecov-commenter commented Dec 24, 2021

Codecov Report

Attention: Patch coverage is 61.16015% with 308 lines in your changes missing coverage. Please review.

Project coverage is 64.24%. Comparing base (56a1b46) to head (783906f).
Report is 539 commits behind head on develop.

Files with missing lines Patch % Lines
src/gui/tag/TagsEdit.cpp 59.30% 243 Missing ⚠️
src/gui/DatabaseWidget.cpp 70.27% 22 Missing ⚠️
src/core/EntrySearcher.cpp 36.36% 14 Missing ⚠️
src/gui/tag/TagModel.cpp 68.29% 13 Missing ⚠️
src/gui/MainWindow.cpp 0.00% 8 Missing ⚠️
src/core/Entry.cpp 42.86% 4 Missing ⚠️
src/core/Database.cpp 92.00% 2 Missing ⚠️
src/gui/DatabaseWidgetStateSync.cpp 84.62% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #6487      +/-   ##
===========================================
- Coverage    64.33%   64.24%   -0.08%     
===========================================
  Files          337      339       +2     
  Lines        42448    43201     +753     
===========================================
+ Hits         27305    27753     +448     
- Misses       15143    15448     +305     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@droidmonkey
Copy link
Member

I squashed all the intermediate commits and updated the translations and code format. One bug I found was that when adding a new tag to an entry, it didn't show up in the tag selection panel in the lower left until I locked/unlocked the database.

@xvallspl
Copy link
Contributor Author

Thanks, Jonathan. I'll take a look as soon as I can.

@eholzbach
Copy link

I strongly agree that tags should be global, not confined to a group. The purpose is to have a second grouping scheme based on tags, orthogonal to the groups scheme.

This is what I've been hoping for. For example, I upgraded my primary Yubikey. I did not have a single place to track where it was being used. If I was able to tag entries in KeePassXC, it would be easy to list all the sites I need to update, without having to break my group schema.

@droidmonkey
Copy link
Member

droidmonkey commented Jan 23, 2022

I picked this up and made substantial changes to implement the rest of the features. I even tossed in a couple default "tags" (more like saved searches) like expired entries and weak passwords.

image

@xvallspl
Copy link
Contributor Author

The sidebar looks so much better! Nice to see the changes in the searchbar!

@droidmonkey droidmonkey marked this pull request as ready for review January 26, 2022 04:57
@droidmonkey
Copy link
Member

Finished up the code cleanup, this is ready for merge and 2.7.0!

src/gui/tag/TagModel.cpp Outdated Show resolved Hide resolved
* show the tags in the entry preview
* allow searching by tag
* add a sidebar listing the tags in the database
* filter entries by tag on click
* Introduce a new TagsEdit widget that provides pill aesthetics, fast removal functionality and autocompletion
* add tests for the tags feature
* introduce the "is" tag for searching. Support for weak passwords and expired added.
@droidmonkey droidmonkey merged commit 4a21cee into develop Jan 28, 2022
@droidmonkey droidmonkey deleted the feature/tags branch January 28, 2022 21:13
@phoerious phoerious added pr: new feature Pull request that adds a new feature and removed new feature labels Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EPIC pr: new feature Pull request that adds a new feature user interface ux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Tag Feature
7 participants