Re: [Superkb-devel] [PATCH] Fix autorepeat in command when non-super was pressed first (bug #432887).

[Octavio A. <alv...@al...>]

On Sat, 04 Dec 2010 22:03:25 -0800, Eduardo A. Bustamante López
<ebu...@du...> wrote:

> This fixes the autorepeat bug detailed in:
> - https://bugs.launchpad.net/superkb/+bug/432887/

Thank you very much! This is interesting. It needs some fixing,
though.

> @@ -200,6 +200,13 @@ void one_superkey_used_friendly_warning(int number,  
> const char *keyname) {
>  	);
>  }
> +int key_is_released (Display *dpy, int keycode)

This should not be int, but KeyCode.

Also, please reverse this to key_is_pressed. An affirmative approach is
much more legible.

+    XQueryKeymap(dpy, key_buffer);

It's a nice idea to use this!

> -				debug(1, "[sk] Super key has been released, code: %d, name: %s.\n",  
> ev.xkey.keycode,
> -					XKeysymToString(XKeycodeToKeysym(this->dpy, ev.xkey.keycode, 0)));
> -
> -				if (--super_was_active) {
> -					debug(2, "[sa] super_was_active decreased to %d, ignoring  
> release.\n", super_was_active);
> -					continue;
> -				}
> -
> -				debug(2, "[sa] super_was_active decreased to %d, taking action.\n",  
> super_was_active);
> -
> -				timerclear(&to[TO_DRAWKB]);
> -				timerclear(&to[TO_CONFIG]);
> -

(snipped)

Please simplify the patch. Unfortunately, even if the logic requires this
code to be now differently indented, try to find another way to do it
without reindenting, as it is not straightforward to identify the actual
changes.

> +        if ( key_is_released (this->dpy, ev.xkey.keycode) )
> +        {
> +				  debug(1, "[sk] Super key has been released, code: %d, name:  
> %s.\n", ev.xkey.keycode,
> +					  XKeysymToString(XKeycodeToKeysym(this->dpy, ev.xkey.keycode,  
> 0)));

You want to test for the key being *press* here.

> @@ -507,23 +516,27 @@ void superkb_start(superkb_p this)
>  			debug(2, "[kp] Pushed key and state to stack: %d, %d\n",  
> ev.xkey.keycode, squashed_state);
>  		} else if ((ev.type == KeyRelease && !ignore_release &&
>  				   super_was_active > 0)) {
> +
> +

This extra space is unnecessary.

>  			/* User might have asked for binding configuration, so ignore key
>  			 * release. That's what ignore_release is for.
>  			 */
> -			timerclear(&to[TO_CONFIG]);
> -
> -			int squashed_state = ev.xkey.state & this->state_mask;

Again. Please work around reindenting. We will surely need a different
patch to modularize this and avoid this limitation in the future.

> +        if ( key_is_released (this->dpy, ev.xkey.keycode) )
> +        {
> +		        __Action(ev.xkey.keycode, squashed_state);

Yes, here we need to check for the key being not pressed.



-- 
Octavio.