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

Enhance keySyms #113

Merged
merged 3 commits into from
Nov 10, 2015
Merged

Enhance keySyms #113

merged 3 commits into from
Nov 10, 2015

Conversation

aurium
Copy link
Contributor

@aurium aurium commented Oct 31, 2015

  • Direct translate from X.Org's xproto/keysymdef.h
  • Adds the key description as JSON data
  • Update examples where the x11.keySyms API changes

@@ -0,0 +1,35 @@
#!/bin/bash

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add set -e

@santigimeno
Copy link
Collaborator

Thanks! The code looks good but it's a major version and I don't know that's desirable. To keep compatibility the description could be inserted as comments

@santigimeno
Copy link
Collaborator

I mean a major version change

@aurium
Copy link
Contributor Author

aurium commented Oct 31, 2015

Thanks! I will update it now.
About the char description at keysyms.js, that is my desired feature. I will also push a "pedagogic" example about it keysyms displaying the description. :-)

Aurélio A. Heckert added 2 commits October 31, 2015 17:37
* Direct translate from X.Org's xproto/keysymdef.h
* Adds the key description as JSON data
* Update examples where the `x11.keySyms` API changes
@santigimeno
Copy link
Collaborator

Oh I see. I understand the reason to store the description for pedagogic reasons, but does it make sense from a logical/API POV?
Anyway, if we would want to keep backwards compatibility, you could store the description in a different object with the same keys.

@aurium
Copy link
Contributor Author

aurium commented Oct 31, 2015

Well, i believe that information can be useful for other things, like sys testing, sys configuration... Imagine a keyboard config gui... You can set chars for each key and the description will help the user to be sure about what they are doing. Well... there is an information. We can easily provide to the user. Lets give it and open more possibilities to the public. For sure my ideas are a small set of the human mindshare. :-p

Separate the data will use more memory and will make the translation a kind bizarre... The API change is simple as you can see on the example updates that i did, but that is true. That is why i try to push a new version on my first push. But you should decide about the numbering. :-)

@aurium
Copy link
Contributor Author

aurium commented Oct 31, 2015

Hey, a cool usage: You have a game with configurable inputs. The config UI can show the key description when it is not a simple letter.

@santigimeno
Copy link
Collaborator

Yes, I can see the usefulness now. Thanks :)

Yes, I understand that trying to keep the current API might not be nice nor optimal but not breaking the API can be good for the users of the module. I'm not totally against it. Let's see what @sidorares think.

@sidorares
Copy link
Owner

I'm probably ok with major version bump ( it's very trivial to upgrade for those who depend on x11.keySyms ). At the same time I'm still thinking if backwards-compatible solution would be better, for example add "descriptions" hash alongside to keysyms:

 var keySyms = x11.keySyms;
 for (codeName in x11.keySyms) {
   code = keySyms[codeName];
   chr2Data[code] = { codeName: codeName, description: keysSyms.description[codeName] };
 }

@aurium
Copy link
Contributor Author

aurium commented Nov 1, 2015

Guys, how about to make x11.keySyms a getter? So the keysyms.js still a direct translation with description and x11.keySyms do filter.

I still prefer the API change, while the world domination is not accomplished, because any solution to maintain backwards compatibility, will add some extra complexity to the code.

However, i'm not here to decide. :-)

@sidorares
Copy link
Owner

getter is actually a good idea.
constants file is pretty large so we could lazily require it on first access which would improve startup time and memory

@aurium
Copy link
Contributor Author

aurium commented Nov 1, 2015

Lazily load is a great idea! I will add this feature.
And how about to break the API? Are you ok with it?

@sidorares
Copy link
Owner

I'm ok with api breakage, it's pretty minor ( but we'll bump major version as its major in semver sense )

@aurium
Copy link
Contributor Author

aurium commented Nov 2, 2015

✨ Done! ✨

white = display.screen[0].white_pixel; // Will paint the window.

// Get the local key mapping to build key2Data.
X.GetKeyboardMapping(min, max-min, function(err, list) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

throw on error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talking about if (err) throw err, or other line?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Sorry if it's not clear. I mean in the callback of X.GetKeyboardMapping

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ow ok... I was worried, thinking there was a bug. As it is a simple example, if the main data provider fail, there is no why to continue. So throw it.

@santigimeno
Copy link
Collaborator

One comment and it looks good to me

sidorares added a commit that referenced this pull request Nov 10, 2015
@sidorares sidorares merged commit b39672b into sidorares:master Nov 10, 2015
@sidorares
Copy link
Owner

hey @aurium

I started looking at what actually happens when you change language layout ( for example, en->ru )

Few problems/suggestions here:

  1. X Keysym is pretty useless if we need to type text. I suggest that we completely remove keysyms.js file and use https://github.com/substack/node-keysym instead ( because it has corresponding unicode codepoinds )
  2. Simple input is relatively easy to track: we listen for MappingNotify and remember new mapping each time. Every key correspond to some symbol
  3. Unfortunately CJK input ( and other complex input methods ) don't have that 1-1 mapping and there needs more work to cover this ( and I don't know yet what is involved )

@aurium
Copy link
Contributor Author

aurium commented Nov 17, 2015

Hi @sidorares, I'm auto writing portuguese text with keysyms.js. The "char table" let us to know the key combination for any char, so i can write "Áçõ" and any crazy latin char.
Is not it the same case for RU?
I believe node-keysym is a nice lib for this approach, however, i also believe node-x11 must to provide all x11 features. Well... I may publish a keysyms helper to simplify the usage.

Anyway, the decision is yours. What you think about this proposal?

@sidorares
Copy link
Owner

That's because latin1 keysyms are the same as corresponding unicode code points:

 keysyms.fromUnicode('Á')
[ { keysym: 193, unicode: 193, status: '.', names: [ 'Aacute' ] } ]
> keysyms.fromUnicode('ф')
[ { keysym: 1734,
    unicode: 1092,
    status: '.',
    names: [ 'Cyrillic_ef' ] } ]

unfortunately not the case for codes outside of 0-255 range, X11 keysyms were created long before Unicode table and are different, so there is a need for translation table keysym -> unicode code point

@sidorares
Copy link
Owner

some future read for me: http://www.x.org/releases/X11R7.7/doc/libX11/XIM/xim.txt

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

Successfully merging this pull request may close these issues.

3 participants