Hi Henry, see my replies to your code review below:

You could also query the exact scale factor with a spin button.

yes this would be simpler to implement and probably require less memory
but it would be not fast and simple as the radiobuttons.
for now I will go on with radiobuttons but if there will be the need for more
sizes I will turn to a spinbutton.

xkb.h: No need to #include "ev.h" anymore.


xkb-plugin.c:269: There already is a variable called "fp" in this
function. I would prefer not to override that definition.


xkb-plugin.c:544,646,848: Do trampolines work everywhere we care? The
gcc(1) manpage says:

> A trampoline is a small piece of data or code that is created at run
> time on the stack when the address of a nested function is taken, and
> is used to call the nested function indirectly.  For some targets, it
> is made up of data only and thus requires no special treatment.  But,
> for most targets, it is made up of code and thus requires the stack to
> be made executable in order for the program to work properly.

Sofar xkb is the only plugin that requires an executable stack (see
execstack(8)). Ubuntu seems to prefer avoiding it:
and I don't think the C standard allows them. Opinions?

I was thinking defining a callback inside of a function was not any problem,
my fault and inexperience, now I declared a single callback globally
in place of the 3 nested ones,
taking as data the pointer to the button that may need to be clicked.
xkb-plugin.c:926-928: If the system() calls fail, an
ERR("xkb: blah, blah failed\n");
message should be returned. And perhaps replace the g_printf() with
LOG(LOG_INFO, "xkb: blah, blah %s\n", str);
to be consistent.

done. I didn't replace the g_print because it is commented and just for my
first steps debug.

xkb-plugin.c: Instead of 5 times on_radiobutton_flag_size_5_toggled()
you could pass the flag_size in the p_data, preferably using
But I am nitpicking. :)

this is unfortunately not possible because I need to give the pointer to the plugin
in the data since that's not global, and there's no room for a second parameter.
xkb-plugin.c:478:13: warning: 'on_hscale_flag_size_value_changed'
defined but not used [-Wunused-function]


Black text on dark background doesn't work too well. It would be nice to
have that configurable. (Or is there a better way?)

now I set the color taking it from the panel text color, it should be fine.
If you feel the need for custom colors anyway I can implement in future versions.
And one bug: The plugin correctly identifies my keyboard layout as "de",
but until it is configured the config dialog only shows "us" in the
"Keyboard Layouts" section.

It's not clear to me, you say that the plugin identifies the keyboard layout but
then it shows "us".
Maybe you mean that on the panel you have the correct indication "DE"
but the configuration dialog shows "US"?
If you can reproduce the problem, can you send me the output of
"setxkbmap -query" either before and after changing the configuration through the

Many thanks for the useful code review, I really appreciated.

For the latest dev code instructions on http://www.giuspen.com/customs/#dev
including cloning and moving to the correct branch