Issue 6822102: Add Ctrl+space customization. (Closed)
|
Created:
11 years, 5 months ago by fujiwara Modified:
11 years, 4 months ago Reviewers:
shawn.p.huang CC:
shawn.p.huang_gmail.com Base URL:
git@github.com:ibus/ibus.git@master Visibility:
Public. |
DescriptionAdd Ctrl+space customization.
Patch Set 1 #Patch Set 2 : Fixed typo. #
Total comments: 5
Patch Set 3 : Updated with message #2. #
Total comments: 2
Patch Set 4 : Updated with message #6. #
Total comments: 5
Patch Set 5 : Removed for loop. #Patch Set 6 : Removed ibus-dconf dependency. #
Total comments: 2
Patch Set 7 : Updated key names. #
Total comments: 29
Patch Set 8 : Updated with message #16. #Patch Set 9 : Updated with message #19 #Patch Set 10 : Updated with the latest master. #
MessagesTotal messages: 21
https://codereview.appspot.com/6822102/diff/1007/data/ibus.schemas.in File data/ibus.schemas.in (right): https://codereview.appspot.com/6822102/diff/1007/data/ibus.schemas.in#newcode53 data/ibus.schemas.in:53: <long>The shortcut keys for turning input method on or off</long> indent should 2 spaces https://codereview.appspot.com/6822102/diff/1007/data/ibus.schemas.in#newcode57 data/ibus.schemas.in:57: <key>/schemas/desktop/ibus/general/hotkey/trigger-accel</key> Does the name trigger-accel work gconf? https://codereview.appspot.com/6822102/diff/1007/data/ibus.schemas.in#newcode65 data/ibus.schemas.in:65: <long>The shortcut keys for turning input method on or off</long> same https://codereview.appspot.com/6822102/diff/1007/data/ibus.schemas.in#newcode69 data/ibus.schemas.in:69: <key>/schemas/desktop/ibus/general/hotkey/trigger-accel-backward</key> I think we should make thing simple. Use should only set Ctrl+Space for ime switch. Users do not need add ctrl+shoft+space by themselves. It will be added automatically. https://codereview.appspot.com/6822102/diff/1007/data/ibus.schemas.in#newcode77 data/ibus.schemas.in:77: <long>The reverse shortcut keys for turning input method on or off</long> same
Sign in to reply to this message.
On 2012/11/09 15:01:26, fujiwara wrote: BTW, Did you test it with super, melt, menu modifier keys? I remembers they do not work, I tired them before.
Sign in to reply to this message.
On 2012/11/12 18:43:56, Peng wrote: > On 2012/11/09 15:01:26, fujiwara wrote: > > BTW, Did you test it with super, melt, menu modifier keys? I remembers they do > not work, I tired them before. Yes, I did. Super and Alt works fine.
Sign in to reply to this message.
On 2012/11/12 18:42:57, Peng wrote: > https://codereview.appspot.com/6822102/diff/1007/data/ibus.schemas.in > File data/ibus.schemas.in (right): > > https://codereview.appspot.com/6822102/diff/1007/data/ibus.schemas.in#newcode53 > data/ibus.schemas.in:53: <long>The shortcut keys for turning input method on or > off</long> > indent should 2 spaces OK, I modified it but I think the indent is not necessary. > > https://codereview.appspot.com/6822102/diff/1007/data/ibus.schemas.in#newcode57 > data/ibus.schemas.in:57: > <key>/schemas/desktop/ibus/general/hotkey/trigger-accel</key> > Does the name trigger-accel work gconf? I think it works fine mostly. I think it's good to change the default is ibus-dconf instead of ibus-gconf for ibus 1.5. > > https://codereview.appspot.com/6822102/diff/1007/data/ibus.schemas.in#newcode65 > data/ibus.schemas.in:65: <long>The shortcut keys for turning input method on or > off</long> > same > > https://codereview.appspot.com/6822102/diff/1007/data/ibus.schemas.in#newcode69 > data/ibus.schemas.in:69: > <key>/schemas/desktop/ibus/general/hotkey/trigger-accel-backward</key> > I think we should make thing simple. > Use should only set Ctrl+Space for ime switch. Users do not need add > ctrl+shoft+space by themselves. It will be added automatically. OK, I modified it. gnome-control-center has forward and backward trigger settings and I confirmed with GNOME people. It was implemented with the mock but no references. You said Mac OS has one setting only and it seems Windows 7/8 also have one setting only. Regarding to https://bugzilla.redhat.com/show_bug.cgi?id=845956 , I confirmed the submitter is satisfied by the feature of switcher-delay-time. So I think removing the backward setting is no problem fully. > > https://codereview.appspot.com/6822102/diff/1007/data/ibus.schemas.in#newcode77 > data/ibus.schemas.in:77: <long>The reverse shortcut keys for turning input > method on or off</long> > same
Sign in to reply to this message.
https://codereview.appspot.com/6822102/diff/8001/ui/gtk3/panel.vala File ui/gtk3/panel.vala (right): https://codereview.appspot.com/6822102/diff/8001/ui/gtk3/panel.vala#newcode155 ui/gtk3/panel.vala:155: string[] ACCELERATOR_IME_HOTKEYS = {}; The up letter variable name looks weird. Please change it. https://codereview.appspot.com/6822102/diff/8001/ui/gtk3/panel.vala#newcode160 ui/gtk3/panel.vala:160: "trigger-accel"); Why not use trigger_accel?
Sign in to reply to this message.
On 2012/11/18 00:44:44, Peng wrote: > https://codereview.appspot.com/6822102/diff/8001/ui/gtk3/panel.vala > File ui/gtk3/panel.vala (right): > > https://codereview.appspot.com/6822102/diff/8001/ui/gtk3/panel.vala#newcode155 > ui/gtk3/panel.vala:155: string[] ACCELERATOR_IME_HOTKEYS = {}; > The up letter variable name looks weird. Please change it. Done. > > https://codereview.appspot.com/6822102/diff/8001/ui/gtk3/panel.vala#newcode160 > ui/gtk3/panel.vala:160: "trigger-accel"); > Why not use trigger_accel? Now the key is referred by gconf, dconf and gsettings and gsettings support the key name rule more strictly and the name won't have to be changed even if ibus changed dconf to gsettings.
Sign in to reply to this message.
https://codereview.appspot.com/6822102/diff/16002/ui/gtk3/panel.vala File ui/gtk3/panel.vala (right): https://codereview.appspot.com/6822102/diff/16002/ui/gtk3/panel.vala#newcode160 ui/gtk3/panel.vala:160: "trigger-accel"); trigger-accel? https://codereview.appspot.com/6822102/diff/16002/ui/gtk3/panel.vala#newcode282 ui/gtk3/panel.vala:282: m_config.watch("general/hotkey", "trigger_accel"); but here is trigger_accel? https://codereview.appspot.com/6822102/diff/16002/ui/gtk3/panel.vala#newcode368 ui/gtk3/panel.vala:368: if (section == "general/hotkey" && name == "trigger_accel") { here is trigger_accel. I think we need use same name in code and schema file.
Sign in to reply to this message.
On 2012/11/18 14:48:11, Peng wrote: > https://codereview.appspot.com/6822102/diff/16002/ui/gtk3/panel.vala > File ui/gtk3/panel.vala (right): > > https://codereview.appspot.com/6822102/diff/16002/ui/gtk3/panel.vala#newcode160 > ui/gtk3/panel.vala:160: "trigger-accel"); > trigger-accel? > > https://codereview.appspot.com/6822102/diff/16002/ui/gtk3/panel.vala#newcode282 > ui/gtk3/panel.vala:282: m_config.watch("general/hotkey", "trigger_accel"); > but here is trigger_accel? > > https://codereview.appspot.com/6822102/diff/16002/ui/gtk3/panel.vala#newcode368 > ui/gtk3/panel.vala:368: if (section == "general/hotkey" && name == > "trigger_accel") { > here is trigger_accel. > I think we need use same name in code and schema file. It's a bug in ibus-dconf.
Sign in to reply to this message.
https://codereview.appspot.com/6822102/diff/16002/ui/gtk3/panel.vala File ui/gtk3/panel.vala (right): https://codereview.appspot.com/6822102/diff/16002/ui/gtk3/panel.vala#newcode368 ui/gtk3/panel.vala:368: if (section == "general/hotkey" && name == "trigger_accel") { OK. If ibus-gconf has bug, we should keep using trigger_accel, before it is fixed. And I also suggest using a global const value for the name. So we can rename it later easily. like: static const CONF_TRIGGER_ACCEL = "trigger_accel".
Sign in to reply to this message.
On 2012/11/19 15:45:44, Peng wrote: I removed a for loop in setup/main.py since enable and disable unconditional bindings does not work and config_get_values has a problem.
Sign in to reply to this message.
https://codereview.appspot.com/6822102/diff/16002/ui/gtk3/panel.vala File ui/gtk3/panel.vala (right): https://codereview.appspot.com/6822102/diff/16002/ui/gtk3/panel.vala#newcode368 ui/gtk3/panel.vala:368: if (section == "general/hotkey" && name == "trigger_accel") { On 2012/11/19 15:45:44, Peng wrote: > OK. If ibus-gconf has bug, we should keep using trigger_accel, before it is > fixed. I think it's not good to follow the bug or I wish not to integrate this feature until ibus 1.6 is available. Now I changed all values. > > And I also suggest using a global const value for the name. So we can rename it > later easily. > > like: > static const CONF_TRIGGER_ACCEL = "trigger_accel".
Sign in to reply to this message.
https://codereview.appspot.com/6822102/diff/12004/ui/gtk3/panel.vala File ui/gtk3/panel.vala (right): https://codereview.appspot.com/6822102/diff/12004/ui/gtk3/panel.vala#newcode284 ui/gtk3/panel.vala:284: m_config.watch("panel", "use_custom_font"); I don't know why you insist using a-b style name. See all configure names in ibus itself are in a_b style. I don't want to mix using them.
Sign in to reply to this message.
On 2012/11/26 16:54:51, Peng wrote: > https://codereview.appspot.com/6822102/diff/12004/ui/gtk3/panel.vala > File ui/gtk3/panel.vala (right): > > https://codereview.appspot.com/6822102/diff/12004/ui/gtk3/panel.vala#newcode284 > ui/gtk3/panel.vala:284: m_config.watch("panel", "use_custom_font"); > I don't know why you insist using a-b style name. See all configure names in > ibus itself are in a_b style. I don't want to mix using them. There are still some concerns which needs the discussion.
Sign in to reply to this message.
I updated the key name to minimize the risk. https://codereview.appspot.com/6822102/diff/12004/ui/gtk3/panel.vala File ui/gtk3/panel.vala (right): https://codereview.appspot.com/6822102/diff/12004/ui/gtk3/panel.vala#newcode284 ui/gtk3/panel.vala:284: m_config.watch("panel", "use_custom_font"); On 2012/11/26 16:54:52, Peng wrote: > I don't know why you insist using a-b style name. See all configure names in > ibus itself are in a_b style. I don't want to mix using them. Probably I think the best name could be considered in the next patch and using either '-' or '_' has the cons and pros since it refers the same key in ibus-dconf and I think to just delete '[-_]accel' to separate the naming issue from this patch and it could satisfy the name rule in gconf, dconf and gsettings. I think the 'triggers' would be good to close this patch and I'm ok if the name will be revised by the next patch. Probably I think the best name could be several options.
Sign in to reply to this message.
https://codereview.appspot.com/6822102/diff/22003/setup/keyboardshortcut.py File setup/keyboardshortcut.py (right): https://codereview.appspot.com/6822102/diff/22003/setup/keyboardshortcut.py#n... setup/keyboardshortcut.py:193: keys = shortcut.split('>') Use rsplit please https://codereview.appspot.com/6822102/diff/22003/setup/keyboardshortcut.py#n... setup/keyboardshortcut.py:250: out = [dlg] ? https://codereview.appspot.com/6822102/diff/22003/setup/keyboardshortcut.py#n... setup/keyboardshortcut.py:257: dlg = out[0] I think you may use dlg and out values in outter function directly https://codereview.appspot.com/6822102/diff/22003/setup/keyboardshortcut.py#n... setup/keyboardshortcut.py:300: keys = shortcut.split('>') Please use rsplit https://codereview.appspot.com/6822102/diff/22003/setup/main.py File setup/main.py (right): https://codereview.appspot.com/6822102/diff/22003/setup/main.py#newcode87 setup/main.py:87: label = 'next_engine' Could we change them to switch_engine https://codereview.appspot.com/6822102/diff/22003/setup/main.py#newcode96 setup/main.py:96: entry.set_text("; ".join(shortcuts)) Do we really need support multiple hot keys ? May be one is enough? https://codereview.appspot.com/6822102/diff/22003/setup/main.py#newcode99 setup/main.py:99: _("Shift key switches to the previous input method") Use shortcut with shift to switch to the previous engine. https://codereview.appspot.com/6822102/diff/22003/setup/main.py#newcode379 setup/main.py:379: title = _("Select keyboard shortcut for %s") % _("next engine") Switch engine https://codereview.appspot.com/6822102/diff/22003/setup/main.py#newcode397 setup/main.py:397: _("Shift key switches to the previous input method") Same https://codereview.appspot.com/6822102/diff/22003/setup/setup.ui File setup/setup.ui (left): https://codereview.appspot.com/6822102/diff/22003/setup/setup.ui#oldcode124 setup/setup.ui:124: <property name="visible">True</property> Did you remove all unused shortcuts? https://codereview.appspot.com/6822102/diff/22003/ui/gtk3/panel.vala File ui/gtk3/panel.vala (right): https://codereview.appspot.com/6822102/diff/22003/ui/gtk3/panel.vala#newcode189 ui/gtk3/panel.vala:189: m_keybindings.remove(keybinding); It is better reset the array out of the while loop https://codereview.appspot.com/6822102/diff/22003/ui/gtk3/switcher.vala File ui/gtk3/switcher.vala (right): https://codereview.appspot.com/6822102/diff/22003/ui/gtk3/switcher.vala#newco... ui/gtk3/switcher.vala:66: private class Keybinding { This is duplicated code. Could we share it https://codereview.appspot.com/6822102/diff/22003/ui/gtk3/switcher.vala#newco... ui/gtk3/switcher.vala:125: public int run(GLib.List keybindings, I think we could make it simple. It is not necessary to use a list here. We should only accept the initial shortcut, not all shortcuts.
Sign in to reply to this message.
https://codereview.appspot.com/6822102/diff/22003/setup/keyboardshortcut.py File setup/keyboardshortcut.py (right): https://codereview.appspot.com/6822102/diff/22003/setup/keyboardshortcut.py#n... setup/keyboardshortcut.py:193: keys = shortcut.split('>') On 2012/11/29 13:26:44, Peng wrote: > Use rsplit please I guess you mean rsplit('>')[0] might return the keycode. But in my test, both split('>') and rsplit('>') returns the same value and need to get the last index rsplit('>')[-1]. Maybe it's a bug in python? https://codereview.appspot.com/6822102/diff/22003/setup/keyboardshortcut.py#n... setup/keyboardshortcut.py:257: dlg = out[0] On 2012/11/29 13:26:44, Peng wrote: > I think you may use dlg and out values in outter function directly Probably I enhanced this. https://codereview.appspot.com/6822102/diff/22003/setup/keyboardshortcut.py#n... setup/keyboardshortcut.py:300: keys = shortcut.split('>') On 2012/11/29 13:26:44, Peng wrote: > Please use rsplit Same above. I'm not sure about the difference between split and rsplit. https://codereview.appspot.com/6822102/diff/22003/setup/main.py File setup/main.py (right): https://codereview.appspot.com/6822102/diff/22003/setup/main.py#newcode87 setup/main.py:87: label = 'next_engine' On 2012/11/29 13:26:44, Peng wrote: > Could we change them to switch_engine Done. https://codereview.appspot.com/6822102/diff/22003/setup/main.py#newcode96 setup/main.py:96: entry.set_text("; ".join(shortcuts)) On 2012/11/29 13:26:44, Peng wrote: > Do we really need support multiple hot keys ? May be one is enough? Currently I assigns Zenkaku_Hankaku and '<Control>space'. https://codereview.appspot.com/6822102/diff/22003/setup/main.py#newcode99 setup/main.py:99: _("Shift key switches to the previous input method") On 2012/11/29 13:26:44, Peng wrote: > Use shortcut with shift to switch to the previous engine. Probably I think 'input method' is better than 'engine'. https://codereview.appspot.com/6822102/diff/22003/setup/main.py#newcode379 setup/main.py:379: title = _("Select keyboard shortcut for %s") % _("next engine") On 2012/11/29 13:26:44, Peng wrote: > Switch engine I think 'Switch engine' might mean that engine switches something by mistake while actually it means 'to switch engines'. I put 'switching input methods' here. https://codereview.appspot.com/6822102/diff/22003/setup/setup.ui File setup/setup.ui (left): https://codereview.appspot.com/6822102/diff/22003/setup/setup.ui#oldcode124 setup/setup.ui:124: <property name="visible">True</property> On 2012/11/29 13:26:44, Peng wrote: > Did you remove all unused shortcuts? I hid some unused shortcuts. Probably I think the previous engine key is not used. I'm not sure about your question. https://codereview.appspot.com/6822102/diff/22003/ui/gtk3/panel.vala File ui/gtk3/panel.vala (right): https://codereview.appspot.com/6822102/diff/22003/ui/gtk3/panel.vala#newcode189 ui/gtk3/panel.vala:189: m_keybindings.remove(keybinding); On 2012/11/29 13:26:44, Peng wrote: > It is better reset the array out of the while loop Done. https://codereview.appspot.com/6822102/diff/22003/ui/gtk3/switcher.vala File ui/gtk3/switcher.vala (right): https://codereview.appspot.com/6822102/diff/22003/ui/gtk3/switcher.vala#newco... ui/gtk3/switcher.vala:66: private class Keybinding { On 2012/11/29 13:26:44, Peng wrote: > This is duplicated code. Could we share it OK, I removed this structure in switcher.vala. https://codereview.appspot.com/6822102/diff/22003/ui/gtk3/switcher.vala#newco... ui/gtk3/switcher.vala:125: public int run(GLib.List keybindings, On 2012/11/29 13:26:44, Peng wrote: > I think we could make it simple. It is not necessary to use a list here. We > should only accept the initial shortcut, not all shortcuts. Done.
Sign in to reply to this message.
https://codereview.appspot.com/6822102/diff/22003/setup/keyboardshortcut.py File setup/keyboardshortcut.py (right): https://codereview.appspot.com/6822102/diff/22003/setup/keyboardshortcut.py#n... setup/keyboardshortcut.py:193: keys = shortcut.split('>') On 2012/12/03 07:40:50, fujiwara wrote: > On 2012/11/29 13:26:44, Peng wrote: > > Use rsplit please > > I guess you mean rsplit('>')[0] might return the keycode. > But in my test, both split('>') and rsplit('>') returns the same value and need > to get the last index rsplit('>')[-1]. > Maybe it's a bug in python? At here, we just need the keycode. So we could use rsplit('>', 1)[-1]. It is littler efficient. https://codereview.appspot.com/6822102/diff/22003/setup/keyboardshortcut.py#n... setup/keyboardshortcut.py:257: dlg = out[0] On 2012/12/03 07:40:50, fujiwara wrote: > On 2012/11/29 13:26:44, Peng wrote: > > I think you may use dlg and out values in outter function directly > > Probably I enhanced this. I meant (If I remember it correctly) the inner function can access variables in outter function directly. So you do not need pass them as arguments. Could you please try use them directly. https://codereview.appspot.com/6822102/diff/22003/setup/keyboardshortcut.py#n... setup/keyboardshortcut.py:300: keys = shortcut.split('>') On 2012/12/03 07:40:50, fujiwara wrote: > On 2012/11/29 13:26:44, Peng wrote: > > Please use rsplit > > Same above. I'm not sure about the difference between split and rsplit. rsplit is split from right side. In this case, we only need the last part. So we could use rsplit('>', 1)[-1].
Sign in to reply to this message.
https://codereview.appspot.com/6822102/diff/22003/setup/keyboardshortcut.py File setup/keyboardshortcut.py (right): https://codereview.appspot.com/6822102/diff/22003/setup/keyboardshortcut.py#n... setup/keyboardshortcut.py:193: keys = shortcut.split('>') On 2012/12/03 13:25:23, Peng wrote: > On 2012/12/03 07:40:50, fujiwara wrote: > > On 2012/11/29 13:26:44, Peng wrote: > > > Use rsplit please > > > > I guess you mean rsplit('>')[0] might return the keycode. > > But in my test, both split('>') and rsplit('>') returns the same value and > need > > to get the last index rsplit('>')[-1]. > > Maybe it's a bug in python? > > At here, we just need the keycode. > So we could use rsplit('>', 1)[-1]. It is littler efficient. > OK, modified. https://codereview.appspot.com/6822102/diff/22003/setup/keyboardshortcut.py#n... setup/keyboardshortcut.py:257: dlg = out[0] On 2012/12/03 13:25:23, Peng wrote: > On 2012/12/03 07:40:50, fujiwara wrote: > > On 2012/11/29 13:26:44, Peng wrote: > > > I think you may use dlg and out values in outter function directly > > > > Probably I enhanced this. > > I meant (If I remember it correctly) the inner function can access variables in > outter function directly. So you do not need pass them as arguments. Could you > please try use them directly. > OK, you meant __accel_edited_cb() is the inner function. I reverted my change.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
