gtk3: Call XIGrabKeycode directly from Vala

gtk3: Call XIGrabKeycode directly from Vala

Port the logic of grabkeycode.c into Vala.  This will make it easier to
implement XI2 GenericEvent handling in KeybindingManager.

BUG=none

[Daiki Ueno, 2012-09-04 03:26:20]

Another refactoring patch.

BTW, I'm a bit curious of the code which shows switcher dialog
(ui/gtk3/panel.vala:handle_engine_switch), where it calls
KeybindingManager.primary_modifier_still_pressed().

It is for detecting the first key repeat, right?  If so, maybe XIKeyRepeat flag
can be used and it would allow unmodified accelerator key.

This patch will help such an extension.

[Peng, 2012-09-04 13:58:06]

https://codereview.appspot.com/6501080/diff/5001/bindings/vala/Makefile.am
File bindings/vala/Makefile.am (right):

https://codereview.appspot.com/6501080/diff/5001/bindings/vala/Makefile.am#ne...
bindings/vala/Makefile.am:47: config.vapi \
I also think maybe Config.vapi is better. It make all file names begin with big
letter.

https://codereview.appspot.com/6501080/diff/5001/bindings/vala/Makefile.am#ne...
bindings/vala/Makefile.am:48: external-libs.vapi \
Maybe name XI.vapi is better.

https://codereview.appspot.com/6501080/diff/5001/bindings/vala/external-libs....
File bindings/vala/external-libs.vapi (right):

https://codereview.appspot.com/6501080/diff/5001/bindings/vala/external-libs....
bindings/vala/external-libs.vapi:1: [CCode (cprefix = "", lower_case_cprefix =
"", cheader_filename = "X11/extensions/XInput2.h")]
If we can upstream it to vala or XI, it would be better.

https://codereview.appspot.com/6501080/diff/5001/ui/gtk3/keybindingmanager.vala
File ui/gtk3/keybindingmanager.vala (right):

https://codereview.appspot.com/6501080/diff/5001/ui/gtk3/keybindingmanager.va...
ui/gtk3/keybindingmanager.vala:217: uint mask = 0;
Why not mask = X.KeyMask.LockMask | X.KeyMask.Mod2Mask | ...?

https://codereview.appspot.com/6501080/diff/5001/ui/gtk3/keybindingmanager.va...
ui/gtk3/keybindingmanager.vala:242: }
I think this logic is a little difficult to understand. I pref just list all
possible combinations here. Like the code .c file.
 83     XIGrabModifiers ximodifiers[] = {	
  84         {modifiers, 0},	
  85         {Mod2Mask | modifiers, 0},	
  86         {LockMask | modifiers, 0},	
  87         {Mod5Mask | modifiers, 0},	
  88         {Mod2Mask | LockMask | modifiers, 0},	
  89         {Mod2Mask | Mod5Mask | modifiers, 0},	
  90         {LockMask | Mod5Mask | modifiers, 0},	
  91         {Mod2Mask | LockMask | Mod5Mask | modifiers, 0},	
  92     };

Or you may simplify the logic as:

int[] masks;

foreach (var m in ignored_modifiers) {
   masks += [m];
   for (int i = 0; i < masks.length - 1; i++) {
       masks += [masks[i] | m];
   }
}

[Peng, 2012-09-04 14:01:06]

On 2012/09/04 03:26:20, Daiki Ueno wrote:
> Another refactoring patch.
> 
> BTW, I'm a bit curious of the code which shows switcher dialog
> (ui/gtk3/panel.vala:handle_engine_switch), where it calls
> KeybindingManager.primary_modifier_still_pressed().

I remember it is for some cases: Users press and release the hotkey very
quickly. So when we process the hotkey event, the user already released the
modifier keys. In that case, we need just switch engine, and do not need show
the switcher UI.


> 
> It is for detecting the first key repeat, right?  If so, maybe XIKeyRepeat
flag
> can be used and it would allow unmodified accelerator key.
> 
> This patch will help such an extension.

