keepassxreboot / keepassxc

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

Add ability to toggle window visibility (to/from Tray) [$30] #99

Open anatoli26 opened 8 years ago

anatoli26 commented 8 years ago

Hi everybody,

That's great we the users of KeePassX have a new dev team for the app!

It would be nice to have an Option called Toggle Window Visibility binded to a global key combination (like the auto-complete) to bring the main window to front and to minimize it with the same key combination. If the minimize-to-tray option is enabled, the minimize action of this option should minimize the window to tray.

To give this issues a higher priority, I'm willing to contribute economically. Those who could implement it please specify how much should be collected and I could contribute to the implementation on bountysource.com or your crowdfunding site of preference.

anatoli26 commented 8 years ago

An alternative to these args could be a new key combination (like the auto-complete) for toggling the window (i.e. bring to front / minimize).

TheZ3ro commented 8 years ago

I like the auto-complete like alternative. It's easier than cli args and it's more user-friendly.

Also I think that the only useful option is the toggle. I don't see much utility in minimize and bring to front when you have the toggle option working.

I'm changing the title

anatoli26 commented 8 years ago

Yepp, the idea for the cli args appeared before the 'start minimized' option was introduced, as a simpler alternative (no need to change the UI, etc.). Now that we have it, if we will also have a key combination for toggle, that would cover almost all the needs. And the toggle combination is indeed needed as sometimes auto-complete is not enough (use additional entry attributes, create a new entry, use the pass generator, etc. all require the main window in front).

droidmonkey commented 8 years ago

Pleas edit your original request for the items you think are still necessary.

TheZ3ro commented 8 years ago

I've edited the main message and self-assigned this.

Old message for history purpose


Hi everybody,

That's great we the users of KeePassX have a new dev team for the app!

The latest KeePassX version before the fork lacked the following command-line parameters:

--toggle-window to assign it to a key combination (external to KPXR, e.g. like the Ubuntu Keyboard shortcuts) to bring the main window to front and to minimize it with the same key combination. As with --minimize, if the minimize-to-tray option is enabled, the minimize action of this arg should minimize the window to tray.

--minimize to either start the app minimized (if the minimize-to-tray option is enabled, this param should minimize the window to tray), or to minimize the current window via an external key combination. Update: There's an option in the latest commit to start minimized, this makes this option not so important anymore, though it would be useful to have it anyway for keyboard shortcuts.

--bring-to-front (optional) for bringing the main window to front from any state: no instance running and we start it with this param, it is minimized to taskbar or tray and we bring it back to front, again mainly for an external key combination.

To give this issues a higher priority, I'm willing to contribute economically. Those who could implement it please specify how much should be collected and I could contribute to the implementation on bountysource.com or your crowdfunding site of preference.

anatoli26 commented 8 years ago

Yepp, the edit by @TheZ3ro looks good, this is what is needed now (the original request was without knowing the advances of the reboot).

TheZ3ro commented 8 years ago

@Manko10 @droidmonkey

I was looking into this and I learned that Autotype register itself with the mac/windows/xcb plugins so we can't do it like autotype. Also autotype listen to the native eventfilter and emit a globalShortcutTriggered when the autotype shortcut is pressed.

For adding a new globalShortcut I've think of the following solution but IDK what fit the best:

  1. Add to the autotype eventfilter a check for the visibility shortcut and emit a different signal
  2. Change the eventfilter to emit together with globalShortcutTriggered the shortcut, add an object (GlobalShortcut) that listen to this signal and if there is a connected event with that shortcut emit the correct signal

Pro/Cons of the first:

Pro/Cons of the second:

What is the best solution in your opinion?

phoerious commented 8 years ago

Cross-platform global shortcuts are surprisingly hard to implement. If you ask me, I would probably go for the second option as it looks cleaner to me.

Unfortunately, Qt itself doesn't provide any way to register global hotkeys, though Qxt seems to have a class for it with QxtGlobalShortcut. This would probably be the easiest and cleanest way to implement global hotkeys. It would maybe even be possible to refactor the autotype code to use this class. However, I don't want to add a full new library just for global hotkeys, especially not in a critical application such as KeePassXC. We would probably need to get more out of Qxt than just global shortcuts to justify using it.

phoerious commented 8 years ago

Thinking about it more: I would definitely move the three different registerGlobalShortcut() and platformEventFilter() implementations to a separate class that is used by both autotype as well as any other hotkeys we may want to register.

TheZ3ro commented 8 years ago

QxtGlobalShortcut seems to be incompatible with Qt5.0

Thinking about it more: I would definitely move the three different registerGlobalShortcut() and platformEventFilter() implementations to a separate class that is used by both autotype as well as any other hotkeys we may want to register.

The problem with this is that we need to do the plugin loading in both autotype and other hotkeys.

