keepassxreboot / keepassxc

KeePassXC is a cross-platform community-driven port of the Windows application “Keepass Password Safe”.
https://keepassxc.org/
Other
20.17k stars 1.42k forks source link

Fix Copy Password button when text is selected #10853

Closed c4rlo closed 4 weeks ago

c4rlo commented 1 month ago

When the user chooses to copy the password for an entry to the clipboard, previously there was logic to check if text was selected, and if so, that text was instead copied to the clipboard. That made sense if (a) the user invoked the Copy Password action via its keyboard shortcut, and (b) that keyboard shortcut was configured (as per default) to be Ctrl-C, i.e. the same as the system action for copy-to-clipboard.

However, it made no sense if the user invoked that action in some other way, for example by clicking the corresponding toolbar button.

It also made no sense in the case that the Copy Password action had some other keyboard shortcut assigned. Also, if some other action had Ctrl-C assigned, the logic would not kick in then.

Fix all of the above by modifying the keyboard shortcut logic to intervene precisely in the case where a shortcut is pressed that matches the system copy-to-clipboard shortcut; only in that case do we now check if text is selected and if so copy that to the clipboard instead of the action we would otherwise take.

Fixes #10734.

Testing strategy

Added test to TestGui.cpp. Also confirmed via manual testing.

Type of change

droidmonkey commented 1 month ago

Love it, glad to use the new action collection too! I really couldn't figure out a fix to this, too focused on the toolbar action and not the keyboard shortcut.

c4rlo commented 1 month ago

I actually have a slightly simpler approach now, based on QObject::installEventFilter(). Moving this back to Draft while I consider it.

c4rlo commented 1 month ago

Ok, this is ready for review now. I've undone all the refactoring and kept this more focused.

c4rlo commented 1 month ago

The macOS CI failure seems like some kind of test flakeyness, unrelated to this change. I have created #10901 to address that.