Hi,

I will take care of your considerations as soon as possible, just wanted to ensure that I did not
overwrite the source archive on my website.
I downloaded it right now and compared with the versioned
(and tagged 0.5, on branch xkb) version and noticed that there's just a problem: in the tarball
two symbolic links in the flags folder were removed, weird.
Probably the .tar.xz discards symbolic links.

In case Julien got the code from version control, be careful that there's more than one branch.

the branch named xkb includes a clean panel with only the xkb plugin modified (and the tagged releases), the default branch instead
includes the integration between plugins launchbar and taskbar I'm working on.

Cheers,
Giuseppe.



On Fri, Aug 10, 2012 at 10:47 PM, Henry Gebhardt <hsggebhardt@googlemail.com> wrote:
Hi guys,

On Thu, Aug 09, 2012 at 08:53:18PM +0200, Giuseppe Penone wrote:
> Hi Julien,
> what you mean with "when the flag image is OK"?
>
> actually with 1 -> 5 you are scaling the flag *0.5 -> *0.9 after the
> lxpanel icons side decide by the user.
> I understand you would like to have a position 6 where the flag has the
> same size as defined on the
> panel icons size, I'll do it as soon as I'll have a couple of hours.

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

> ...
>
> On Thu, Aug 9, 2012 at 12:01 AM, Julien Lavergne <gilir@ubuntu.com> wrote:
> > I pushed version 5 to trunk to get more testing.

Hm, it seems there are differences to the current version 5. Just to
clarify, Giuseppe, did you accidentally overwrite your version 5 with
what should be 6?


I have a few small comments about the code, version 0.5.10.5 on your
website. The numbers after the filename are line numbers.


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:
https://wiki.ubuntu.com/SecurityTeam/Roadmap/ExecutableStacks
and I don't think the C standard allows them. Opinions?


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.


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
GINT_TO_POINTER() and GPOINTER_TO_INT(), see
http://developer.gnome.org/glib/2.32/glib-Type-Conversion-Macros.html
But I am nitpicking. :)


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?)


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.


Thanks for the great work! It looks really awesome!

Regards,
Henry