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 g-key support for use in global shortcuts #1730

Closed
wants to merge 13 commits into from

Conversation

@forivall
Copy link
Contributor

@forivall forivall commented Jun 28, 2015

Fixes #1721

There are still some todos for this before it should be merged:

  1. The location of the dll needs to be loaded by looking at the registry, instead of how it is currently hardcoded see checklist.
  2. code review plz! I modeled it after CELTCodec.cpp/h. "LGTM"
  3. If somebody has a logitech mouse with billions of buttons, it would be nice if they could test this (G600, G300, G400, G700, G9/G9x, MX518, G402, G502, or G602). Note that this is only for the extra buttons, higher than button 5.
  4. should the QList<QVariant> format for passing around buttons be made uniform, so that the first element is always a signifier of if it's a dinput or a gkey? or should i create a uuid for gkeys, and keep the list length at 2? Create a uuid for gkeys and gmousebuttons

And finally, the instructions to the user, that will need to be placed somewhere on the wiki: This requires Logitech Gaming Software version 8.55+, and they need to set the mumble profile to "Set as Default" or "Set as Persistent" to allow the keys to work while mumble is in the background. Included in pr.


  • Rename files and class
  • Include Spec and doc in .cpp file
  • add hidden config option
  • Create guid for gkey key and gkey mousebutton
  • update reporting to use guid
  • query registry for dll location
  • update build settings
  • Also add user documentation to the wiki