I'm working on the second option right now to see if it's possible to do something good

phoerious commented 8 years ago

Can't we just load the plugin once and then use it for both?

TheZ3ro commented 8 years ago

Yes, I was trying to expose the m_plugin variable with a getPluginInterface function. The AutoType will load plugin and GlobalShortcut will access the plugin interface. Sounds good?

phoerious commented 8 years ago

I thin if we implement it, we should to it in the most extensible way we can. I would therefore maybe split the plugin and make a generic GlobalKeyboardActions interface which is implemented by platform-specific plugins. These plugins are loaded via an also generic plugin loader and used by AutoType as well as our global hotkeys which can therefore be completely separated.

TheZ3ro commented 8 years ago

This will be a big refactor. I don't know. We are diverging a lot from KeePassX

phoerious commented 8 years ago

I know. And it's a lot of work for just a simple feature. But the full autotype code is already super complex and adding more complexity in the form of basically unrelated functionality doesn't make the code any more readable. If we don't split up and simplify our code and define clear and generic interfaces, but simply stuff in more functionality, we will land in hell next time we have to add some other global hotkey stuff. or anything the like.

TheZ3ro commented 8 years ago

I'm down with that, if you want to start it I will push what I've done

phoerious commented 8 years ago

If you want to add this feature now and quickly, do it the way you suggested. But in the long term, I'm afraid we have to do the refactoring. I've had a deep look through the autotype code and a lot of things there still don't make sense to me. Adding some global hotkey feature there is certainly possible, but definitely not beautiful.

TheZ3ro commented 8 years ago

I totally agree with you. Maybe a look at this can help https://github.com/mitei/qglobalshortcut

So we need multiple plugin interface, or only 1 for global ?

phoerious commented 8 years ago

That looks cool, but it's missing the reverse direction, i.e., sending keystrokes. But maybe we can use it only for the hotkey part and remove that from the current autotype implementation. So leave the hotkey sending stuff as is, but register any global shortcuts (including the autotype shortcut) using QGlobalShortcut. That would already streamline and simplify a lot of things.

phoerious commented 8 years ago

Umm, QGlobalShortcut has one major downside: the Mac implementation:

#include "qglobalshortcut.h"
#include <QKeySequence>

quint32 QGlobalShortcut::toNativeKeycode(Qt::Key k) {
    // TODO
}

quint32 QGlobalShortcut::toNativeModifiers(Qt::KeyboardModifiers m) {
    // TODO
}

void QGlobalShortcut::registerKey(quint32 k, quint32 m) {
    // TODO
}

void QGlobalShortcut::unregisterKey(quint32 k, quint32 m) {
    // TODO
}

:unamused:

TheZ3ro commented 8 years ago

Yes, I'm porting some of that code but still using the autotype m_plugin (that have a MacOS implementation)

anatoli26 commented 8 years ago

My initial request for cli args was exactly to avoid complex solutions of global hotkeys implementations for the 3 OSes.

Maybe it should be reconsidered? I.e. implement both auto-type and toggle as cli args and then use the OS interface to invoke the app with necessary args for the desired hotkeys? On GNU/Linux it's one of the main ways of accomplishing the objective (maybe not the most elegant, but aligned with its KISS philosophy). On OS X there are also native global hotkeys for app/menu items.

