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 "Lock selected database" action on toolbar #6652

Merged

Conversation

yamabiiko
Copy link
Contributor

This feature allows locking a single database using an icon in the toolbar.
Also the "Lock databases" icon is changed to make it more clear that its going to lock all databases.
Closes #6445
The main reason behind this change is allowing to lock a single database when having multiple databases open (without closing it), as described in the issue above.

I wanted to update the translations (change the "Lock databases" string into "Lock all databases" to make the description less confusing and add the "Lock selected databases" string for translations) but I am not quite sure how to do it (the update script seems to fail).

Screenshots

2021-06-18_19-06-1624037539
2021-06-18_19-06-1624039071
2021-06-18_19-06-1624039101
2021-06-18_19-06-1624039117

Testing strategy

This is basically an UI change (existing code was used) so it should be already covered by tests.

Type of change

  • ✅ New feature (change that adds functionality)
  • ✅ Refactor (significant modification to existing code)

@droidmonkey
Copy link
Member

Nice, good icon too

@michaelk83
Copy link

I'd place the "lock all" button next to the "lock current" button, or maybe as a dropdown option. So main button would lock the current DB, and dropdown → Lock All would lock everything.

[🔒|▾]
[Lock All]

@droidmonkey
Copy link
Member

droidmonkey commented Jun 18, 2021

Hmmmmm that is not a bad idea. I do like consolidating it into a drop down. But is the default action lock all or lock current?

@yamabiiko
Copy link
Contributor Author

yamabiiko commented Jun 19, 2021

I am not quite sure if it's worth changing the toolbar to add a drop down just for that (why should add entry, delete entry, change entry separate then?), also the "lock all" position is to keep it consistent with the interface overview, but if we decide to change it I'm ok with it

@michaelk83
Copy link

michaelk83 commented Jun 19, 2021

why should add entry, delete entry, change entry separate then?

Add/edit/delete are completely different actions, even if related. Lock current and lock all are both lock actions.

the "lock all" position is to keep it consistent with the interface overview

Then the question is what makes more sense: group the "current DB actions" (open/save/lock), or group the "lock actions" (lock current/lock all). My non-dropdown idea was to group all of them on the left: [Open][Save][Lock Current][Lock All]. I admit the Toolbar section in the User Guide will need to be updated, but it will need to be updated anyway.

If we go with the drop-down, I'd argue for lock current to be the default:

  • Most users will only have one DB open. For them, both actions are the same. It's still a single click in the most common case. (Maybe even hide the drop-down when there's only one DB open.)
  • Those who work with multiple DBs are the ones who want the "lock current" option. I expect for them, "lock current" would be more common: you unlock one at a time, and you lock one at time (often LIFO style, probably). If "lock current" is the default, that makes the (presumably) common use always a single click. If you make "lock all" the default, then "lock current" can't be done with less than two clicks.
  • If locking the current DB also closes the tab (or at least switches to the next unlocked one), that provides a clear visual indication that not everything is locked yet, and then locking everything is just a matter of repeatedly clicking the button a few times.
  • The dropdown provides a shorthand to lock everything with at most two clicks, but that's only shorter if you have more than 2 DBs open. That's probably rare even among multi-DB users.
  • As a fallback, keyboard shortcuts are the same "length" for both actions.
  • Aesthetically, the lock current icon is more generic.

Anyway, the final decision is up to @droidmonkey .

@droidmonkey
Copy link
Member

We have a drop down choice for the Auto-Type button, which I just noticed you are missing in your toolbar. This might be because you are either running Wayland or didn't build the autotype plugin. A drop down button is rather trivial to setup, you can use the Auto-Type one in code as reference.

@yamabiiko
Copy link
Contributor Author

I have started working on the dropdown change and the issue I've encountered is that if the current database is already locked the button is disabled resulting in the dropdown menu being disabled too. As far as I know, this behaviour can't be changed without an override to the QToolButton class. So the possible solutions are:

  • override QToolButton's mouseEventFilter in a custom class like DatabaseLockToolButton
  • place both icons without a dropdown (one next to each other)
  • use as a default action for the icon's dropdown "Lock all" (so the action is not disabled if our current tab is on a locked db)

2021-06-25_10-06-1624610615

2021-06-25_10-06-1624610608

@michaelk83
Copy link

The other icons aren't disabled when the DB is locked, so it probably has something to do with how the button is connected. So another option is disconnect it when the DB is locked, and reconnect when unlocked (or when switching tabs). Or just replace it with the lock-all button when the current DB is locked.

But the solution I would prefer is don't show tabs of locked DBs. When the current DB is locked, close its tab. That way, the UI immediately switches to the next unlocked tab, which gives a visual indication that there are still unlocked DBs, and allows to quickly and conveniently close the next DB by the clicking the same button again (without even moving the mouse).

