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

Enable browser-like DbTab experience (Alt + Nums) #4063

Merged
merged 1 commit into from
Jan 27, 2020

Conversation

JulianVolodia
Copy link
Contributor

1-9 goes to 1-9 tab; 0 - goes to the last one

Please read Testing strategy, thanks!

Type of change

  • ✅ New feature (non-breaking change which adds functionality)

Description and Context

In browsers we could not only use CTRL + PgDn/PgUp and CTRL (+ SHIFT) + TAB to switch tabs.
There is also ALT + which have various types of work. 1-9 operates on tabs from 1st to 9th one, and (on Chrome?) the 0 goes to last one.
On the another similar to KPXC application found this kind of feature. ;)

Screenshots

N/A

Testing strategy

This is WIP, I wonder if this code could be (bc of less complexity) or should change to functor or some Signal mapper.

Checklist:

  • ✅ I have read the CONTRIBUTING document. [REQUIRED]
  • ✅ My code follows the code style of this project. [REQUIRED]
  • ✅ All new and existing tests passed. [REQUIRED]
  • ✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]
    Not yet: - ✅ My change requires a change to the documentation, and I have updated it accordingly.
    Not yet: - ✅ I have added tests to cover my changes.

@droidmonkey
Copy link
Member

droidmonkey commented Dec 27, 2019

  1. This should be a keyboard shortcut assigned within the DatabaseTabWidget.

  2. You can use the modern Signal/Slot call methods using lambda functions:

auto shortcut = QShortcut(Qt::Alt + Qt::Key_1, this);
connect(shortcut, SIGNAL(activated()), this, [this] { selectDatabaseTab(1); });

@JulianVolodia
Copy link
Contributor Author

1. This should be a keyboard shortcut assigned within the DatabaseTabWidget.

2. You can use the modern Signal/Slot call methods using lambda functions:
auto shortcut = QShortcut(Qt::Alt + Qt::Key_1, this);
connect(shortcut, SIGNAL(activated()), this, [this] { selectDatabaseTab(1); });

Thanks for your response. I will change PR in next day. Best

@JulianVolodia
Copy link
Contributor Author

1. This should be a keyboard shortcut assigned within the DatabaseTabWidget.

@droidmonkey, I don't see any QShortcuts in there.
Also, MainWindow has shortcuts for other Db shortcuts stuff there. Does it needs refactor indeed?

@droidmonkey
Copy link
Member

droidmonkey commented Dec 28, 2019

Arguably those shortcuts are also misplaced. The DB tab widget directly controls which tab is selected so it makes way more sense to have it bind the keyboard shortcut. I wouldn't mind the other tab controlling shortcuts to move as well.

@droidmonkey
Copy link
Member

I made the necessary changes, ready for merge.

* Pressing ALT+1-9 goes to 1-9 tab
* Pressing ALT+0 goes to the last tab
@JulianVolodia
Copy link
Contributor Author

JulianVolodia commented Jan 23, 2020 via email

@droidmonkey
Copy link
Member

Oh its all done

@JulianVolodia
Copy link
Contributor Author

JulianVolodia commented Jan 23, 2020 via email

@droidmonkey droidmonkey merged commit 06e5f19 into keepassxreboot:develop Jan 27, 2020
@JulianVolodia
Copy link
Contributor Author

Arguably those shortcuts are also misplaced. The DB tab widget directly controls which tab is selected so it makes way more sense to have it bind the keyboard shortcut. I wouldn't mind the other tab controlling shortcuts to move as well.

I was mentioning this, about changing other. I will look if I catch sth up or if could help with something other.

Appreciate your work, thank You for what you are doing for us, @droidmonkey

@droidmonkey
Copy link
Member

So I tried moving the shortcut commands and realized if you do that then they only work when the tabs have explicit focus which is not desirable. So moved back to MainWindow

@JulianVolodia
Copy link
Contributor Author

JulianVolodia commented Jan 28, 2020

So I tried moving the shortcut commands and realized if you do that then they only work when the tabs have explicit focus which is not desirable. So moved back to MainWindow

Great! I now understand it completely, that seems familiar with what I was thinking about during stub of change. Thanks that you fixed it.

I hope I will give you more help in future, because that tool is avvesome and and want to move from KeePassX https://keepass.info/ KeePass completely.

Happy hacking!

@varjolintu
Copy link
Member

This breaks the possibility to write @ char on macOS, because the shortcut for that is Alt+2. So any attempt to write, for example, an email address manually to an entry always switches to a second database.

@droidmonkey
Copy link
Member