[Daiki Ueno, 2012-09-04 14:44:53]

https://codereview.appspot.com/6501080/diff/5001/bindings/vala/Makefile.am
File bindings/vala/Makefile.am (right):

https://codereview.appspot.com/6501080/diff/5001/bindings/vala/Makefile.am#ne...
bindings/vala/Makefile.am:47: config.vapi \
On 2012/09/04 13:58:06, Peng wrote:
> I also think maybe Config.vapi is better. It make all file names begin with
big
> letter.

I'm ok with that, but currently all the vapi files under /usr/share/vala*/vapi
use lower letters only.

https://codereview.appspot.com/6501080/diff/5001/bindings/vala/external-libs....
File bindings/vala/external-libs.vapi (right):

https://codereview.appspot.com/6501080/diff/5001/bindings/vala/external-libs....
bindings/vala/external-libs.vapi:1: [CCode (cprefix = "", lower_case_cprefix =
"", cheader_filename = "X11/extensions/XInput2.h")]
On 2012/09/04 13:58:06, Peng wrote:
> If we can upstream it to vala or XI, it would be better.

Sure.

https://codereview.appspot.com/6501080/diff/5001/ui/gtk3/keybindingmanager.vala
File ui/gtk3/keybindingmanager.vala (right):

https://codereview.appspot.com/6501080/diff/5001/ui/gtk3/keybindingmanager.va...
ui/gtk3/keybindingmanager.vala:217: uint mask = 0;
On 2012/09/04 13:58:06, Peng wrote:
> Why not mask = X.KeyMask.LockMask | X.KeyMask.Mod2Mask | ...?

Sure.

https://codereview.appspot.com/6501080/diff/5001/ui/gtk3/keybindingmanager.va...
ui/gtk3/keybindingmanager.vala:242: }
On 2012/09/04 13:58:06, Peng wrote:
> I think this logic is a little difficult to understand. I pref just list all
> possible combinations here. Like the code .c file.
>  83     XIGrabModifiers ximodifiers[] = {	
>   84         {modifiers, 0},	
>   85         {Mod2Mask | modifiers, 0},	
>   86         {LockMask | modifiers, 0},	
>   87         {Mod5Mask | modifiers, 0},	
>   88         {Mod2Mask | LockMask | modifiers, 0},	
>   89         {Mod2Mask | Mod5Mask | modifiers, 0},	
>   90         {LockMask | Mod5Mask | modifiers, 0},	
>   91         {Mod2Mask | LockMask | Mod5Mask | modifiers, 0},	
>   92     };
> 
> Or you may simplify the logic as:
> 
> int[] masks;
> 
> foreach (var m in ignored_modifiers) {
>    masks += [m];
>    for (int i = 0; i < masks.length - 1; i++) {
>        masks += [masks[i] | m];
>    }
> }
>  

The former does not conform to the Vala syntax.  And the latter is not correct -
the combination LockMask | Mod5Mask won't be enumerated.

Actually, the code is based on
gnome-settings-daemon/panels/common/gsd-keygrab.c.  I think it is better among
others since it allows more modifiers to be added.

[Peng, 2012-09-04 15:07:03]

https://codereview.appspot.com/6501080/diff/5001/bindings/vala/Makefile.am
File bindings/vala/Makefile.am (right):

https://codereview.appspot.com/6501080/diff/5001/bindings/vala/Makefile.am#ne...
bindings/vala/Makefile.am:47: config.vapi \
sorry. my misstake. please forget it.
On 2012/09/04 14:44:53, Daiki Ueno wrote:
> On 2012/09/04 13:58:06, Peng wrote:
> > I also think maybe Config.vapi is better. It make all file names begin with
> big
> > letter.
> 
> I'm ok with that, but currently all the vapi files under /usr/share/vala*/vapi
> use lower letters only.

https://codereview.appspot.com/6501080/diff/5001/bindings/vala/Makefile.am#ne...
bindings/vala/Makefile.am:48: external-libs.vapi \
On 2012/09/04 13:58:06, Peng wrote:
> Maybe name XI.vapi is better.

