Add per device support for key mappings #752

starsy · 2017-05-25T17:21:25Z

Hi Fumihiko San,

Hope you are doing fine.

Thanks for writing Karabiner such a great key mapping tool. I wrote some code to add per-device support for each key mappings for my own needs, e.g. this key mapping only works on keyboard A and that key mapping only works on keyboard B. The new feature works fine on my MBPR thus I would like to contribute this code back to the origin source to benefit more people having the similar needs. Also, it would be fantastic if you can review the code changes and let me know your comments. I would be more than happy to make further improvement accordingly.

Here is a screen shot for the configuration window:

There are many scenarios people needs this feature, especially when there is an external PC keyboard attached to the MacBook Pro and when they are using the two keyboards at the same time. E.g. 1) swap the left command/option on the external KB but leave the internal ones untouched. 2) map the right_option on the internal KB to forward_delete, but leave right_option on the external KB unchanged because there is a specific "delete" key on the external PC keyboard, etc.

There are certain points that you might need be aware of:

the config file name changed to karabiner2.json for its format incompatibility with the current one.
the format changes in the config file. Unfortunately the original format was too limited to make any extension, I have to define some new format for the new feature:

"simple_modifications": [
                {
                    "disabled": false,
                    "from": "left_option",
                    "product_id": 1957,
                    "to": "left_command",
                    "vendor_id": 1118
                },
                ...
}

there are data structure and interface changes in class simple_modifications and other classes, which makes many unit test cases compilation fail or execution fail. I am still working on it but it takes time to fix all of them, so I removed "make -C test" from .travis.yml temporarily to make the CI build pass.
created a new device_grabber.cpp and split the implementation of class device_grabber into it.

Other changes should be straightforward by reading the code diff.

Anyway, appreciate you can review the code, let me know your comments, and I would be extremely happy if you found these changes are useful and would like to accept them into the base.

Cheers,
Yang

device control

carrigmore · 2017-05-26T11:22:15Z

@starsy this feature looks great. Would your change also fix the issue #719 ? thanks a lot

starsy · 2017-05-27T05:23:22Z

Hi @carrigmore Thanks for your comments. However sorry my change will not fix the problem that you raised.

There is a "not_to = true" attribute on "locking_num_lock" in simple_modifications.json. I think @tekezo may have some reason to put it there and he can explain. But before that, would you please try remove that attribute from the json file and see whether the newly defined mapping works?

As none of my keyboards has num_lock keys, sorry I would not be able to test your scenario on my side.

support identifying available devices and their names on-the-fly

perlun · 2017-07-13T12:52:01Z

Thanks @starsy. Could you rebase this code on top of the latest master branch from @tekezo? (That will also get rid of all the superfluous merge commits)

starsy · 2017-07-23T11:48:10Z

@perlun I would like to do the rebase, but as @tekezo is not showing his opinion on accepting the functionality and meanwhile KBE is being developed in very fast pace, the rebased code will expire very soon, so i decide to follow it instead of create another pull request or so.

I put some thoughts in another thread: #792
as my guess on why @tekezo is not responding to the request, now i copy them here to explain what I think as the reason.

Yes, i opened a pull request quite long ago, @tekezo didn't respond to it. I think there could be several reasons, as @tekezo not sending any words, this is my pure guess:

The change itself is big in terms both code size and scope. There are several thousands of lines of change, from PreferenceWindow to KarabinerKit to libkrb to device_grabber to console_user_server, from ObjectiveC to C to C++, the only thing not touched is the kernel module. It is difficult to review so much code change at one time and to merge w/o a throughout review is risky and not responsible for the large KBE user groups.
The configuration file format requires a change which introduce a backward compatibility issue to the wide user groups again. The initial KBE's config file has no per device key mapping support in design -- this is quite strange because it is there in the old KB feature set. I have to change the config file format in a non-backward compatible way to support it, hmmm @tekezo i would like to blame this to you. 😞
@tekezo maintains a very nice suite of unit test cases for his code as quality guarantee. But due to a wide spectrum of changes, especially the changes in device_grabber and its low level dependencies, many of the C/C++ function interfaces has been changed, which breaks the unit test suite. i haven't decided to update them due to the workload and the necessity. But if @tekezo is willing to accept the pull request, i would be glad to extend the change to unit test suite.
Code style. when you look deep into the code, @tekezo has his very own taste in how to write C++ code. the whole device_grabber and user_console_server is composed purely by .hpp, which means both the class/function definitions and implementations are put in header files. this is definitely debatable with certain limitations like long build time -- not be able to do parallel compiling, the cross-reference class types across header files is cumbersome or impossible. Thus I introduced a few new .cpp files to address these challenges, however it apparently broke @tekezo 's code style purity and paradigms.
KBE is obviously @tekezo 's own baby when looking at the contribution history. There is no other significant contributors to the project, @tekezo may just want to keep his total ownership on this, which is understandable.