@varjolintu is it not possible to enter an '@' using Shift+2? Is this due to an non-US keyboard? Before I disable this feature on macOS, need to understand how widespread the issue actually is.

@varjolintu
Copy link
Member

@droidmonkey non-US keyboard layout, yes. Shift+2 gives " for Finnish. I need to lookup the whole layout table somewhere and see which one are affected.

@JulianVolodia
Copy link
Contributor Author

JulianVolodia commented Feb 8, 2020

Hey @varjolintu could you tell me what ⌘Command + 0 does on your MacOS with browser with 13 or more tabs?
I checked that MacOS should have behaviour like that in browser but using ⌘Command + NumLine keys. Could you tell me that this is OK and do not collide with anything on MacOS?
ref: https://www.wikihow.com/Switch-Tabs-in-Chrome

with regards to https://www.idownloadblog.com/2017/05/15/keyboard-shortcut-switch-safari-tabs/ on MacOS: **

  1. Hold Shift+⌘Command and press the right or left arrow key. (...to go to next/prev tab in browser) **
  2. Control+Tab or Control+Shift+Tab to cycle through your tabs. (...same as (1) )
  3. Hold down ⌘Command + press 1-9 on your numeric keypad. The numbers on your keypad will correspond with the order and count of the tabs currently active.

Same from 1-2 above in 3-4 said here - no reference about NumLine https://www.quora.com/Is-there-a-shortcut-key-for-switching-between-tabs-in-Chrome-for-Mac/answer/Chris-Chovanek :

  1. Command + Tab = switch between applications.
  2. Command + ~ = switch between windows of an application. **
  3. CTRL + Tab = move forward through tabs (in same window)
  4. CTRL + Shift + Tab = move backward through tabs (in same window)

On Linux:
On Linux, Firefox and Chromium, on both PL and US keyboard switch tabs when used combination:

  • LeftAlt + NumLine
  • LeftCtrl + NumLine
  • RightCtrl + NumLine

For RightAlt + NumLine works as above only on US kb laybout. On PL produces additional characters.

Dear @varjolintu I've checked Linux with Finnish by setxkbmap fi and (RightAlt) AltGr + 2 produces @ but (LeftAlt) Alt works as the same as for US kb layout

Dear @droidmonkey maybe there is another general 'ModX' key in Qt introducted to have this working across platforms and kb layouts? I have a glance on it but not found it.

@varjolintu
Copy link
Member

@JulianVolodia ⌘Command + 0 does nothing with Chromium or Firefox. The issue is not using ⌘Command+2 but ⌥Alt+2.

@varjolintu
Copy link
Member

The same ⌥Alt+2 for writing @ applies to following keyboard layouts on macOS:
Ainu, Belarusian, Croatian, Czech, Estonian, Finnish, Icelandic, Khmer, Pashto, Persian, Polish, Russian, Slovak, Ukrainian, Uzbek and Vietnamese.

@JulianVolodia
Copy link
Contributor Author

JulianVolodia commented Feb 8, 2020

@JulianVolodia ⌘Command + 0 does nothing with Chromium or Firefox. The issue is not using ⌘Command+2 but ⌥Alt+2.

I am asking you because:

  • I don't have hackintosh on currently using notebook with limited resources and can't reproduce behaviour with good UX for MacOS users

I understand because that is a glitch. Change introduce Alt + NumLine as I proposed and @droidmonkey prepared change, but I missed completely testing on MacOS - so I want to repair my mistake.

That is why I asking that. On MacOS there should be only using Command as you said. But @varjolintu please tell me how Chrome or Firefox behave on your system, or actually what is more natural for MacOS user. Thanks in advance and sorry for inconvenience.

PS to C + 0 --> good, and Command + 9? does it go to last tab in them, or to nineth of them?

@varjolintu
Copy link
Member

varjolintu commented Feb 8, 2020

@JulianVolodia I understand. This happens with my desktop with Apple Keyboard. Also with my old MacBook Pro.

And I agree using the ⌘Command with macOS would be far better, because it seems to switch tabs with both Chromium and Firefox (so that must be the standard way to do it). Just the ⌘Command+0 doesn't switch to the tenth tab. It does't do anything.

It's common that with non-US keyboard layouts, the tab switching always happens with CTRL+Tab and CTRL+Shift+Tab. Sadly, I haven't been able to get that to work with macOS and Qt.

@JulianVolodia
Copy link
Contributor Author

JulianVolodia commented Feb 8, 2020

@varjolintu wait a moment. Everything is allright on your side. Only bleeding-edge KeepassXC have infroduced bug because of me.

