Enable layout(variant)[option,...] format in IBusEngineDesc.layout

Add layout_variant and layout_option in IBusEngineDesc for XKB.


TEST=Manually

[Peng, 2012-10-04 13:32:02]

https://codereview.appspot.com/6586075/diff/1/src/ibusenginedesc.h
File src/ibusenginedesc.h (right):

https://codereview.appspot.com/6586075/diff/1/src/ibusenginedesc.h#newcode120
src/ibusenginedesc.h:120: *     The format "layout(variant)[option1,...]" is
supported.
May I ask why you define the format as this?
Could we make the format simpler for parsing? like layout:variant:option1,2,3
So we can use layout.split(":").

https://codereview.appspot.com/6586075/diff/1/ui/gtk3/panel.vala
File ui/gtk3/panel.vala (right):

https://codereview.appspot.com/6586075/diff/1/ui/gtk3/panel.vala#newcode215
ui/gtk3/panel.vala:215: string keymap = engine.get_layout().dup();
Could you add some comments about layout sample here? So I can understand how
you parse the layout.

https://codereview.appspot.com/6586075/diff/1/ui/gtk3/panel.vala#newcode219
ui/gtk3/panel.vala:219: string arg = null;
Can we use GString in vala? So you may use g_string_append_printf() to make code
simpler.

[fujiwara, 2012-10-05 06:31:35]

On 2012/10/04 13:32:02, Peng wrote:
> https://codereview.appspot.com/6586075/diff/1/src/ibusenginedesc.h
> File src/ibusenginedesc.h (right):
> 
> https://codereview.appspot.com/6586075/diff/1/src/ibusenginedesc.h#newcode120
> src/ibusenginedesc.h:120: *     The format "layout(variant)[option1,...]" is
> supported.
> May I ask why you define the format as this?
> Could we make the format simpler for parsing? like layout:variant:option1,2,3
> So we can use layout.split(":").

XKB options include ":". E.g. "ctrl:swapcaps".

> 
> https://codereview.appspot.com/6586075/diff/1/ui/gtk3/panel.vala
> File ui/gtk3/panel.vala (right):
> 
> https://codereview.appspot.com/6586075/diff/1/ui/gtk3/panel.vala#newcode215
> ui/gtk3/panel.vala:215: string keymap = engine.get_layout().dup();
> Could you add some comments about layout sample here? So I can understand how
> you parse the layout.
> 
> https://codereview.appspot.com/6586075/diff/1/ui/gtk3/panel.vala#newcode219
> ui/gtk3/panel.vala:219: string arg = null;
> Can we use GString in vala? So you may use g_string_append_printf() to make
code
> simpler.

Done.

[TiagoMatos, 2012-10-05 11:42:40]

Hi, I'd like to avoid parsing if at all possible. Could we instead add more
properties to IBusEngineDesc like "layout_variant" and "layout_options"?
"layout_options" could be a simple NULL terminated string vector.

[fujiwara, 2012-10-05 13:07:18]

On 2012/10/05 11:42:40, TiagoMatos wrote:
> Hi, I'd like to avoid parsing if at all possible. Could we instead add more
> properties to IBusEngineDesc like "layout_variant" and "layout_options"?
> "layout_options" could be a simple NULL terminated string vector.

OK, probably I think the suggestion is better and will update the patch later.
I think layout_variant and layout_option and each parameter could be comma
separated value while we need the options only for CVS at present.

[fujiwara, 2012-10-05 15:27:52]

On 2012/10/05 13:07:18, fujiwara wrote:
> On 2012/10/05 11:42:40, TiagoMatos wrote:
> > Hi, I'd like to avoid parsing if at all possible. Could we instead add more
> > properties to IBusEngineDesc like "layout_variant" and "layout_options"?
> > "layout_options" could be a simple NULL terminated string vector.
> 
> OK, probably I think the suggestion is better and will update the patch later.
> I think layout_variant and layout_option and each parameter could be comma
> separated value while we need the options only for CVS at present.

Updated the patch.

[fujiwara, 2012-10-05 17:49:08]

On 2012/10/05 15:27:52, fujiwara wrote:
> On 2012/10/05 13:07:18, fujiwara wrote:
> > On 2012/10/05 11:42:40, TiagoMatos wrote:
> > > Hi, I'd like to avoid parsing if at all possible. Could we instead add
more
> > > properties to IBusEngineDesc like "layout_variant" and "layout_options"?
> > > "layout_options" could be a simple NULL terminated string vector.
> > 
> > OK, probably I think the suggestion is better and will update the patch
later.
> > I think layout_variant and layout_option and each parameter could be comma
> > separated value while we need the options only for CVS at present.
> 
> Updated the patch.