So I have created this fork as https://github.com/starsy/Karabiner-Elements , the "master" branch is for stable release and the "follow" branch keeps tracking and following KBE original's change, in case one day @tekezo would like to accept it, it can be done quickly. The stable release and pre-built packages can be found in "Release" section. Once the recently added complex configuration in KBE original is stabilized, I would like to review the difficulty and necessity to extend this highly demanded feature to it.

perlun · 2017-07-24T11:32:56Z

Thanks for the explanation @starsy. To avoid confusion, could you rename your fork to Starsy-KBE or something more elegant you can come up with? Just to make it clear that it diverges from the origianl project and is "not intended to merge", at least not in the short term.

(v0.91.11)

tekezo · 2017-07-31T02:12:24Z

Thank you for your fork!

I'm working on importing the changes.
First, I've added device_if to complex_modifications > conditions.
As you know, the simple_modifications also uses the basic manipulator.
I'll change to use device_if in simple_modifications and fn_function_keys.

starsy · 2017-07-31T03:34:05Z

Thanks @tekezo for KBE such great tool!

Really glad to see you are adding this highly demand feature to make KBE even greater! As you are personally working on it now, I am going to close this pull request. But in case you need any help, please don't hesitate to let me know.

Turns out that simple_modifications does the right thing for us here but complex_modifications does not. Specifically, the latter produces different events in the following cases: - Shift down, Command down (produces Shift down, Command down). - Command down, Shift down (produces Option down, Shift down). And: - Shift down, Option down (produces Shift down, Option down). - Option down, Shift down (produces Command down, Shift down). In other words, the keys get swapped only if they are pressed alone. I believe `device_if` support is coming to `simple_modifications` soon (see pqrs-org/Karabiner-Elements#752 (comment)) so I am just going to switch to that and wait. If it doesn't come soon I'll see if I can come up with a more verbose complex_modifications equivalent to what simple_modifications is doing.

petr-ujezdsky · 2017-08-01T19:47:54Z

@starsy really, really great work! This is actually the core feature I need from this program. Without it is kinda useless, applying rebinds to ALL keyboards does not make sense to me at all. Unless your keyboards are all the same :)
Thanks @tekezo for all your time and effort incorporating this functionality into main branch.

tekezo · 2017-08-12T16:49:25Z

Karabiner-Elements v0.91.13 supports per device settings in Simple Modifications and Fn Function Keys.

Note:
The json format is changed from this PR for compatibility.

starsy added 14 commits May 12, 2017 21:09

hard code for internal/external keyboard separation

28c1d79

refractor passing hid object to manipulators

516e1c3

Add key mapping for SimpleModification and support vendor/product Id

926cac9

be able to configure from App and apply config in grabber to enforce per

63ed871

device control

remove dead code

47c7944

remove unnecessary .cpp file

d930786

support F1-F12 key under new key mapping configuration format

5f1348a

enable f1-f12 key mapping

35759a4

Support show device product name in PreferenceWindow for key mappings

b7f014b

Add screen shot and example config file: karabiner2.json

4f7deb0

Update README.md

55376c3

ignore make test temperarily to pass travis build

11ba5bc

Merge branch 'master' of https://github.com/starsy/Karabiner-Elements

ad0fc1f

find device by vendor id and product id

d99db79

starsy added 3 commits May 27, 2017 18:03

