Handle XK_Zenkaku_Hankaku, Henkan, Muhenkan, Hangul etc. in Chrome

chrome/browser/chromeos/input_method/input_method_manager.cc

Index: chrome/browser/chromeos/input_method/input_method_manager.cc
diff --git a/chrome/browser/chromeos/input_method/input_method_manager.cc b/chrome/browser/chromeos/input_method/input_method_manager.cc
index 5cf84c749da8e03d55865d2cf461dc418e2e90ef..7b3ce8e9538f51475060c2f600e703963432596c 100644
--- a/chrome/browser/chromeos/input_method/input_method_manager.cc
+++ b/chrome/browser/chromeos/input_method/input_method_manager.cc
@@ -42,9 +42,58 @@ namespace {
 const char kIBusDaemonPath[] = "/usr/bin/ibus-daemon";
 
 // For hotkey handling.
-enum HotkeyEvents {
+enum HotkeyEvent {
   kPreviousInputMethod = 0,
   kNextInputMethod,
+  kJapaneseInputMethod,
+  kJapaneseLayout,
+  kJapaneseInputMethodOrLayout,
+  kKoreanInputMethodOrLayout,
+};
+
+// Engine specific hotkeys.
+const struct EngineSpecificHotkeySetting {
+  const char* input_method_ids[2];
+  int event_id;
+  KeySym keysym;
+  uint32 modifiers;
+  bool trigger_on_press;

[mazda, 2011/09/12 10:02:54]

What happens if trigger_on_press is false?

[Yusuke Sato, 2011/09/12 11:46:05]

IME or layout is changed on key release. The Shift+Alt hotkey uses "trigger on
release" not to interfere with other hotkeys like Shift+Alt+Tab.

[mazda, 2011/09/12 11:57:31]

Thanks.
Probably it's better to add comment since the opposite of 'trigger on press' can
mean anything else like 'do not trigger on press'.

On 2011/09/12 11:46:05, Yusuke Sato wrote:

[Yusuke Sato, 2011/09/12 12:02:14]

Done.

+} kEngineSpecificHotkeySettings[] = {
+  {
+    { "mozc-jp" },
+    kJapaneseInputMethod,
+    XK_Henkan,
+    0x0,
+    true,
+  },
+  {
+    { "xkb:jp::jpn" },
+    kJapaneseLayout,
+    XK_Muhenkan,
+    0x0,
+    true,
+  },
+  {
+    { "mozc-jp", "xkb:jp::jpn" },
+    kJapaneseInputMethodOrLayout,
+    XK_Zenkaku_Hankaku,
+    0x0,
+    true,
+  },
+  {
+    { "mozc-hangul", "xkb:kr:kr104:kor" },
+    kKoreanInputMethodOrLayout,
+    XK_Hangul,
+    0x0,
+    true,
+  },
+  {
+    { "mozc-hangul", "xkb:kr:kr104:kor" },
+    kKoreanInputMethodOrLayout,
+    XK_space,
+    ShiftMask,
+    true,
+  },
 };
 
 // Finds a property which has |new_prop.key| from |prop_list|, and replaces the
@@ -104,6 +153,19 @@ class InputMethodManagerImpl : public HotkeyManager::Observer,
     ibus_controller_->AddObserver(this);
     ibus_controller_->Connect();
 
+    // Initialize engine_hotkeys_.
+    for (size_t i = 0; i < arraysize(kEngineSpecificHotkeySettings); ++i) {
+      const char* const* input_method_ids =
+          kEngineSpecificHotkeySettings[i].input_method_ids;
+      const size_t input_method_ids_len =
+          arraysize(kEngineSpecificHotkeySettings[i].input_method_ids);
+      for (size_t j = 0; j < input_method_ids_len && input_method_ids[j]; ++j) {
+        engine_hotkeys_.insert(std::make_pair(
+            input_method_ids[j], &kEngineSpecificHotkeySettings[i]));
+      }
+    }
+
+    // Register global input method hotkeys: Control+space and Shift+Alt.
     InitHotkeyManager();
   }
 
@@ -223,6 +285,7 @@ class InputMethodManagerImpl : public HotkeyManager::Observer,
         config_name == language_prefs::kPreloadEnginesConfigName &&
         value.type == ImeConfigValue::kValueTypeStringList) {
       active_input_method_ids_ = value.string_list_value;
+      UpdateEngineSpecificHotkeys();

[mazda, 2011/09/12 10:02:54]

Is it OK that UpdateEngineSpecificHotkeys is called before the IDs in
extra_input_methods_ids_ are added to active_input_method_ids_?

[Yusuke Sato, 2011/09/12 11:46:05]

For now it's okay since hotkey information for an IME extension is not passed to
this class. I'll improve the class so IME extensions could support hotkeys.
Added some TODOs.

 
       std::map<std::string, InputMethodDescriptor>::const_iterator ix;
       for (ix = extra_input_method_ids_.begin();
@@ -349,16 +412,21 @@ class InputMethodManagerImpl : public HotkeyManager::Observer,
   }
 
   virtual void HotkeyPressed(HotkeyManager* manager, int event_id) {
-    switch (HotkeyEvents(event_id)) {
+    const HotkeyEvent event = HotkeyEvent(event_id);
+    switch (event) {
       case kPreviousInputMethod:
         SwitchToPreviousInputMethod();
         break;
       case kNextInputMethod:
         SwitchToNextInputMethod();
         break;
+      case kJapaneseInputMethod:
+      case kJapaneseLayout:
+      case kJapaneseInputMethodOrLayout:
+      case kKoreanInputMethodOrLayout:
+        SwitchToInputMethod(event);

[mazda, 2011/09/12 10:02:54]

default case.

[Yusuke Sato, 2011/09/12 11:46:05]

Since HotkeyEvent is an enum, adding default: is not necessary (and has negative
effect - it disables gcc's warning capability.)

+        break;
     }
-    // TODO(yusukes): handle engine specific hotkeys like Henkan, Muhenkan, and
-    // Hangul.
   }
 
   static InputMethodManagerImpl* GetInstance() {
@@ -961,12 +1029,30 @@ class InputMethodManagerImpl : public HotkeyManager::Observer,
                               XK_Meta_R,
                               ShiftMask | Mod1Mask,
                               false);
+    // Engine specific hotkeys will be added in SetImeConfig().
 
-    // TODO(yusukes): Support engine specific hotkeys like XK_Henkan, Muhenkan,
-    // and Hangul.
     hotkey_manager_.AddObserver(this);
   }
 
+  void UpdateEngineSpecificHotkeys() {
+    // Remove all engine specific hotkeys first.
+    for (size_t i = 0; i < arraysize(kEngineSpecificHotkeySettings); ++i) {
+      hotkey_manager_.RemoveHotkey(kEngineSpecificHotkeySettings[i].event_id);
+    }
+    for (size_t i = 0; i < active_input_method_ids_.size(); ++i) {
+      typedef std::map<std::string,
+          const EngineSpecificHotkeySetting*>::const_iterator Iter;
+      std::pair<Iter, Iter> result = engine_hotkeys_.equal_range(
+          active_input_method_ids_[i]);
+      for (Iter iter = result.first; iter != result.second; ++iter) {
+        hotkey_manager_.AddHotkey(iter->second->event_id,
+                                  iter->second->keysym,
+                                  iter->second->modifiers,
+                                  iter->second->trigger_on_press);
+      }
+    }
+  }
+
   // Handles "Shift+Alt" hotkey.
   void SwitchToNextInputMethod() {
     // Sanity checks.
@@ -1019,6 +1105,72 @@ class InputMethodManagerImpl : public HotkeyManager::Observer,
     }
   }
 
+  // Handles engine specific hotkeys like XK_Henkan.
+  void SwitchToInputMethod(HotkeyEvent event) {
+    // Sanity check.
+    if (active_input_method_ids_.empty()) {
+      LOG(ERROR) << "active input method is empty";
+      return;
+    }
+
+    const char* const* input_method_ids_to_switch = NULL;
+    size_t input_method_ids_to_switch_len = 0;
+
+    // Get the list of input method ids for |event_id|. For example, get
+    // { "mozc-hangul", "xkb:kr:kr104:kor" } for kKoreanInputMethodOrLayout.
+    switch (event) {
+      case kPreviousInputMethod:
+      case kNextInputMethod: {
+        break;

[mazda, 2011/09/12 10:02:54]

Are these two error case?

[Yusuke Sato, 2011/09/12 11:46:05]

added a comment.

+      }
+      case kJapaneseInputMethod:
+      case kJapaneseLayout:
+      case kJapaneseInputMethodOrLayout:
+      case kKoreanInputMethodOrLayout: {
+        const size_t index = static_cast<size_t>(event) - 2;

[Zachary Kuznia, 2011/09/12 09:48:07]

This line looks scary.  Please give it a reassuring comment.

[Yusuke Sato, 2011/09/12 11:46:05]

Probably this was too error-prone. I've written this in a safer way (using for
loop). Thanks.

+        DCHECK(event == kEngineSpecificHotkeySettings[index].event_id);
+        input_method_ids_to_switch =
+            kEngineSpecificHotkeySettings[index].input_method_ids;
+        input_method_ids_to_switch_len =
+            arraysize(kEngineSpecificHotkeySettings[index].input_method_ids);
+        break;
+      }
+    }
+    if (!input_method_ids_to_switch) {

[mazda, 2011/09/12 10:02:54]

How about moving this to the default case of the switch statement?

[Yusuke Sato, 2011/09/12 11:46:05]

ditto. I prefer not to use default: in order to keep gcc's warning capability
enabled.

+      LOG(ERROR) << "Unknown event: " << event;
+      return;
+    }
+
+    // Obtain the intersection of input_method_ids_to_switch and
+    // active_input_method_ids_. The order of IDs in active_input_method_ids_ is
+    // preserved.
+    std::vector<std::string> ids;
+    for (size_t i = 0; i < input_method_ids_to_switch_len; ++i) {
+      const char* id = input_method_ids_to_switch[i];
+      if (id && std::find(active_input_method_ids_.begin(),
+                          active_input_method_ids_.end(),
+                          id) != active_input_method_ids_.end()) {
+        ids.push_back(id);
+      }
+    }
+    if (ids.empty()) {
+      LOG(ERROR) << "No input method for " << event << " is active";
+      return;
+    }
+
+    // If current_input_method_ is not in ids, switch to ids[0]. If
+    // current_input_method_ is ids[N], switch to ids[N+1].
+    std::vector<std::string>::const_iterator iter =
+        std::find(ids.begin(), ids.end(), current_input_method_.id());
+    if (iter != ids.end()) {
+      ++iter;
+    }
+    if (iter == ids.end()) {
+      iter = ids.begin();
+    }
+    ChangeInputMethod(*iter);
+  }
+
   // A reference to the language api, to allow callbacks when the input method
   // status changes.
   IBusController* ibus_controller_;
@@ -1088,6 +1240,10 @@ class InputMethodManagerImpl : public HotkeyManager::Observer,
   // those created by extension.
   std::map<std::string, InputMethodDescriptor> extra_input_method_ids_;
 
+  // A muitlmap from input method id to engine specific hotkey information.
+  // e.g. "mozc-jp" to XK_ZenkakuHankaku, "mozc-jp" to XK_Henkan.
+  std::multimap<std::string,
+                const EngineSpecificHotkeySetting*> engine_hotkeys_;
   // An object which detects Control+space and Shift+Alt key presses.
   HotkeyManager hotkey_manager_;