Forgot to update panel.vala

[Peng, 2012-10-05 18:17:38]

https://codereview.appspot.com/6586075/diff/13001/src/ibusenginedesc.c
File src/ibusenginedesc.c (right):

https://codereview.appspot.com/6586075/diff/13001/src/ibusenginedesc.c#newcod...
src/ibusenginedesc.c:520: g_variant_get_child (variant, retval++, "s",
&desc->priv->layout_variant);
I think it may has some problem, if we get some data from old version ibus.

[fujiwara, 2012-10-05 18:51:02]

On 2012/10/05 18:17:38, Peng wrote:
> https://codereview.appspot.com/6586075/diff/13001/src/ibusenginedesc.c
> File src/ibusenginedesc.c (right):
> 
>
https://codereview.appspot.com/6586075/diff/13001/src/ibusenginedesc.c#newcod...
> src/ibusenginedesc.c:520: g_variant_get_child (variant, retval++, "s",
> &desc->priv->layout_variant);
> I think it may has some problem, if we get some data from old version ibus.

I think this patch itself is no problem because that's why we will bump ibus to
1.5.
But if you mind the future change, my previous suggestion was to add the total
numbers in the serialized object. E.g. in this patch, IBusEngineDesc will have
14 of the members and add the number:

  g_variant_builder_add (builder, "i", NOTNULL (14));

But you disagreed with this idea previously.

Now probably I think we can add desc->priv->entries = PROP_TOTAL;

[Daiki Ueno, 2012-10-06 01:23:48]

https://codereview.appspot.com/6586075/diff/13001/ui/gtk3/panel.vala
File ui/gtk3/panel.vala (right):

https://codereview.appspot.com/6586075/diff/13001/ui/gtk3/panel.vala#newcode321
ui/gtk3/panel.vala:321: string arg = null;
why not prepare the args as an array and use spawn_sync?

https://codereview.appspot.com/6586075/diff/13001/ui/gtk3/panel.vala#newcode331
ui/gtk3/panel.vala:331: buffer.append_printf(" -option -option %s", option);
typo?