And I agree using the ⌘Command with macOS would be far better, because it seems to switch tabs with both Chromium and Firefox (so that must be the standard way to do it). Just the ⌘Command+0 doesn't switch to the tenth tab. It does't do anything.

On Chomium/Firefox on Linux also does nothing. I must mix something up when reporting, or recheck on Windows.
On Firefox@Linux works only with (Left)Alt+NumLine. On Chrome works both with Ctrl and (L)Alt.
1-8 goes to tabs, and 9 to last one, of all.

It's common that with non-US keyboard layouts, the tab switching always happens with CTRL+Tab and CTRL+Shift+Tab. Sadly, I haven't been able to get that to work with macOS and Qt.

Um.... that's strange. Have you used on MacOS, Qt.Key s:
Key_Control as Command keys, and
Key_Meta as Control keys ?

Could you try to change locally and post feedback?

@varjolintu
Copy link
Member

@JulianVolodia I made a new PR for it: #4305

@JulianVolodia
Copy link
Contributor Author

I must check why (different on Windows?) I proposed combo with 0 to go to last not 9.

On Linux and MacOS this is 9, which activate last one.

@JulianVolodia
Copy link
Contributor Author

JulianVolodia commented Feb 8, 2020

@varjolintu I checked your commit and 👍 because as I faith you tested it should fix @ character on MacOS but 👎 because you said that cmmnd + 0 do nothing but coded the behaviour of cmmnd + 9.

@varjolintu
Copy link
Member

varjolintu commented Feb 8, 2020

@JulianVolodia With KeePassXC ⌘Command+0 changes to last selected tab, as it is coded. What you asked was what it does with browsers. As I said, nothing happens with Chromium and Firefox.

I don't see any reason why ⌘Command+0 would be changed to ⌘Command+9 in KeePassXC.

EDIT: I was wrong. See below.

@JulianVolodia
Copy link
Contributor Author

Have you read my comment? I don't know why I proposed 0 as last. 9 should go because UX of 80% users who use browser (most commonly used SW these days...) will look for cmd+9 not what you coded, is it clear now @varjolintu ?

@JulianVolodia
Copy link
Contributor Author

And, as I mentioned there should go fix on Linux and I must check on windows but I dont have any other than Linux box near me under control. Cheers.

@droidmonkey could we change it once again?

@varjolintu
Copy link
Member

@JulianVolodia Sorry. Missed that. You are absolutely correct. 9 goes to the last tab with any browser.

@JulianVolodia
Copy link
Contributor Author

JulianVolodia commented Feb 8, 2020

@JulianVolodia Sorry. Missed that. You are absolutely correct. 9 goes to the last tab with any browser.

If you say so, I am sure now @varjolintu there is another bug I introducted in the very first place that missed (and cannot check now) if on Windows there is same as on MacOS/Linux and 0 was another mistake.

I just want to return your full support for KB but other users on Mac could like this 9'combo as on browser. I hope we are good now. Cheers

JulianVolodia added a commit to JulianVolodia/keepassxc that referenced this pull request Feb 9, 2020
This is follow up of:
 PR keepassxreboot#4063 Enable browser-like DbTab experience (Alt + Nums)
 PR keepassxreboot#4305 Fix browser-like DbTab experience with macOS
This commit introduces desired behaviour across platforms.
 On MacOS only Command key + Nums combination should be used. (on MacOS, ⌘Command is QT::CTRL)
 On Linux and Windows there are combinations with Ctrl and (Left) Alt. (code could be reused)

This is not breaking for various keyboard layout organization on MacOS because of @varjolintu fixes,
nor breaking on Linux or Windows because they use AltGr (Right Alt) as modkey.
JulianVolodia added a commit to JulianVolodia/keepassxc that referenced this pull request Feb 9, 2020
This is follow up of:
 PR keepassxreboot#4063 Enable browser-like DbTab experience (Alt + Nums)
 PR keepassxreboot#4305 Fix browser-like DbTab experience with macOS
This commit introduces desired behaviour across platforms.
 On MacOS only Command key + Nums combination should be used. (on MacOS, ⌘Command is QT::CTRL)
 On Linux and Windows there are combinations with Ctrl and (Left) Alt. (code could be reused)

This is not breaking for various keyboard layout organization on MacOS because of @varjolintu fixes,
nor breaking on Linux or Windows because they use AltGr (Right Alt) as modkey.
@JulianVolodia
Copy link
Contributor Author

JulianVolodia commented Feb 9, 2020

@varjolintu said in #4305 :