so it should be xi.vapi

[Daiki Ueno, 2012-09-05 09:14:19]

I think all the issues should be addressed now.  Please review again when you
have time.  Thanks.

https://codereview.appspot.com/6501080/diff/5001/ui/gtk3/keybindingmanager.vala
File ui/gtk3/keybindingmanager.vala (right):

https://codereview.appspot.com/6501080/diff/5001/ui/gtk3/keybindingmanager.va...
ui/gtk3/keybindingmanager.vala:242: }
On 2012/09/04 14:44:53, Daiki Ueno wrote:
> And the latter is not correct -
> the combination LockMask | Mod5Mask won't be enumerated.

Sorry, I was wrong here - simplified with your code.

[Peng, 2012-09-05 13:20:29]

lgtm with comments.

https://codereview.appspot.com/6501080/diff/3002/ui/gtk3/keybindingmanager.vala
File ui/gtk3/keybindingmanager.vala (right):

https://codereview.appspot.com/6501080/diff/3002/ui/gtk3/keybindingmanager.va...
ui/gtk3/keybindingmanager.vala:217: uint ignored_modifiers = IGNORED_MODIFIERS)
{
This line is more than 80 columns.

https://codereview.appspot.com/6501080/diff/3002/ui/gtk3/keybindingmanager.va...
ui/gtk3/keybindingmanager.vala:217: uint ignored_modifiers = IGNORED_MODIFIERS)
{
Do we need the ignored_modifiers arg?

https://codereview.appspot.com/6501080/diff/3002/ui/gtk3/keybindingmanager.va...
ui/gtk3/keybindingmanager.vala:219: for (int i = 0; ignored_modifiers != 0; i++,
ignored_modifiers >>= 1) {
Looks great. BTW, with this logic, I think probably making IGNORED_MODIFIERS an
array and enumerating it would be more readable.

[Peng, 2012-09-05 13:22:41]

On 2012/09/05 13:20:29, Peng wrote:
> lgtm with comments.
> 
>
https://codereview.appspot.com/6501080/diff/3002/ui/gtk3/keybindingmanager.vala
> File ui/gtk3/keybindingmanager.vala (right):
> 
>
https://codereview.appspot.com/6501080/diff/3002/ui/gtk3/keybindingmanager.va...
> ui/gtk3/keybindingmanager.vala:217: uint ignored_modifiers =
IGNORED_MODIFIERS)
> {
> This line is more than 80 columns.
> 
>
https://codereview.appspot.com/6501080/diff/3002/ui/gtk3/keybindingmanager.va...
> ui/gtk3/keybindingmanager.vala:217: uint ignored_modifiers =
IGNORED_MODIFIERS)
> {
> Do we need the ignored_modifiers arg?
> 
>
https://codereview.appspot.com/6501080/diff/3002/ui/gtk3/keybindingmanager.va...
> ui/gtk3/keybindingmanager.vala:219: for (int i = 0; ignored_modifiers != 0;
i++,
> ignored_modifiers >>= 1) {
> Looks great. BTW, with this logic, I think probably making IGNORED_MODIFIERS
an
> array and enumerating it would be more readable.

BTW, If we have some test cases can cover code in ui/gtk3 that would be better.

[Daiki Ueno, 2012-09-06 00:52:10]

I'll open a new CL for the tests later.  Thanks for the review.

https://codereview.appspot.com/6501080/diff/3002/ui/gtk3/keybindingmanager.vala
File ui/gtk3/keybindingmanager.vala (right):

https://codereview.appspot.com/6501080/diff/3002/ui/gtk3/keybindingmanager.va...
ui/gtk3/keybindingmanager.vala:219: for (int i = 0; ignored_modifiers != 0; i++,
ignored_modifiers >>= 1) {
On 2012/09/05 13:20:29, Peng wrote:
> Looks great. BTW, with this logic, I think probably making IGNORED_MODIFIERS
an
> array and enumerating it would be more readable.

So, I removed ignored_modifiers arg and make it local constant array.