g910-gkey-macro-support icon indicating copy to clipboard operation
g910-gkey-macro-support copied to clipboard

reduce complexity/cleanup of gkey_functionality

Open voglster opened this issue 5 years ago • 3 comments

Had some time, saw some formatting issues and the code felt overly complex with the inspect, refactored a bit to simplify the handling of gkeys to make it a little more explicit.

Note! As of right now, this is untested! Before I went too far in refactoring/cleanup I wanted to make sure you actually like it.

voglster avatar Feb 25 '20 06:02 voglster

Hey, I looked through the code a bit and I like the aesthetic changes (white space and all that) but don't like the functionality change (like removing hotkey_type.py or changing emitKeys(device, key)). These thing were coded this way so that an expert user can easily inject additional code on just one GKey. File hotkey_type.py is there so everyone can see exactly what kind of hotkey strings are used (ex. wiki/docs include a wrongly typed hotkey type, user gets errors, user looks into hotkey_type.py and see exactly what kind of string he needs to use).

Thank you for your work on the driver,

Jan

JSubelj avatar Feb 25 '20 07:02 JSubelj

hotkey_type.py is literally copied straight from hotkey_types which you already had in supported_configs.py. Hence why I removed the file. It is duplicated, and duplicated logic makes people double-check which one is correct.

ex:

command = key_config.get("hotkey_type", supported_configs.default_hotkey_type) if command not in supported_configs.hotkey_types:

Now you can add new functionality later without touching multiple files and there is only one source of truth for the hotkey types.

As for the replacement of emitKeys and change to handle_gkey_press, I modeled it off of your existing code in media_static_keys_functionality.py. Take a look at resolve_key it has the exact same signature (device,key). My changes also reduce the number of lines of code and cyclomatic complexity. I find it more explicit/easier to read, but I understand that's also my opinion and it's your codebase. I was only trying to help.

In either case if you don't like it feel free to close to PR then :). Thanks again for starting this I very much appreciate you making it public!

voglster avatar Feb 26 '20 04:02 voglster

Hey, thank you for your answer. You're definitely right about hotkey_type.py. About handle_gkey_press I can see your view on it. I would put the valid_gkeys list in supported_configs.

If you test the code out to see if it works as expected I will merge your pull request.

If you'll be doing some additional work on the driver, take a look at https://github.com/JSubelj/g910-gkey-macro-support/issues/13. It should be easy to add, I just haven't found the time to do it myself.

JSubelj avatar Feb 28 '20 10:02 JSubelj

Thank you @voglster I also like your work and merged it into my dev-0.2.x branch. There was only one little mistake on the call of handle_gkey_press(). You mixed up the parameters device and key. But no worries i fixed it already after i merged and tested it.

I wanna check the other pull requests before i make a pull request my self.

suabo avatar Oct 31 '22 00:10 suabo