Merge https://github.com/tekezo/Karabiner-Elements into follow

326bed0

add vendor/product check back

aa7ea26

add back key mappings which is lost during merge

da9bfd7

starsy mentioned this pull request May 27, 2017

Add per device support for key mappings #755

Closed

remove commented code

6c6eb77

starsy mentioned this pull request May 27, 2017

Add per device support for key mappings (code merged w/ latest) #756

Closed

starsy added 8 commits May 31, 2017 10:29

Merge https://github.com/tekezo/Karabiner-Elements into follow

1302dca

Merge https://github.com/tekezo/Karabiner-Elements into follow

ddcfd90

remove built executables

4a8f4a1

Merge https://github.com/tekezo/Karabiner-Elements into follow

88d0563

Merge https://github.com/tekezo/Karabiner-Elements into follow

3af64a9

Update README.md

2d01795

Update README.md

fa3feb4

Update README.md

4cec499

starsy added 13 commits June 15, 2017 17:57

Merge https://github.com/tekezo/Karabiner-Elements into follow

85fff94

Merge https://github.com/tekezo/Karabiner-Elements into follow

b44de40

Merge branch 'master' into follow

0f6e8cc

Merge tag 'v1.1' into follow

8be828b

support identifying available devices and their names on-the-fly

Merge https://github.com/tekezo/Karabiner-Elements into follow

a85e4ab

Merge https://github.com/tekezo/Karabiner-Elements into follow

a92c8e0

Merge branch 'follow'

08d6337

Merge remote-tracking branch 'origin' into follow

ef15dad

Merge https://github.com/tekezo/Karabiner-Elements into follow

b31e432

Merge https://github.com/tekezo/Karabiner-Elements into follow

f647867

keep following

b2ace74

Merge https://github.com/tekezo/Karabiner-Elements into follow

5bb4a88

Merge https://github.com/tekezo/Karabiner-Elements into follow

1d8fcc7

Merge https://github.com/tekezo/Karabiner-Elements into follow

cdae32e

starsy added 7 commits July 27, 2017 23:37

Merge https://github.com/tekezo/Karabiner-Elements into follow (0.91.10)

4ce4095

surpress error messages in log for keyboard+mouse devices

6f8690e

disable update feature and change url

35bb299

resolve conflict

c0a6ab1

Merge remote-tracking branch 'origin/follow' 0.91.10-starsy

bc1171e

Merge https://github.com/tekezo/Karabiner-Elements into follow

40d3e46

(v0.91.11)

Merge remote-tracking branch 'origin/follow'

318e974

starsy closed this Jul 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Add per device support for key mappings #752

Add per device support for key mappings #752

starsy commented May 25, 2017 •

edited

Loading

Uh oh!

carrigmore commented May 26, 2017

Uh oh!

starsy commented May 27, 2017 •

edited

Loading

Uh oh!

perlun commented Jul 13, 2017

Uh oh!

starsy commented Jul 23, 2017 •

edited

Loading

Uh oh!

perlun commented Jul 24, 2017

Uh oh!

tekezo commented Jul 31, 2017 •

edited

Loading

Uh oh!

starsy commented Jul 31, 2017

Uh oh!

petr-ujezdsky commented Aug 1, 2017

Uh oh!

tekezo commented Aug 12, 2017 •

edited

Loading

Uh oh!

Add per device support for key mappings #752

Add per device support for key mappings #752

Conversation

starsy commented May 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carrigmore commented May 26, 2017

Uh oh!

starsy commented May 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

perlun commented Jul 13, 2017

Uh oh!

starsy commented Jul 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

perlun commented Jul 24, 2017

Uh oh!

tekezo commented Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

starsy commented Jul 31, 2017

Uh oh!

petr-ujezdsky commented Aug 1, 2017

Uh oh!

tekezo commented Aug 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

starsy commented May 25, 2017 •

edited

Loading

starsy commented May 27, 2017 •

edited

Loading

starsy commented Jul 23, 2017 •

edited

Loading

tekezo commented Jul 31, 2017 •

edited

Loading

tekezo commented Aug 12, 2017 •

edited

Loading