The question is, would this affect how multiple DBs are unlocked? If you had 2-3 DBs open, and you lock-all, and close KPXC then launch it again, do you get 2-3 tabs of locked DBs? If you had 10 or 15 DBs that you worked with recently (even not open at the same time), would you get 10-15 tabs when you launch KPXC?

@michaelk83
Copy link

michaelk83 commented Jun 25, 2021

But the solution I would prefer is don't show tabs of locked DBs. When the current DB is locked, close its tab.
...
The question is, would this affect how multiple DBs are unlocked?

Hmm, yes, this conflicts with #5427. @droidmonkey , thoughts? Maybe close the tabs in the main UI in this PR, but still keep the last few recents in the unlock widget in #5427?

I'd rather not change the default to "lock all", at least, for the reasons I listed earlier.

Another option: leave the button (and drop-down) disabled if the current tab is locked, but switch to the next unlocked tab when locking.

@droidmonkey
Copy link
Member

Locking databases does NOT close them. This behavior definitely shouldn't change.

@michaelk83
Copy link

michaelk83 commented Jun 25, 2021

Ok. Then I vote for this:

Another option: leave the button (and drop-down) disabled if the current tab is locked, but switch to the next unlocked tab when locking.

Once the tab is switched, the button should be automatically re-enabled, and drop-down will be accessible. Normally you'd only switch back to a locked tab to unlock it. Other than that, you keep working with the unlocked tabs, where the button is enabled.

@yamabiiko
Copy link
Contributor Author

yamabiiko commented Jul 2, 2021

I have changed the code to automatically switch to an open tab when a database is locked (if available). I also have updated the menu, moved the action "Lock Databases" from "Tools" (as it did not seem appropriate) to "Database) and added the lock single database action to the menu.
I also have changed the current shortcut "CTRL+L" and assigned it to single database lock instead of lock all (which I changed to CTRL+SHIFT+L). This can be reverted but it seems more proper that way to me.

2021-07-02_18-07-1625244525
2021-07-02_18-07-1625244595
2021-07-02_18-07-1625244602
2021-07-02_18-07-1625244606

recording.mp4

@michaelk83
Copy link

Looks nice. Just maybe change "Lock Databases" to "Lock All Databases", to contrast it more with "Lock selected database". When you hover over the main button, does it show "Lock selected database" in a tooltip? Also, the capitalization is inconsistent. Looking at the rest of the menu, this should be "Lock Selected Database".

@yamabiiko
Copy link
Contributor Author

I have changed the strings as suggested

@michaelk83
Copy link

LGTM. The rest is up to @droidmonkey .

@yamabiiko yamabiiko force-pushed the feature/single-lock-database branch 4 times, most recently from 7d16853 to c5c7cd2 Compare July 3, 2021 11:19
@chron-isch
Copy link

I've been testing this for over a week now without a problem. The option of locking just a single DB is something I've always missed in KeepassXC, so THANK YOU for the work.

@droidmonkey droidmonkey added this to the v2.7.0 milestone Nov 14, 2021
@droidmonkey droidmonkey self-requested a review November 14, 2021 04:22
@droidmonkey droidmonkey force-pushed the feature/single-lock-database branch from c5c7cd2 to 078e37c Compare November 23, 2021 14:34
@codecov-commenter
Copy link

codecov-commenter commented Nov 23, 2021

Codecov Report

Attention: Patch coverage is 61.53846% with 15 lines in your changes missing coverage. Please review.

Project coverage is 64.26%. Comparing base (4567265) to head (883effa).
Report is 545 commits behind head on develop.

Files with missing lines Patch % Lines
src/gui/DatabaseTabWidget.cpp 0.00% 15 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #6652      +/-   ##
===========================================
+ Coverage    64.22%   64.26%   +0.04%     
===========================================
  Files          336      336              
  Lines        42377    42411      +34     
===========================================
+ Hits         27214    27252      +38     
+ Misses       15163    15159       -4     

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


🚨 Try these New Features:

@droidmonkey droidmonkey force-pushed the feature/single-lock-database branch from 078e37c to 938eda2 Compare November 24, 2021 05:01
@droidmonkey
Copy link
Member

This works great, ready for merge.

@droidmonkey droidmonkey force-pushed the feature/single-lock-database branch from 938eda2 to 84a078f Compare November 27, 2021 16:51
@droidmonkey droidmonkey force-pushed the feature/single-lock-database branch from 84a078f to 6eab926 Compare December 9, 2021 03:45
Closes keepassxreboot#6445

Switch tab when locking a database and move Lock Database actions to the Database section of the toolbar.
@droidmonkey droidmonkey force-pushed the feature/single-lock-database branch from 6eab926 to 883effa Compare December 12, 2021 17:38
@droidmonkey droidmonkey merged commit c88d8c8 into keepassxreboot:develop Dec 13, 2021
@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
pr: new feature Pull request that adds a new feature user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Please add feature to selectively lock a single database
6 participants