@JulianVolodia I could've done any necessary fixes in this PR. Also, your new PR seems to break things.

Which things? BTW. in general that was rude imho, but maybe I am wrong. If you could, I hope you will tell me what things are broken and why you said that.

@varjolintu commented on change https://github.com/keepassxreboot/keepassxc/pull/4306/files/c66ad4144381e614e8c2ff62a6a817167f0eecf0 :

Now you are keeping CTRL+9 shortcut alive even with WIndows systems. Why did you remove the previous ifdef?

Have you read commit message before commenting? I described why it is there. Also, see #4063 (comment) because there is reference that Windows also have this feature working as on Linux.

Your commit does the same thing for Windows, but silently because on Windows MacOS preprocessor constant is not definied. I hope you made mistake in comment.

So, to be verbose and clear:

  • your ⌘Command combination works on MacOS as combo with Qt::CTRL (block w/o if clauses)
  • mine Linux and Windows boxes have combinations using their Control a.k.a. Qt::CTRL and that above is reused code for that case.
  • Linux and Windows but not MacOS (block with if clause) have also shortcut with Qt::ALT a.k.a (Left) Alt not (Right Alt|AltGr) - that will be added only on Windows and Unix but not on AIX. Best is to if clause the code verbosely but there is IMHO no chance to introduction of KeePassXC on AIX i.e. so no need to adding additional block.
    Defining explicitly, that you want this and not that is IMHO better than ELSE'ing some blocks.

Check full git diff against the develop from HEAD of that PR to see that it is more readable, when you remove 9 combo from end and change 0 to 9 combo. Easier to review, not 'hacked out' imho.

@droidmonkey should I delete my PR #4306 or you like it?

@varjolintu
Copy link
Member

@JulianVolodia What I meant was this: your new PR modified the shortcuts in a way the CTRL+num are already activated on every system, plus it added ALT+num shortcuts on Windows and Linux as additional shortcuts. So both of those were active, and that's not something we want, is it?

I'm not quite sure what do you mean. The whole point was just to change MacOS to use CTRL instead of ALT.

If we want to use some different shortcuts on AIX, then it would be easiest to add #if defined(Q_OS_UNIX) && !defined(Q_OS_MACOS) define to the PR.

@ebneazad
Copy link

ebneazad commented Feb 9, 2020 via email

@JulianVolodia
Copy link
Contributor Author

@varjolintu OK, so I need more coffee when I writing to someone about UX I think.

Let's start (and hope that will be end and you will get my point):

@JulianVolodia What I meant was this: your new PR modified the shortcuts...

Yes, this change is about that.

...in a way the CTRL+num are already activated on every system, ...

No. Was not. Have you read my commit msg?
On MacOS users use Command + Numline on browsers which are Qt::CTRL + Num. As you informed (thanks again) 1-8 change active tab to one of following beginnig, and 9 warps us to last. That is what you want, because Alt is not for you (and all non-US ppl or US using weird aka nonstandard characters). I used your comment to do that and documentation. Checked 3 sources as you could find on posts above. or here https://www.twam.info/wp-content/uploads/2010/08/us_international.png

So with 0 and 9 was clearly the bug. All ppl use 9 to end and 1-8 to static first tabs. 0 not used. Clear now I hope. Same goes below.

Here goes rollercoaster:
On Windows and Linux

  • (Left Alt aka Alt not Right/AltGr) + NumLine works in Mozilla (Thunderbird, Firefox, Chromium), and do described thing as I checked and you commited was left in your change Fix browser-like DbTab experience with macOS and Windows #4305 - good enought we say.
  • here goes sth else: On Windows and Linux Qt::CTRL (left and right, this is the same key) is not Command key as on MacOS. It's Ctrl and in Chromium Ctrl+NumLine does same thing as Alt+NumLine. Thats why I added it.

plus it added ALT+num shortcuts on Windows and Linux as additional shortcuts. So both of those were active, and that's not something we want, is it?

As I described above, and tired you probably (I feel tired about that, this is pointless for me much more but not for UX case of some group of users - as they could say more of them use Chromium than are from Finland so could say that this change is more important not because non-US layout).

I'm not quite sure what do you mean. The whole point was just to change MacOS to use CTRL instead of ALT.

That was easier to code reuse if I understand correctly. Than Kopi-Pasten theorem applying with 3 blocks - just unneded.

If we want to use some different shortcuts on AIX, then it would be easiest to add #if defined(Q_OS_UNIX) && !defined(Q_OS_MACOS) define to the PR.