https://codereview.appspot.com/6586075/diff/13001/ui/gtk3/panel.vala#newcode335
ui/gtk3/panel.vala:335: if (arg == null || arg.length == 0) {
better check buffer.len

[fujiwara, 2012-10-06 07:54:36]

On 2012/10/06 01:23:48, Daiki Ueno wrote:
> https://codereview.appspot.com/6586075/diff/13001/ui/gtk3/panel.vala
> File ui/gtk3/panel.vala (right):
> 
>
https://codereview.appspot.com/6586075/diff/13001/ui/gtk3/panel.vala#newcode321
> ui/gtk3/panel.vala:321: string arg = null;
> why not prepare the args as an array and use spawn_sync?

I don't see much benefits to use the string array but followed your suggestion.

> 
>
https://codereview.appspot.com/6586075/diff/13001/ui/gtk3/panel.vala#newcode331
> ui/gtk3/panel.vala:331: buffer.append_printf(" -option -option %s", option);
> typo?

No

> 
>
https://codereview.appspot.com/6586075/diff/13001/ui/gtk3/panel.vala#newcode335
> ui/gtk3/panel.vala:335: if (arg == null || arg.length == 0) {
> better check buffer.len

[Peng, 2012-10-08 00:33:43]

https://codereview.appspot.com/6586075/diff/16002/ibus/enginedesc.py
File ibus/enginedesc.py (right):

https://codereview.appspot.com/6586075/diff/16002/ibus/enginedesc.py#newcode143
ibus/enginedesc.py:143: self.__setup  = struct.pop(0)
You need check number of struct(), if it does not have layout_variant and etc,
you should stop deserialize them

https://codereview.appspot.com/6586075/diff/16002/src/ibusenginedesc.c
File src/ibusenginedesc.c (right):

https://codereview.appspot.com/6586075/diff/16002/src/ibusenginedesc.c#newcod...
src/ibusenginedesc.c:304: desc->priv->length = PROP_LAST - 1;
It is a constant. I don't see any reason to add it.

https://codereview.appspot.com/6586075/diff/16002/src/ibusenginedesc.c#newcod...
src/ibusenginedesc.c:525: if (desc->priv->length < 14)
I think you need use g_variant_n_children() to get the serialized properties in
variant.

[fujiwara, 2012-10-12 11:57:28]

On 2012/10/08 00:33:43, Peng wrote:
> https://codereview.appspot.com/6586075/diff/16002/ibus/enginedesc.py
> File ibus/enginedesc.py (right):
> 
>
https://codereview.appspot.com/6586075/diff/16002/ibus/enginedesc.py#newcode143
> ibus/enginedesc.py:143: self.__setup  = struct.pop(0)
> You need check number of struct(), if it does not have layout_variant and etc,
> you should stop deserialize them

Done

> 
> https://codereview.appspot.com/6586075/diff/16002/src/ibusenginedesc.c
> File src/ibusenginedesc.c (right):
> 
>
https://codereview.appspot.com/6586075/diff/16002/src/ibusenginedesc.c#newcod...
> src/ibusenginedesc.c:304: desc->priv->length = PROP_LAST - 1;
> It is a constant. I don't see any reason to add it.

You told me that the deserialized desc object is not sent between two apps but
serialized data is sent. So I removed the serialize check.

> 
>
https://codereview.appspot.com/6586075/diff/16002/src/ibusenginedesc.c#newcod...
> src/ibusenginedesc.c:525: if (desc->priv->length < 14)
> I think you need use g_variant_n_children() to get the serialized properties
in
> variant.

Done.

[Peng, 2012-10-12 23:53:20]

https://codereview.appspot.com/6586075/diff/21001/ibus/enginedesc.py
File ibus/enginedesc.py (right):

https://codereview.appspot.com/6586075/diff/21001/ibus/enginedesc.py#newcode35
ibus/enginedesc.py:35: license="", author="", icon="", layout="en", hotkeys="",
en? does xkb have en layout? maybe us?

https://codereview.appspot.com/6586075/diff/21001/src/ibusenginedesc.c
File src/ibusenginedesc.c (right):

https://codereview.appspot.com/6586075/diff/21001/src/ibusenginedesc.c#newcode39
src/ibusenginedesc.c:39: PROP_LAYOUT,
Could we put new properties here?

https://codereview.appspot.com/6586075/diff/21001/src/ibusenginedesc.c#newcod...
src/ibusenginedesc.c:522: if (g_variant_n_children (variant) - parent_length <
PROP_LAYOUT_OPTION)
test g_variant_n_children(variant) >= retval + 2 is more readable.

[fujiwara, 2012-10-15 04:07:45]

On 2012/10/12 23:53:20, Peng wrote:
> https://codereview.appspot.com/6586075/diff/21001/ibus/enginedesc.py
> File ibus/enginedesc.py (right):
> 
> https://codereview.appspot.com/6586075/diff/21001/ibus/enginedesc.py#newcode35
> ibus/enginedesc.py:35: license="", author="", icon="", layout="en",
hotkeys="",
> en? does xkb have en layout? maybe us?

Fixed. I mistook it.

> 
> https://codereview.appspot.com/6586075/diff/21001/src/ibusenginedesc.c
> File src/ibusenginedesc.c (right):
> 
>
https://codereview.appspot.com/6586075/diff/21001/src/ibusenginedesc.c#newcode39
> src/ibusenginedesc.c:39: PROP_LAYOUT,
> Could we put new properties here?
> 
>
https://codereview.appspot.com/6586075/diff/21001/src/ibusenginedesc.c#newcod...
> src/ibusenginedesc.c:522: if (g_variant_n_children (variant) - parent_length <
> PROP_LAYOUT_OPTION)
> test g_variant_n_children(variant) >= retval + 2 is more readable.

Done.

[Peng, 2012-10-15 14:44:49]

On 2012/10/15 04:07:45, fujiwara wrote:
> On 2012/10/12 23:53:20, Peng wrote:
> > https://codereview.appspot.com/6586075/diff/21001/ibus/enginedesc.py
> > File ibus/enginedesc.py (right):
> > 
> >
https://codereview.appspot.com/6586075/diff/21001/ibus/enginedesc.py#newcode35
> > ibus/enginedesc.py:35: license="", author="", icon="", layout="en",
> hotkeys="",
> > en? does xkb have en layout? maybe us?
> 
> Fixed. I mistook it.
> 
> > 
> > https://codereview.appspot.com/6586075/diff/21001/src/ibusenginedesc.c
> > File src/ibusenginedesc.c (right):
> > 
> >
>
https://codereview.appspot.com/6586075/diff/21001/src/ibusenginedesc.c#newcode39
> > src/ibusenginedesc.c:39: PROP_LAYOUT,
> > Could we put new properties here?
> > 
> >
>
https://codereview.appspot.com/6586075/diff/21001/src/ibusenginedesc.c#newcod...
> > src/ibusenginedesc.c:522: if (g_variant_n_children (variant) - parent_length
<
> > PROP_LAYOUT_OPTION)
> > test g_variant_n_children(variant) >= retval + 2 is more readable.
> 
> Done.

lgtm