Issue 103440043: Set up ibus engines when users log into the session initially. (Closed)
DescriptionSet up ibus engines when users log into the session for the first time.
XKB engines are configured by the session XKB.
IM engines are configured by the current locale and IBusEngineDesc.lang.
Also append 'us' layout after non-ascii layout likes 'ara', 'ru'.
BUG=rhbz#541492
TEST=ui/gtk3/ibus-ui-gtk3 data/dconf/org.freedesktop.ibus.gschema.xml
Patch Set 1 #
Total comments: 38
Patch Set 2 : Updated with message #5. #
Total comments: 27
Patch Set 3 : Updated with message #7. #
Total comments: 4
Patch Set 4 : Updated with message #9. #
MessagesTotal messages: 10
This is a fork of cl #5969078.
Sign in to reply to this message.
On 2014/06/13 08:41:47, fujiwara wrote: > This is a fork of cl #5969078. I think the description is not clear. From the description, I don't understand what is CL for.
Sign in to reply to this message.
On 2014/06/14 02:22:35, Peng wrote: > On 2014/06/13 08:41:47, fujiwara wrote: > > This is a fork of cl #5969078. > > I think the description is not clear. From the description, I don't understand > what is CL for. Description? See the git log.
Sign in to reply to this message.
On 2014/06/16 02:36:20, fujiwara wrote: > On 2014/06/14 02:22:35, Peng wrote: > > On 2014/06/13 08:41:47, fujiwara wrote: > > > This is a fork of cl #5969078. > > > > I think the description is not clear. From the description, I don't understand > > what is CL for. > > Description? > See the git log. Updated the subject slightly.
Sign in to reply to this message.
https://codereview.appspot.com/103440043/diff/1/data/ibus.schemas.in File data/ibus.schemas.in (right): https://codereview.appspot.com/103440043/diff/1/data/ibus.schemas.in#newcode66 data/ibus.schemas.in:66: <short>Latin layout which have no ASCII</short> layout -> layouts https://codereview.appspot.com/103440043/diff/1/data/ibus.schemas.in#newcode67 data/ibus.schemas.in:67: <long>us layout is appended to the latin layouts. variant is not needed.</long> > 80 chars https://codereview.appspot.com/103440043/diff/1/ui/gtk3/panel.vala File ui/gtk3/panel.vala (right): https://codereview.appspot.com/103440043/diff/1/ui/gtk3/panel.vala#newcode272 ui/gtk3/panel.vala:272: private void update_xkb_engines(GLib.List<IBus.EngineDesc> engines) { Please add comment about this function to explain what does it do. https://codereview.appspot.com/103440043/diff/1/ui/gtk3/panel.vala#newcode288 ui/gtk3/panel.vala:288: for (unowned GLib.List<IBus.EngineDesc> p = engines; does vala support foreach statement to go through a list? https://codereview.appspot.com/103440043/diff/1/ui/gtk3/panel.vala#newcode298 ui/gtk3/panel.vala:298: ((engine.get_layout_variant() == "" && variant == null) || How about m_xkblayout.get_layout() return "" instead of null for the empty variant. so the comparing will be simpler. https://codereview.appspot.com/103440043/diff/1/ui/gtk3/panel.vala#newcode317 ui/gtk3/panel.vala:317: string[] a = m_settings_general.get_strv("preload-engines"); why fetch "preload-engines" setting here? https://codereview.appspot.com/103440043/diff/1/ui/gtk3/panel.vala#newcode327 ui/gtk3/panel.vala:327: GLib.List<IBus.EngineDesc> im_engines = null; http://www.valadoc.org/#!api=glib-2.0/GLib.List should we new a List object here? https://codereview.appspot.com/103440043/diff/1/ui/gtk3/panel.vala#newcode386 ui/gtk3/panel.vala:386: if (inited_engines_order) why need check it? I think check m_settings_general.get_strv("preload-engines").length >0 is enough? https://codereview.appspot.com/103440043/diff/1/ui/gtk3/panel.vala#newcode396 ui/gtk3/panel.vala:396: update_im_engines(engines); Seems those two functions will get and set preload_engines several times. Could you please let them just update engine list in memory, and set the preload_engines one time? https://codereview.appspot.com/103440043/diff/1/ui/gtk3/xkblayout.vala File ui/gtk3/xkblayout.vala (right): https://codereview.appspot.com/103440043/diff/1/ui/gtk3/xkblayout.vala#newcode28 ui/gtk3/xkblayout.vala:28: const string m_xkb_get_args = "-query"; static? Does vala support static variables? https://codereview.appspot.com/103440043/diff/1/ui/gtk3/xkblayout.vala#newcode42 ui/gtk3/xkblayout.vala:42: public void get_layout(out string layout, If the get_layout does not modify any member fields, maybe change it to as static method. https://codereview.appspot.com/103440043/diff/1/ui/gtk3/xkblayout.vala#newcode43 ui/gtk3/xkblayout.vala:43: out string variant, indent wrong https://codereview.appspot.com/103440043/diff/1/ui/gtk3/xkblayout.vala#newcod... ui/gtk3/xkblayout.vala:194: args += "-option"; why add option twice? https://codereview.appspot.com/103440043/diff/1/ui/gtk3/xkblayout.vala#newcod... ui/gtk3/xkblayout.vala:200: if (!GLib.Process.spawn_async(null, why async? maybe sync is simpler? https://codereview.appspot.com/103440043/diff/1/ui/gtk3/xkblayout.vala#newcod... ui/gtk3/xkblayout.vala:218: #if 0 remove those unused code?
Sign in to reply to this message.
https://codereview.appspot.com/103440043/diff/1/data/ibus.schemas.in File data/ibus.schemas.in (right): https://codereview.appspot.com/103440043/diff/1/data/ibus.schemas.in#newcode66 data/ibus.schemas.in:66: <short>Latin layout which have no ASCII</short> On 2014/06/18 15:34:23, Peng wrote: > layout -> layouts Done. https://codereview.appspot.com/103440043/diff/1/data/ibus.schemas.in#newcode67 data/ibus.schemas.in:67: <long>us layout is appended to the latin layouts. variant is not needed.</long> On 2014/06/18 15:34:23, Peng wrote: > > 80 chars Done. https://codereview.appspot.com/103440043/diff/1/ui/gtk3/panel.vala File ui/gtk3/panel.vala (right): https://codereview.appspot.com/103440043/diff/1/ui/gtk3/panel.vala#newcode272 ui/gtk3/panel.vala:272: private void update_xkb_engines(GLib.List<IBus.EngineDesc> engines) { On 2014/06/18 15:34:24, Peng wrote: > Please add comment about this function to explain what does it do. Done. https://codereview.appspot.com/103440043/diff/1/ui/gtk3/panel.vala#newcode288 ui/gtk3/panel.vala:288: for (unowned GLib.List<IBus.EngineDesc> p = engines; On 2014/06/18 15:34:24, Peng wrote: > does vala support foreach statement to go through a list? You're right. Done. https://codereview.appspot.com/103440043/diff/1/ui/gtk3/panel.vala#newcode298 ui/gtk3/panel.vala:298: ((engine.get_layout_variant() == "" && variant == null) || On 2014/06/18 15:34:24, Peng wrote: > How about m_xkblayout.get_layout() return "" instead of null for the empty > variant. so the comparing will be simpler. get_layout() returns "" but "".split(",") causes null. I modified "variant = variant_array[i]" slightly for this. https://codereview.appspot.com/103440043/diff/1/ui/gtk3/panel.vala#newcode317 ui/gtk3/panel.vala:317: string[] a = m_settings_general.get_strv("preload-engines"); On 2014/06/18 15:34:24, Peng wrote: > why fetch "preload-engines" setting here? Sorry it was a debug code. Done. https://codereview.appspot.com/103440043/diff/1/ui/gtk3/panel.vala#newcode327 ui/gtk3/panel.vala:327: GLib.List<IBus.EngineDesc> im_engines = null; On 2014/06/18 15:34:24, Peng wrote: > http://www.valadoc.org/#!api=glib-2.0/GLib.List > > should we new a List object here? I think the index of foreach will be confused if a member is removed during foreach. So I think a new list would be good. Also the list will be sorted by IBus.EngineDesc.rank later. https://codereview.appspot.com/103440043/diff/1/ui/gtk3/panel.vala#newcode386 ui/gtk3/panel.vala:386: if (inited_engines_order) On 2014/06/18 15:34:24, Peng wrote: > why need check it? I think check > m_settings_general.get_strv("preload-engines").length >0 is enough? If users remove all engines on ibus-setup and restart ibus, I think users don't wish to load IMEs. Then this API is passed and only "xkb:us::eng" will be added later. https://codereview.appspot.com/103440043/diff/1/ui/gtk3/panel.vala#newcode396 ui/gtk3/panel.vala:396: update_im_engines(engines); On 2014/06/18 15:34:24, Peng wrote: > Seems those two functions will get and set preload_engines several times. Could > you please let them just update engine list in memory, and set the > preload_engines one time? Done. https://codereview.appspot.com/103440043/diff/1/ui/gtk3/xkblayout.vala File ui/gtk3/xkblayout.vala (right): https://codereview.appspot.com/103440043/diff/1/ui/gtk3/xkblayout.vala#newcode28 ui/gtk3/xkblayout.vala:28: const string m_xkb_get_args = "-query"; On 2014/06/18 15:34:24, Peng wrote: > static? Does vala support static variables? I think "static const" and "const" is same in vala. I think "const" is better than "static" here because the variables won't be modified. https://codereview.appspot.com/103440043/diff/1/ui/gtk3/xkblayout.vala#newcode42 ui/gtk3/xkblayout.vala:42: public void get_layout(out string layout, On 2014/06/18 15:34:24, Peng wrote: > If the get_layout does not modify any member fields, maybe change it to as > static method. I guess you mean private method instead of static method. This API update internal m_default_options so that set_layout() can revert the original option. Probably I think it's good to put the public get_layout() and set_layout() in this class. https://codereview.appspot.com/103440043/diff/1/ui/gtk3/xkblayout.vala#newcode43 ui/gtk3/xkblayout.vala:43: out string variant, On 2014/06/18 15:34:24, Peng wrote: > indent wrong Done. https://codereview.appspot.com/103440043/diff/1/ui/gtk3/xkblayout.vala#newcod... ui/gtk3/xkblayout.vala:194: args += "-option"; On 2014/06/18 15:34:24, Peng wrote: > why add option twice? Because this overrides XKB option. See man page. https://codereview.appspot.com/103440043/diff/1/ui/gtk3/xkblayout.vala#newcod... ui/gtk3/xkblayout.vala:200: if (!GLib.Process.spawn_async(null, On 2014/06/18 15:34:24, Peng wrote: > why async? maybe sync is simpler? I'm thinking, after this patch is integrated, I will add to call 'xmodmap .xmodmaprc" after call the xkb command. And maybe take much time to call two commands? I've been implemented xkb command and xmodmap in Fedora upon the users request. I'm not sure if sync is better here. https://codereview.appspot.com/103440043/diff/1/ui/gtk3/xkblayout.vala#newcod... ui/gtk3/xkblayout.vala:218: #if 0 On 2014/06/18 15:34:24, Peng wrote: > remove those unused code? Done.
Sign in to reply to this message.
https://codereview.appspot.com/103440043/diff/1/ui/gtk3/xkblayout.vala File ui/gtk3/xkblayout.vala (right): https://codereview.appspot.com/103440043/diff/1/ui/gtk3/xkblayout.vala#newcode28 ui/gtk3/xkblayout.vala:28: const string m_xkb_get_args = "-query"; On 2014/06/19 10:20:51, fujiwara wrote: > On 2014/06/18 15:34:24, Peng wrote: > > static? Does vala support static variables? > > I think "static const" and "const" is same in vala. > I think "const" is better than "static" here because the variables won't be > modified. Could you please use a better variable name for const variables? For example: private static const string XKB_COMMAND = ".."; private static const string XKB_QUERY_ARGS = "..."; https://codereview.appspot.com/103440043/diff/1/ui/gtk3/xkblayout.vala#newcode42 ui/gtk3/xkblayout.vala:42: public void get_layout(out string layout, On 2014/06/19 10:20:51, fujiwara wrote: > On 2014/06/18 15:34:24, Peng wrote: > > If the get_layout does not modify any member fields, maybe change it to as > > static method. > > I guess you mean private method instead of static method. > This API update internal m_default_options so that set_layout() can revert the > original option. > Probably I think it's good to put the public get_layout() and set_layout() in > this class. I don't see this function modify any member variables. So it could be static function. So it could be used without an instance. https://codereview.appspot.com/103440043/diff/1/ui/gtk3/xkblayout.vala#newcod... ui/gtk3/xkblayout.vala:200: if (!GLib.Process.spawn_async(null, On 2014/06/19 10:20:51, fujiwara wrote: > On 2014/06/18 15:34:24, Peng wrote: > > why async? maybe sync is simpler? > > I'm thinking, after this patch is integrated, I will add to call 'xmodmap > .xmodmaprc" after call the xkb command. > And maybe take much time to call two commands? > I've been implemented xkb command and xmodmap in Fedora upon the users request. > I'm not sure if sync is better here. Probably sync is OK. It could avoid the reenter issue (The second call will be ignored). https://codereview.appspot.com/103440043/diff/20001/ui/gtk3/panel.vala File ui/gtk3/panel.vala (right): https://codereview.appspot.com/103440043/diff/20001/ui/gtk3/panel.vala#newcod... ui/gtk3/panel.vala:282: private string[] update_xkb_engines(GLib.List<IBus.EngineDesc> engines) { How about get_engines_from_xkb()? https://codereview.appspot.com/103440043/diff/20001/ui/gtk3/panel.vala#newcod... ui/gtk3/panel.vala:282: private string[] update_xkb_engines(GLib.List<IBus.EngineDesc> engines) { maybe just return a GLib.List? https://codereview.appspot.com/103440043/diff/20001/ui/gtk3/panel.vala#newcod... ui/gtk3/panel.vala:292: for (int i = 0; i < layout_array.length; i++) { foreach? https://codereview.appspot.com/103440043/diff/20001/ui/gtk3/panel.vala#newcod... ui/gtk3/panel.vala:300: variant = ""; It is not necessary. The variant will be assigned to "" below. https://codereview.appspot.com/103440043/diff/20001/ui/gtk3/panel.vala#newcod... ui/gtk3/panel.vala:312: if (name.length < 4 || name[0:4] != "xkb:") http://references.valadoc.org/#!api=glib-2.0/string.has_prefix ? https://codereview.appspot.com/103440043/diff/20001/ui/gtk3/panel.vala#newcod... ui/gtk3/panel.vala:346: private string[] update_im_engines(GLib.List<IBus.EngineDesc> engines) { get_engines_by_local()? https://codereview.appspot.com/103440043/diff/20001/ui/gtk3/panel.vala#newcod... ui/gtk3/panel.vala:346: private string[] update_im_engines(GLib.List<IBus.EngineDesc> engines) { return GLib.List https://codereview.appspot.com/103440043/diff/20001/ui/gtk3/panel.vala#newcod... ui/gtk3/panel.vala:358: if (name.length >= 4 && name[0:4] == "xkb:") name.has_prefix("xkb:") https://codereview.appspot.com/103440043/diff/20001/ui/gtk3/panel.vala#newcod... ui/gtk3/panel.vala:366: lang = lang.split("_")[0]; move it into below if {} https://codereview.appspot.com/103440043/diff/20001/ui/gtk3/panel.vala#newcod... ui/gtk3/panel.vala:371: if (name.length >= 4 && name[0:4] == "xkb:") has_prefix(). https://codereview.appspot.com/103440043/diff/20001/ui/gtk3/xkblayout.vala File ui/gtk3/xkblayout.vala (right): https://codereview.appspot.com/103440043/diff/20001/ui/gtk3/xkblayout.vala#ne... ui/gtk3/xkblayout.vala:79: line[0:element.length] == element) { line.has_prefix(element) https://codereview.appspot.com/103440043/diff/20001/ui/gtk3/xkblayout.vala#ne... ui/gtk3/xkblayout.vala:90: line[0:element.length] == element) { has_prefix https://codereview.appspot.com/103440043/diff/20001/ui/gtk3/xkblayout.vala#ne... ui/gtk3/xkblayout.vala:101: line[0:element.length] == element) { has_prefix https://codereview.appspot.com/103440043/diff/20001/ui/gtk3/xkblayout.vala#ne... ui/gtk3/xkblayout.vala:130: Could you please add some comment? I don't know why it should return here https://codereview.appspot.com/103440043/diff/20001/ui/gtk3/xkblayout.vala#ne... ui/gtk3/xkblayout.vala:138: foreach (string latin_layout in m_xkb_latin_layouts) { if (variant != "eng") need_us_layout = layout in m_xkb_latin_layouts; if (variant != null && !need_us_layout) need_us_layout = "%s(%s)".printf(layout, variant) in m_xkb_latin_layouts; ?
Sign in to reply to this message.
https://codereview.appspot.com/103440043/diff/1/ui/gtk3/xkblayout.vala File ui/gtk3/xkblayout.vala (right): https://codereview.appspot.com/103440043/diff/1/ui/gtk3/xkblayout.vala#newcode28 ui/gtk3/xkblayout.vala:28: const string m_xkb_get_args = "-query"; On 2014/06/19 14:46:14, Peng wrote: > On 2014/06/19 10:20:51, fujiwara wrote: > > On 2014/06/18 15:34:24, Peng wrote: > > > static? Does vala support static variables? > > > > I think "static const" and "const" is same in vala. > > I think "const" is better than "static" here because the variables won't be > > modified. > > Could you please use a better variable name for const variables? > > For example: > > private static const string XKB_COMMAND = ".."; > private static const string XKB_QUERY_ARGS = "..."; Done. https://codereview.appspot.com/103440043/diff/1/ui/gtk3/xkblayout.vala#newcode42 ui/gtk3/xkblayout.vala:42: public void get_layout(out string layout, On 2014/06/19 14:46:14, Peng wrote: > On 2014/06/19 10:20:51, fujiwara wrote: > > On 2014/06/18 15:34:24, Peng wrote: > > > If the get_layout does not modify any member fields, maybe change it to as > > > static method. > > > > I guess you mean private method instead of static method. > > This API update internal m_default_options so that set_layout() can revert the > > original option. > > Probably I think it's good to put the public get_layout() and set_layout() in > > this class. > > I don't see this function modify any member variables. So it could be static > function. So it could be used without an instance. > Ah you meant really static. Done. https://codereview.appspot.com/103440043/diff/1/ui/gtk3/xkblayout.vala#newcod... ui/gtk3/xkblayout.vala:200: if (!GLib.Process.spawn_async(null, On 2014/06/19 14:46:14, Peng wrote: > On 2014/06/19 10:20:51, fujiwara wrote: > > On 2014/06/18 15:34:24, Peng wrote: > > > why async? maybe sync is simpler? > > > > I'm thinking, after this patch is integrated, I will add to call 'xmodmap > > .xmodmaprc" after call the xkb command. > > And maybe take much time to call two commands? > > I've been implemented xkb command and xmodmap in Fedora upon the users > request. > > I'm not sure if sync is better here. > > Probably sync is OK. It could avoid the reenter issue (The second call will be > ignored). I changed it to sync but I'm still concerned about dbus timeout rather than ignored second calls. https://codereview.appspot.com/103440043/diff/20001/ui/gtk3/panel.vala File ui/gtk3/panel.vala (right): https://codereview.appspot.com/103440043/diff/20001/ui/gtk3/panel.vala#newcod... ui/gtk3/panel.vala:282: private string[] update_xkb_engines(GLib.List<IBus.EngineDesc> engines) { On 2014/06/19 14:46:14, Peng wrote: > How about get_engines_from_xkb()? Done. https://codereview.appspot.com/103440043/diff/20001/ui/gtk3/panel.vala#newcod... ui/gtk3/panel.vala:282: private string[] update_xkb_engines(GLib.List<IBus.EngineDesc> engines) { On 2014/06/19 14:46:14, Peng wrote: > maybe just return a GLib.List? Done. https://codereview.appspot.com/103440043/diff/20001/ui/gtk3/panel.vala#newcod... ui/gtk3/panel.vala:292: for (int i = 0; i < layout_array.length; i++) { On 2014/06/19 14:46:14, Peng wrote: > foreach? No, the index is used by both layouts and variants. https://codereview.appspot.com/103440043/diff/20001/ui/gtk3/panel.vala#newcod... ui/gtk3/panel.vala:300: variant = ""; On 2014/06/19 14:46:14, Peng wrote: > It is not necessary. The variant will be assigned to "" below. Done. https://codereview.appspot.com/103440043/diff/20001/ui/gtk3/panel.vala#newcod... ui/gtk3/panel.vala:312: if (name.length < 4 || name[0:4] != "xkb:") On 2014/06/19 14:46:14, Peng wrote: > http://references.valadoc.org/#!api=glib-2.0/string.has_prefix ? Done. https://codereview.appspot.com/103440043/diff/20001/ui/gtk3/panel.vala#newcod... ui/gtk3/panel.vala:346: private string[] update_im_engines(GLib.List<IBus.EngineDesc> engines) { On 2014/06/19 14:46:14, Peng wrote: > return GLib.List Done. https://codereview.appspot.com/103440043/diff/20001/ui/gtk3/panel.vala#newcod... ui/gtk3/panel.vala:346: private string[] update_im_engines(GLib.List<IBus.EngineDesc> engines) { On 2014/06/19 14:46:14, Peng wrote: > get_engines_by_local()? s/local/locale/ https://codereview.appspot.com/103440043/diff/20001/ui/gtk3/panel.vala#newcod... ui/gtk3/panel.vala:358: if (name.length >= 4 && name[0:4] == "xkb:") On 2014/06/19 14:46:14, Peng wrote: > name.has_prefix("xkb:") Done. https://codereview.appspot.com/103440043/diff/20001/ui/gtk3/panel.vala#newcod... ui/gtk3/panel.vala:366: lang = lang.split("_")[0]; On 2014/06/19 14:46:14, Peng wrote: > move it into below if {} Done. https://codereview.appspot.com/103440043/diff/20001/ui/gtk3/panel.vala#newcod... ui/gtk3/panel.vala:371: if (name.length >= 4 && name[0:4] == "xkb:") On 2014/06/19 14:46:14, Peng wrote: > has_prefix(). Done. https://codereview.appspot.com/103440043/diff/20001/ui/gtk3/xkblayout.vala File ui/gtk3/xkblayout.vala (right): https://codereview.appspot.com/103440043/diff/20001/ui/gtk3/xkblayout.vala#ne... ui/gtk3/xkblayout.vala:130: On 2014/06/19 14:46:14, Peng wrote: > Could you please add some comment? I don't know why it should return here Done. https://codereview.appspot.com/103440043/diff/20001/ui/gtk3/xkblayout.vala#ne... ui/gtk3/xkblayout.vala:138: foreach (string latin_layout in m_xkb_latin_layouts) { On 2014/06/19 14:46:14, Peng wrote: > if (variant != "eng") > need_us_layout = layout in m_xkb_latin_layouts; > if (variant != null && !need_us_layout) > need_us_layout = "%s(%s)".printf(layout, variant) in m_xkb_latin_layouts; > > ? Done.
Sign in to reply to this message.
lgtm with nits https://codereview.appspot.com/103440043/diff/1/ui/gtk3/panel.vala File ui/gtk3/panel.vala (right): https://codereview.appspot.com/103440043/diff/1/ui/gtk3/panel.vala#newcode327 ui/gtk3/panel.vala:327: GLib.List<IBus.EngineDesc> im_engines = null; On 2014/06/19 10:20:51, fujiwara wrote: > On 2014/06/18 15:34:24, Peng wrote: > > http://www.valadoc.org/#!api=glib-2.0/GLib.List > > > > should we new a List object here? > > I think the index of foreach will be confused if a member is removed during > foreach. > So I think a new list would be good. > Also the list will be sorted by IBus.EngineDesc.rank later. I mean: GLib.List<IBus.EngineDesc> im_engines = new GLib.List<..>(); https://codereview.appspot.com/103440043/diff/40001/ui/gtk3/xkblayout.vala File ui/gtk3/xkblayout.vala (right): https://codereview.appspot.com/103440043/diff/40001/ui/gtk3/xkblayout.vala#ne... ui/gtk3/xkblayout.vala:27: const string XKB_COMMAND = "setxkbmap"; Add private for all member variables? https://codereview.appspot.com/103440043/diff/40001/ui/gtk3/xkblayout.vala#ne... ui/gtk3/xkblayout.vala:166: args += "-layout"; maybe define a const variable for "-layout" ?
Sign in to reply to this message.
https://codereview.appspot.com/103440043/diff/1/ui/gtk3/panel.vala File ui/gtk3/panel.vala (right): https://codereview.appspot.com/103440043/diff/1/ui/gtk3/panel.vala#newcode327 ui/gtk3/panel.vala:327: GLib.List<IBus.EngineDesc> im_engines = null; On 2014/06/20 20:05:38, Peng wrote: > On 2014/06/19 10:20:51, fujiwara wrote: > > On 2014/06/18 15:34:24, Peng wrote: > > > http://www.valadoc.org/#!api=glib-2.0/GLib.List > > > > > > should we new a List object here? > > > > I think the index of foreach will be confused if a member is removed during > > foreach. > > So I think a new list would be good. > > Also the list will be sorted by IBus.EngineDesc.rank later. > > I mean: > > GLib.List<IBus.EngineDesc> im_engines = new GLib.List<..>(); Done. https://codereview.appspot.com/103440043/diff/40001/ui/gtk3/xkblayout.vala File ui/gtk3/xkblayout.vala (right): https://codereview.appspot.com/103440043/diff/40001/ui/gtk3/xkblayout.vala#ne... ui/gtk3/xkblayout.vala:27: const string XKB_COMMAND = "setxkbmap"; On 2014/06/20 20:05:38, Peng wrote: > Add private for all member variables? Done. https://codereview.appspot.com/103440043/diff/40001/ui/gtk3/xkblayout.vala#ne... ui/gtk3/xkblayout.vala:166: args += "-layout"; On 2014/06/20 20:05:38, Peng wrote: > maybe define a const variable for "-layout" ? Done.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