forivall added 2 commits Jun 26, 2015
This commit was created by squashing the following commits
ad28c29 Change globalshortcut_win_gkey to gkey
886f356 Prepared header with api specification
8872ec2 Stub out gkey handler
I don't have a logitech mouse with gkeys, so someone else needs to test that
@forivall forivall changed the title Adds g-key support for use as push to talk and other shortcut keys Add g-key support for use in global shortcuts Jun 28, 2015
@@ -557,12 +566,53 @@ void GlobalShortcutWin::timeTicked() {
handleButton(ql, rgdod[j].dwData & 0x80);
}
}
#ifdef USE_GKEYS
if (gkey->isValid()) {
for (int button = GKEY_MIN_MOUSE_BUTTON; button <= GKEY_MAX_MOUSE_BUTTON; button++) {

This comment has been minimized.

@mkrautz

mkrautz Jun 28, 2015
Member

Indentation.

This comment has been minimized.

@forivall

forivall Jun 28, 2015
Author Contributor

Wierd, my editor switched to spaces for those lines. Fixed. Should I just squash in the indentation fix?

ql << 0; // 3 entries in the list means gkey; add '0' as a dummy
handleButton(ql, gkey->isMouseButtonPressed(button));
}
for (int mode = GKEY_MIN_KEYBOARD_MODE; mode <= GKEY_MAX_KEYBOARD_MODE; mode++) {

This comment has been minimized.

@mkrautz

mkrautz Jun 28, 2015
Member

Indentation.

@mkrautz
Copy link
Member

@mkrautz mkrautz commented Jun 28, 2015

I'd like if this was using a GUID for G-keys instead. Fits the GlobalShortcut_win architecture better.

I'd like it if you added a comment at the top of Gkeys.cpp saying: "The code interfacing with the Logitech G-Keys DLL was implemented using the following spec:\n\n"

Let's rename the file from Gkeys.{cpp,h} to GKeys.{cpp,h}. Very nitpicky.

I think we might as well enable G-keys support by default.
You can easily do that by having a !CONFIG(no-gkeys) { CONFIG *= gkeys } (with correct indentation) just above your CONFIG(gkeys) section.

Code LGTM.... If you implement my spec, registry querying and GUIDs for G-Ke-Keyboard and G-Key-Mouse, I think it's ready to go in.

@mkrautz
Copy link
Member

@mkrautz mkrautz commented Jun 28, 2015

I guess also means I prefer the class to be called GKeyLibrary. :-)

@mkrautz
Copy link
Member

@mkrautz mkrautz commented Jun 28, 2015

Oh, and the !CONFIG(no-gkeys) would probably need to go after CONFIG(gkeys)...

@mkrautz
Copy link
Member

@mkrautz mkrautz commented Jun 28, 2015

Also, I would like to ask you to include a config option (no UI, just a config option) for disabling G-keys suport.

You can base it on 18c359f

I think it should just be called "shortcut/gkey" or something. I don't think we need to put it under "shortcut/win/gkey" since it is not OS-vendor specific. We could grow an implementation for other OSes in the future, so "shortcut/gkey" seems fine.

It should be enabled by default.

In GlobalShortcut_win.cpp, you can then just do (g.s.bEnableGKey) { gkeys = new GKeyLibrary(); }, and gate the polling inside the ifdefs with a similar check.

This allows us to have users apply a registry hotfix to disable G-Key support in case it causes performance problems, hangs or crashes in the future.

@forivall
Copy link
Contributor Author

@forivall forivall commented Jun 28, 2015

Awesome! should I just create a guid with uuidgen, or is there some other way (hashing something?).

Should I include the full text of the spec in the comment, or just a link?

I would anticipate that the only other os that this would be needed for is OSX. (Since Linux users have to use custom drivers 😓)

I'll do all of this sometime tomorrow (PDT). It's late.

@mkrautz
Copy link
Member

@mkrautz mkrautz commented Jun 28, 2015

Just create a v4 GUID somewhere. I use python: import uuid; uuid.uuid4().

Spec: just include a line stating you implemented it using the spec below, and include it verbatim inside the source file.

@mkrautz
Copy link
Member

@mkrautz mkrautz commented Jun 28, 2015

You might also want to add a small doc snipped inside the .cpp file describing how to setup Logitech Gaming Software to interact with Mumble.

You are right that it probably should live in the Wiki for end-users to see, but I think it would be nice to have in the code as well.

@forivall
Copy link
Contributor Author

@forivall forivall commented Jun 29, 2015

And done!

QLibrary qlLogiGkey;
bool bValid;

bool (*LogiGkeyInit)(void *);

This comment has been minimized.

@mkrautz

mkrautz Jun 29, 2015
Member

BOOL as per the spec.


bool (*LogiGkeyInit)(void *);
void (*LogiGkeyShutdown)();
bool (*LogiGkeyIsMouseButtonPressed)(int button);

This comment has been minimized.

@mkrautz

mkrautz Jun 29, 2015
Member

BOOL as per the spec.

bool (*LogiGkeyInit)(void *);
void (*LogiGkeyShutdown)();
bool (*LogiGkeyIsMouseButtonPressed)(int button);
bool (*LogiGkeyIsKeyboardGkeyPressed)(int key, int mode);

This comment has been minimized.

@mkrautz

mkrautz Jun 29, 2015
Member

BOOL as per the spec.

qWarning("GKeyLibrary: Error looking up ServerBinary (Error: 0x%x, Type: 0x%x, len: %d)", err, type, len);
}
} else {
qWarning("GKeyLibrary: Could not open lib location regkey (Error: 0x%x)", errOpen);

This comment has been minimized.

@mkrautz

mkrautz Jun 29, 2015
Member

Probably drop this warning. It'll be a lot of noise for people without LGS.

@mkrautz
Copy link
Member

@mkrautz mkrautz commented Jun 29, 2015

Looks pretty good now!

I know I suggested GKeys.cpp and GKeys.h, but would it be more correct for it to be GKey.cpp and GKey.h? And USE_GKEY? It fits better with the class name, DLL function names and so on. If it's not too burdensome, I think we should change it to just be GKey all around.

#ifndef MUMBLE_MUMBLE_GKEY_H
#define MUMBLE_MUMBLE_GKEY_H

// #include <stdint.h>

This comment has been minimized.

@mkrautz

mkrautz Jun 29, 2015
Member

Drop if not needed?

DWORD len = 255;
LONG errOpen = RegOpenKeyExA(GKEY_LOGITECH_DLL_REG_HKEY, GKEY_LOGITECH_DLL_REG_PATH, NULL, KEY_READ, &key);
if (errOpen == ERROR_SUCCESS) {
// XXX: Should the unicode variants be used instead?

This comment has been minimized.

@mkrautz

mkrautz Jun 29, 2015
Member

Yes, we simply use RegOpenKeyEx, RegQueryValueEx, etc. in most places in the rest of the codebase.

forivall added 2 commits Jun 30, 2015
Use utf16 for registry query
Remove unused comments and code
Use BOOL instead of bool as defined by spec
Drop extra warning
@forivall
Copy link
Contributor Author

@forivall forivall commented Jun 30, 2015

Yeah, I'm fine with renaming stuff (except that when I changed "USE_GKEYS" to "USE_GKEY", I had do do a clean build cuz the pch had the old define. 😧) I also changed the qmake flag from gkeys to gkey, for more consistency. All references now use "GKey" instead of "GKeys", except for one place in the comments, where it's grammatically using the plural form.

@mkrautz
Copy link
Member

@mkrautz mkrautz commented Jun 30, 2015

Looks good! One thing: I can't land this with the current license text.

You've changed "All rights reserved" to "Some rights reserved", which I myself don't particularly mind.

But the "All rights reserved" text is part of the 3-clause BSD license, whether we like it or not. So it ought to stay.

I would have done this myself, but since your modified BSD-license text contains "Redistributions of source code must retain the above copyright notice, this list of conditions and the following disclaimer.", I cannot make that change myself, as I would breach the license.

So in order for me to be able to land this, please revert it back to the default 3-clause BSD license used elsewhere in Mumble.

Thanks!

@forivall
Copy link
Contributor Author

@forivall forivall commented Jun 30, 2015

Sorry about that. I usually just slap a LICENSE file at the base of my project, and then not really pay attention to the individual file header license text.

@mkrautz
Copy link
Member

@mkrautz mkrautz commented Jun 30, 2015

Marking as ready-to-land. LGTM.

I will land once I am able to.

@mkrautz
Copy link
Member

@mkrautz mkrautz commented Jul 14, 2015

Landed as 6c096c3.

Very sorry for the long wait. :-(

@mkrautz mkrautz closed this Jul 14, 2015
@mkrautz
Copy link
Member

@mkrautz mkrautz commented Jul 14, 2015

Oh, and just to add: Thank you very much, this is pretty cool :-)

@jkluch
Copy link

@jkluch jkluch commented Jul 14, 2015

Any idea when the build with this addition be released? and If I want to build this myself do I have to use the gkey flag? I want to test this on my G500 and G710+

@mkrautz
Copy link
Member

@mkrautz mkrautz commented Jul 14, 2015

I will do a snapshot soon.

@mkrautz
Copy link
Member

@mkrautz mkrautz commented Jul 14, 2015

Triggered a snapshot... It's building!

@pyrojoe The gkey CONFIG option is always on, unless disabled with CONFIG-=no-gkey.

@jkluch
Copy link

@jkluch jkluch commented Jul 15, 2015

Thanks @mkrautz I saw the snapshot "mumble-1.3.0690ge614a7b~snapshot"

I've tested it and I've got it working on both my keyboard (G710+) and mouse (G500 predecessor to the G502). I had to reinstall LGS for it to work on both my machines. The downside of this implementation is mumble only reacts to that key while the mumble profile is active (either when mumble is in focus or if you have that profile set as persistent)

This means the mumble profile is the only profile you can use, if you have profiles with bindings for games you can't use them. So while this sounded like a useful feature on paper it's only useful to those who use one profile across all their applications anyway.

It would also be a viable solution if the G-keys could be assigned in all profiles. Then I could just have my mouse button set to the G-key for all profiles and not have to worry about it. But as far as I can tell you can only assign the G-key for programs that support it, and only one program can use a G-key at a time.

To be sure, is my understanding correct @forivall ? If it is I'm trying to figure out the benefit of this addition. I didn't have much familiarity with G-Keys so wasn't expecting the use case to be so narrow =/

@forivall
Copy link
Contributor Author

@forivall forivall commented Jul 15, 2015

@pyrojoe Yes, unfortunately, that is correct. That's just the way that the LGS works and the way that the G-key API works, and the G-key API is the only way to listen directly to g-keys. I wish that Logitech would allow a way to globally listen to g-keypresses, without requiring a profile. The only other ways to interact with the G-keys are by using the LGS GUI or embedded Lua, which don't lend well to direct integration with mumble. The third solution would be to use the Lua scripting to simulate F13-F18, and fix #987. However, that's a harder bug, and one that I'm not equipped to try to fix 😞.

This behaviour is documented in the code (link), and will be updated in the wiki. I was just waiting for this pr to land before updating the wiki; didn't want to document something that hasn't landed yet. I'll also add screenshots 🎉.

One thing that you may have missed is that you can also set the mumble profile as the default, so if you have a specific game profile, it will still activate. Personally, I really only use one profile anyways.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.