On Windows I don't remember if there's something similar, there all apps tend to implement their own internal hotkey interfaces, so... (that's why I was proposing to drop its support, because of these OS-specific integration pitfalls ;)

Maybe implement cli args for *nix family and 2 additional options with hotkeys in the UI for windows?

colonelpanic8 commented 7 years ago

I would prefer a command line option (or at least would like a command line option in addition to the ability to add a keyboard shortcut), because command line options are much more flexible than keyboard shortcuts, and because I prefer to bind keybindings all in one place (in my WM).

TheZ3ro commented 7 years ago

Actually I think that implementing the command line option will be less trivial then the keyboard shortcut (that need cross-platform support)

droidmonkey commented 7 years ago

The autotype code to handle the global shortcuts should be extracted and moved into an independent "module" that can broker global shortcuts for arbitrary behaviors (pub-sub behavior). That way we don't have to recode handling the shortcut all the time.

colonelpanic8 commented 7 years ago

Actually I think that implementing the command line option will be less trivial then the keyboard shortcut (that need cross-platform support)

It may be more difficult (but thats not entirely clear to me -- I'm not an expert, but I'd expect that there are more cross platform issues with keyboard shortcuts than with command line flags, except that I suppose you might have to deal with inter process signaling with this particular command line flag), my argument is that it is much more flexible, and it avoids reinventing the wheel with keyboard shortcuts.

Having a command line flag allows raiseOrSpawn behavior AND it allows the shortcut to work even if keepass is not open. If the keyboard shortcut only exists in keepass itself, it will not work if it has not already been spawned.

arigit commented 7 years ago

Having a command line flag allows raiseOrSpawn behavior AND it allows the shortcut to work even if keepass is not open. If the keyboard shortcut only exists in keepass itself, it will not work if it has not already been spawned.

+1 on this. If keepassx could support a command line flag to --raise an existing instance from the tray or wherever (or open a new one if no running instance was found). This in combination with an --autotype option (and --keyfile as needed) would allow keepass to be invoked directly from a Desktop shortcut to perform autotype

e.g. in Gnome > Settings > Keyboard > Shortcuts, add Ctrl-Alt-A and bind it to

/usr/bin/keepassx --raise --autotype --keyfile mykeyfile

back in the day, the old keepass (no X) running on mono was able to do this when running on gnome, ages ago. keepassx never really matched this.

phoerious commented 7 years ago

That might actually also be a way for Wayland users to use autotype. Right now there is no solution for it. We'll have a look at it. 

droidmonkey commented 7 years ago

The solution proposed by @arigit is definitely the most elegant and would solve multiple issues including #14

TheZ3ro commented 7 years ago

Watch out this PR https://github.com/keepassx/keepassx/pull/171

colonelpanic8 commented 7 years ago

@TheZ3ro That pull request is on a different repository. Is there any chance that this will get merged any time soon?

phoerious commented 7 years ago

We can't make any statement about "when", but it's very likely that we will integrate that feature into one of the coming 2.x releases.

DominicWatson commented 7 years ago

fwiw, I manually merged that keepassx pull request (keepassx/keepassx#171) into a local clone of keepassxc. There were a bunch of conflicts but all simple to resolve (keeping both for the most part). The application compiled fine and it works a treat for me. I can now assign a WM keyboard shortcut to open up KeePassXC and then focus on the search input with no fuss:

#!/bin/bash
keepassxc
xdotool key --clearmodifiers ctrl+f

(keepassxc will always raise/focus an existing running instance, or start a new one if none running).

anatoli26 commented 7 years ago

@DominicWatson, could you please submit your changes as a PR here for review by the dev team?

droidmonkey commented 7 years ago

This has become overcome by the implementation of Single Instance in #510. I am opening a new issue with @arigit comment above about autotype command line argument.

anatoli26 commented 7 years ago

Mm.. from the discussion at https://github.com/keepassxreboot/keepassxc/pull/510 I thought this issue is not solved by the PR, is it?

droidmonkey commented 7 years ago

It is not, but this issue have become quite convoluted. If you can identify EXACTLY what needs to happen given the current state of the develop branch that would be far more useful (and put it in a new issue).

gaia commented 7 years ago

May I chime in, and I will open a new issue if necessary? I would like to raise it from the tray/taskbar using CTRL-ALT-K, same as I do with KeepassX on Windows (Integration>Show KeePass Window).

Spheerys commented 7 years ago

I'm also interested by this toggle function !

ccoenen commented 7 years ago

Ctrl+Alt+K is desperately needed here, too!

TheZ3ro commented 7 years ago

Right now we don't have plans to introduce this for the following points:

Maybe in the future we will reconsider this but for now we prefer focus on other issues

anatoli26 commented 7 years ago

@TheZ3ro, how do you do it with DE shortcuts? I know how to show (just assign keepassxc to a shortcut), but how to hide the window with the same shortcut, i.e. the toggle functionality?

solariz commented 6 years ago

Hey, because I totally used to work with a hotkey I created a small AutoIT script for windows to achieve the missing functionality. Feel free to use as a workaround.

(Windows Only, KeePassXC need to set to use tray icon)

https://gist.github.com/solariz/9fd8f0c58d15aaf3776e520ec82e5e83

Spheerys commented 6 years ago

Nice for Windows users !

Gittyperson commented 6 years ago

I usually overcome this issue by creating a direct shortcut to launching the program however, in this case KeePassXC will not come to front when minimised to tray, its icon will only appear in the taskbar. Launching KeePassXC yet another time will bring the program to front. This ability would be very helpful.

Thanks for that @solariz , will it work in AutoHotkey too?

solariz commented 6 years ago

@Gittyperson Autoit and AutoHotkey are two different script languages, so the principle will work but code need to be rewritten to work in AHK too.

sergeevabc commented 6 years ago

Everyone who cares about this feature, go and upvote here. Damn, so obvious we need the way to manipulate the app by means of global shortcuts.

phoerious commented 6 years ago

I'm adding this to the 2.4 milestone, but I can't promise anything. Would be nice to fix a double-digit issue, though.

phoerious commented 6 years ago

I see we closed it when we implemented single-instance mode. I agree that it'd need some extra love to work properly, though.

droidmonkey commented 5 years ago

Should also have the option to issue the "database unlock" window through this or another global shortcut.