So, easiest for you refactoring it, as the code readed in first place or easier to implement now?

I got your point, maybe I am just out of doing it well because time passed and must more practice.

@varjolintu
Copy link
Member

@JulianVolodia The PR is already updated so it would solve all the problems we have discovered.

droidmonkey added a commit that referenced this pull request Jul 7, 2020
Added

- Custom Light and Dark themes [#4110, #4769, #4791, #4796, #4892, #4915]
- Compact mode to use classic Group and Entry line height [#4910]
- View menu to quickly switch themes, compact mode, and toggle UI elements [#4910]
- Search for groups and scope search to matched groups [#4705]
- Save Database Backup feature [#4550]
- Sort entries by "natural order" and move lines up/down [#4357]
- Option to launch KeePassXC on system startup/login [#4675]
- Caps Lock warning on password input fields [#3646]
- Add "Size" column to entry view [#4588]
- Browser-like tab experience using Ctrl+[Num] (Alt+[Num] on Linux) [#4063, #4305]
- Password Generator: Define additional characters to choose from [#3876]
- Reports: Database password health check (offline) [#3993]
- Reports: HIBP online service to check for breached passwords [#4438]
- Auto-Type: DateTime placeholders [#4409]
- Browser: Show group name in results sent to browser extension [#4111]
- Browser: Ability to define a custom browser location (macOS and Linux only) [#4148]
- Browser: Ability to change root group UUID and inline edit connection ID [#4315, #4591]
- CLI: `db-info` command [#4231]
- CLI: Use wl-clipboard if xclip is not available (Linux) [#4323]
- CLI: Incorporate xclip into snap builds [#4697]
- SSH Agent: Key file path env substitution, SSH_AUTH_SOCK override, and connection test [#3769, #3801, #4545]
- SSH Agent: Context menu actions to add/remove keys [#4290]

Changed

- Complete replacement of default database icons [#4699]
- Complete replacement of application icons [#4066, #4161, #4203, #4411]
- Complete rewrite of documentation and manpages using Asciidoctor [#4937]
- Complete refactor of config files; separate between local and roaming [#4665]
- Complete refactor of browser integration and proxy code [#4680]
- Complete refactor of hardware key integration (YubiKey and OnlyKey) [#4584, #4843]
- Significantly improve performance when saving and opening databases [#4309, #4833]
- Remove read-only detection for database files [#4508]
- Overhaul of password fields and password generator [#4367]
- Replace instances of "Master Key" with "Database Credentials" [#4929]
- Change settings checkboxes to positive phrasing for consistency [#4715]
- Improve UX of using entry actions (focus fix) [#3893]
- Set expiration time to Now when enabling entry expiration [#4406]
- Always show "New Entry" in context menu [#4617]
- Issue warning before adding large attachments [#4651]
- Improve importing OPVault [#4630]
- Improve AutoOpen capability [#3901, #4752]
- Check for updates every 7 days even while still running [#4752]
- Improve Windows installer UI/UX [#4675]
- Improve config file handling of portable distribution [#4131, #4752]
- macOS: Hide dock icon when application is hidden to tray [#4782]
- Browser: Use unlock dialog to improve UX of opening a locked database [#3698]
- Browser: Improve database and entry settings experience [#4392, #4591]
- Browser: Improve confirm access dialog [#2143, #4660]
- KeeShare: Improve monitoring file changes of shares [#4720]
- CLI: Rename `create` command to `db-create` [#4231]
- CLI: Cleanup `db-create` options (`--set-key-file` and `--set-password`) [#4313]
- CLI: Use stderr for help text and password prompts [#4086, #4623]
- FdoSecrets: Display existing secret service process [#4128]

Fixed

- Fix changing focus around the main window using tab key [#4641]
- Fix search field clearing while still using the application [#4368]
- Improve search help widget displaying on macOS and Linux [#4236]
- Return keyboard focus after editing an entry [#4287]
- Reset database path after failed "Save As" [#4526]
- Use SHA256 Digest for Windows code signing [#4129]
- Improve handling of ccache when building [#4104, #4335]
- macOS: Properly re-hide application window after browser integration and Auto-Type usage [#4909]
- Auto-Type: Fix crash when performing on new entry [#4132]
- Browser: Send legacy HTTP settings to recycle bin [#4589]
- Browser: Fix merging browser keys [#4685]
- CLI: Fix encoding when exporting database [#3921]
- SSH Agent: Improve reliability and underlying code [#3833, #4256, #4549, #4595]
- FdoSecrets: Fix crash when editing settings before service is enabled [#4332]
@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 ux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants