Use gsettings instead of ibus-config in ibus-setup and ibus-ui-gtk3.

Use gsettings instead of ibus-config in ibus-setup and ibus-ui-gtk3.


TEST=ibus-setup and ibus-ui-gtk3

[Peng, 2013-08-08 19:26:51]

+Daiki as reviewer.

[Daiki Ueno, 2013-08-09 19:31:39]

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

https://codereview.appspot.com/12442044/diff/4001/ui/gtk3/panel.vala#newcode424
ui/gtk3/panel.vala:424: if (name == "preload-engines") {
how about using Settings.bind() instead of checking the key name every time? 
like:

public string[] preload_engines {
  set {
    update_engines(value, null);
  }
}

var settings = new Settings("...");
settings.bind("preload-engines", this, "preload_engines", ...);

[fujiwara, 2013-08-10 14:41:45]

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

https://codereview.appspot.com/12442044/diff/4001/ui/gtk3/panel.vala#newcode424
ui/gtk3/panel.vala:424: if (name == "preload-engines") {
On 2013/08/09 19:31:39, Daiki Ueno wrote:
> how about using Settings.bind() instead of checking the key name every time? 
> like:

I don't think checking key names is a trouble but now I think you may mind to
restrict the signals likes ibus_config_watch.
I think connecting "changed::preload-engines" signal is better than
g_settings_bind.

> 
> public string[] preload_engines {
>   set {
>     update_engines(value, null);
>   }
> }
> 
> var settings = new Settings("...");
> settings.bind("preload-engines", this, "preload_engines", ...);

I think it's not necessary to map "preload-engines" and "preload_engines".

[Daiki Ueno, 2013-08-12 08:01:00]

On 2013/08/10 14:41:45, fujiwara wrote:
> https://codereview.appspot.com/12442044/diff/4001/ui/gtk3/panel.vala
> File ui/gtk3/panel.vala (right):
> 
>
https://codereview.appspot.com/12442044/diff/4001/ui/gtk3/panel.vala#newcode424
> ui/gtk3/panel.vala:424: if (name == "preload-engines") {
> On 2013/08/09 19:31:39, Daiki Ueno wrote:
> > how about using Settings.bind() instead of checking the key name every time?

> > like:
> 
> I don't think checking key names is a trouble but now I think you may mind to
> restrict the signals likes ibus_config_watch.

yes, it is better.  removing such a conditionals is a common
refactoring technique:
http://refactoring.com/catalog/replaceConditionalWithPolymorphism.html

> I think connecting "changed::preload-engines" signal is better than
> g_settings_bind.

why do you think it is better?  if you use "changed::preload-engines",
you will need to manually retrieve the value by yourself.  you may
want to check existing gnome code, such as gnome-control-center, to
learn how they use gsettings.

[fujiwara, 2013-08-13 01:50:04]

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

https://codereview.appspot.com/12442044/diff/4001/ui/gtk3/panel.vala#newcode424
ui/gtk3/panel.vala:424: if (name == "preload-engines") {
> > I don't think checking key names is a trouble but now I think you may mind
to
> > restrict the signals likes ibus_config_watch.
> 
> yes, it is better.  removing such a conditionals is a common
refactoring technique:
> http://refactoring.com/catalog/replaceConditionalWithPolymorphism.html

Probably I don't think the replaceConditionalWithPolymorphism is same. That
example changes a class method with conditions to several methods. Your
suggestion changes a callback with conditions to each properties with conditions
in panel_set_property().

> > I think connecting "changed::preload-engines" signal is better than
> > g_settings_bind.
> 
> why do you think it is better?  if you use "changed::preload-engines",
> you will need to manually retrieve the value by yourself.  you may
> want to check existing gnome code, such as gnome-control-center, to
> learn how they use gsettings.

Because your suggestions need to install additional properties by manual.
My understanding is g_settings_bind is useful when a gsetting callback updates
an existent property likes "label" in gtk-lable.
I know gedit uses changed::each-signal to migrate dconf to gedit while I don't
think checking key names is a trouble.

BTW, it's better for reviews to reply on codes than reply on messages.

[fujiwara, 2013-08-16 03:21:06]

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

https://codereview.appspot.com/12442044/diff/4001/ui/gtk3/panel.vala#newcode424
ui/gtk3/panel.vala:424: if (name == "preload-engines") {
On 2013/08/13 01:50:04, fujiwara wrote:
> > > I don't think checking key names is a trouble but now I think you may mind
> to
> > > restrict the signals likes ibus_config_watch.
> > 
> > yes, it is better.  removing such a conditionals is a common
> refactoring technique:
> > http://refactoring.com/catalog/replaceConditionalWithPolymorphism.html
> 
> Probably I don't think the replaceConditionalWithPolymorphism is same. That
> example changes a class method with conditions to several methods. Your
> suggestion changes a callback with conditions to each properties with
conditions
> in panel_set_property().
> 
> > > I think connecting "changed::preload-engines" signal is better than
> > > g_settings_bind.
> > 
> > why do you think it is better?  if you use "changed::preload-engines",
> > you will need to manually retrieve the value by yourself.  you may
> > want to check existing gnome code, such as gnome-control-center, to
> > learn how they use gsettings.
> 
> Because your suggestions need to install additional properties by manual.
> My understanding is g_settings_bind is useful when a gsetting callback updates
> an existent property likes "label" in gtk-lable.
> I know gedit uses changed::each-signal to migrate dconf to gedit while I don't
> think checking key names is a trouble.
> 
> BTW, it's better for reviews to reply on codes than reply on messages.

Done.

[fujiwara, 2013-08-19 07:00:29]

If there are no any other comments, I'd integrate this patch.

[Daiki Ueno, 2013-08-19 08:46:34]

lgtm. sorry for the delay.

[Peng, 2013-08-19 16:18:29]

On 2013/08/19 08:46:34, Daiki Ueno wrote:
> lgtm. sorry for the delay.

lgtm.