Stop working around a limitation in GTK IM-module.

There is a long standing limitation in GTK IM-module that IMEs cannot retrieve the screen coordinates of each composing character, which is definitely needed to align suggestion window to the left edge of composing text. In ibus-mozc, we have worked around this limitation by recording the cursor rectangle in the mozc server rather and simulating the character screen coordinates from those cursor rectangles since OSS Mozc 1.3.911.102 (a1fae21). This emulation has, however, never been perfect. Following issues are actually edge cases of the emulation. - #243: ibus predict window is shown at the previous cursor position - https://bugzilla.mozilla.org/show_bug.cgi?id=1120851 Therefore we decided to remove the above emulation from ibus-mozc and live in more robust but unsophisticated world instead. With this CL, the suggestion window will show up just under the cursor location rather than being aligned with composing text. This clean-up also enables us to refactor mozc-server without bothering future ibus-mozc maintainers because that emulation code that is implemented in mozc-server. In subsequent CLs we can remove the emulation code without breaking existing ibus-mozc client. Closes #243. BUG=#243 TEST=manually done on Ubuntu 14.04.
google · Aug 15, 2015 · 9fbcdd5 · 9fbcdd5
1 parent 0796f51
commit 9fbcdd5
Show file tree

Hide file tree

Showing 11 changed files with 164 additions and 83 deletions.
diff --git a/src/mozc_version_template.txt b/src/mozc_version_template.txt
@@ -1,6 +1,6 @@
 MAJOR=2
 MINOR=17
-BUILD=2108
+BUILD=2109
 REVISION=102
 # NACL_DICTIONARY_VERSION is the target version of the system dictionary to be
 # downloaded by NaCl Mozc.

diff --git a/src/renderer/unix/window_manager.cc b/src/renderer/unix/window_manager.cc
@@ -101,27 +101,19 @@ Rect WindowManager::UpdateCandidateWindow(
   const Size new_window_size = candidate_window_->Update(candidates);
 
   Point new_window_pos = candidate_window_->GetWindowPos();
-  if (candidates.has_window_location()) {
-    if (candidates.window_location() == commands::Candidates::CARET) {
-      DCHECK(candidates.has_caret_rectangle());
-      new_window_pos.x = candidates.caret_rectangle().x();
-      new_window_pos.y = candidates.caret_rectangle().y()
-          + candidates.caret_rectangle().height();
-    } else {
-      DCHECK(candidates.has_composition_rectangle());
-      new_window_pos.x = candidates.composition_rectangle().x();
-      new_window_pos.y = candidates.composition_rectangle().y()
-          + candidates.composition_rectangle().height();
-    }
+  if (command.has_preedit_rectangle()) {
+    new_window_pos.x = command.preedit_rectangle().left();
+    new_window_pos.y = command.preedit_rectangle().bottom();
   }
 
   const Rect working_area = GetMonitorRect(new_window_pos.x, new_window_pos.y);
   const Point alignment_base_point_in_local_window_coord(
       candidate_window_->GetCandidateColumnInClientCord().Left(), 0);
-  const Rect caret_rect(candidates.caret_rectangle().x(),
-                              candidates.caret_rectangle().y(),
-                              candidates.caret_rectangle().width(),
-                              candidates.caret_rectangle().height());
+  const auto &preedit_rect = command.preedit_rectangle();
+  const Rect caret_rect(preedit_rect.left(),
+                        preedit_rect.top(),
+                        preedit_rect.right() - preedit_rect.left(),
+                        preedit_rect.bottom() - preedit_rect.top());
   // |caret_rect| is not always equal to preedit rect but can be an alternative
   // in terms of positional calculation, especially for vertical adjustment in
   // horizontal writing.

diff --git a/src/renderer/unix/window_manager_test.cc b/src/renderer/unix/window_manager_test.cc
@@ -331,13 +331,12 @@ TEST(WindowManagerTest, UpdateCandidateWindowTest) {
     const Size window_size(35, 45);
     const gint monitor = 0x7777;
 
-    candidates->set_window_location(commands::Candidates::CARET);
     const Rect caret_rect(16, 26, 2, 13);
-    commands::Rectangle *rectangle = candidates->mutable_caret_rectangle();
-    rectangle->set_x(caret_rect.Left());
-    rectangle->set_y(caret_rect.Top());
-    rectangle->set_width(caret_rect.Width());
-    rectangle->set_height(caret_rect.Height());
+    auto *rectangle = command.mutable_preedit_rectangle();
+    rectangle->set_left(caret_rect.Left());
+    rectangle->set_top(caret_rect.Top());
+    rectangle->set_right(caret_rect.Right());
+    rectangle->set_bottom(caret_rect.Bottom());
     const Point expected_window_position(
         caret_rect.Left() - client_cord_rect.Left(),
         caret_rect.Top() + caret_rect.Height());
@@ -400,14 +399,12 @@ TEST(WindowManagerTest, UpdateCandidateWindowTest) {
     const Size window_size(35, 45);
     const gint monitor = 0x7777;
 
-    candidates->set_window_location(commands::Candidates::COMPOSITION);
     const Rect comp_rect(16, 26, 2, 13);
-    commands::Rectangle *rectangle
-        = candidates->mutable_composition_rectangle();
-    rectangle->set_x(comp_rect.Left());
-    rectangle->set_y(comp_rect.Top());
-    rectangle->set_width(comp_rect.Width());
-    rectangle->set_height(comp_rect.Height());
+    auto *rectangle = command.mutable_preedit_rectangle();
+    rectangle->set_left(comp_rect.Left());
+    rectangle->set_top(comp_rect.Top());
+    rectangle->set_right(comp_rect.Right());
+    rectangle->set_bottom(comp_rect.Bottom());
     const Point expected_window_position(
         comp_rect.Left() - client_cord_rect.Left(),
         comp_rect.Top() + comp_rect.Height());

diff --git a/src/unix/ibus/candidate_window_handler_interface.h b/src/unix/ibus/candidate_window_handler_interface.h
@@ -49,6 +49,10 @@ class CandidateWindowHandlerInterface {
   virtual void Update(IBusEngine *engine,
                       const commands::Output &output) = 0;
 
+  // Updates candidate state. This function also shows or hides candidate window
+  // based on the last |Update| call.
+  virtual void UpdateCursorRect(IBusEngine *engine) = 0;
+
   // Hides candidate window.
   virtual void Hide(IBusEngine *engine) = 0;
 

diff --git a/src/unix/ibus/gtk_candidate_window_handler.cc b/src/unix/ibus/gtk_candidate_window_handler.cc
@@ -54,16 +54,25 @@ GtkCandidateWindowHandler::~GtkCandidateWindowHandler() {
 }
 
 bool GtkCandidateWindowHandler::SendUpdateCommand(
-    const commands::Output &output, bool visibility) const {
+    IBusEngine *engine,
+    const commands::Output &output,
+    bool visibility) const {
   using commands::RendererCommand;
-
   RendererCommand command;
-  command.mutable_output()->CopyFrom(output);
-  command.set_type(commands::RendererCommand::UPDATE);
+
+  *command.mutable_output() = output;
+  command.set_type(RendererCommand::UPDATE);
   command.set_visible(visibility);
   RendererCommand::ApplicationInfo *appinfo
       = command.mutable_application_info();
 
+  auto *preedit_rectangle = command.mutable_preedit_rectangle();
+  const auto &cursor_area = engine->cursor_area;
+  preedit_rectangle->set_left(cursor_area.x);
+  preedit_rectangle->set_top(cursor_area.y);
+  preedit_rectangle->set_right(cursor_area.x + cursor_area.width);
+  preedit_rectangle->set_bottom(cursor_area.y + cursor_area.height);
+
   // Set pid
   static_assert(sizeof(::getpid()) <= sizeof(appinfo->process_id()),
                 "|appinfo->process_id()| must have sufficient room.");
@@ -83,17 +92,24 @@ bool GtkCandidateWindowHandler::SendUpdateCommand(
 
 void GtkCandidateWindowHandler::Update(IBusEngine *engine,
                                        const commands::Output &output) {
-  last_update_output_->CopyFrom(output);
+  *last_update_output_ = output;
+
+  UpdateCursorRect(engine);
+}
 
-  SendUpdateCommand(output, output.candidates().candidate_size() != 0);
+void GtkCandidateWindowHandler::UpdateCursorRect(IBusEngine *engine) {
+  const bool has_candidates =
+      last_update_output_->has_candidates() &&
+      last_update_output_->candidates().candidate_size() > 0;
+  SendUpdateCommand(engine, *last_update_output_, has_candidates);
 }
 
 void GtkCandidateWindowHandler::Hide(IBusEngine *engine) {
-  SendUpdateCommand(*(last_update_output_.get()), false);
+  SendUpdateCommand(engine, *last_update_output_, false);
 }
 
 void GtkCandidateWindowHandler::Show(IBusEngine *engine) {
-  SendUpdateCommand(*(last_update_output_.get()), true);
+  SendUpdateCommand(engine, *last_update_output_, true);
 }
 
 void GtkCandidateWindowHandler::OnIBusCustomFontDescriptionChanged(

diff --git a/src/unix/ibus/gtk_candidate_window_handler.h b/src/unix/ibus/gtk_candidate_window_handler.h
@@ -36,6 +36,9 @@
 #include "unix/ibus/candidate_window_handler_interface.h"
 
 namespace mozc {
+namespace commands {
+class RendererCommand;
+}  // namespace commands
 namespace renderer {
 class RendererInterface;
 }  // namespace renderer
@@ -48,6 +51,7 @@ class GtkCandidateWindowHandler : public CandidateWindowHandlerInterface {
   virtual ~GtkCandidateWindowHandler();
 
   virtual void Update(IBusEngine *engine, const commands::Output &output);
+  virtual void UpdateCursorRect(IBusEngine *engine);
   virtual void Hide(IBusEngine *engine);
   virtual void Show(IBusEngine *engine);
 
@@ -58,7 +62,9 @@ class GtkCandidateWindowHandler : public CandidateWindowHandlerInterface {
       bool use_custom_font_description);
 
  protected:
-  bool SendUpdateCommand(const commands::Output &output, bool visibility) const;
+  bool SendUpdateCommand(IBusEngine *engine,
+                         const commands::Output &output,
+                         bool visibility) const;
 
   std::unique_ptr<renderer::RendererInterface> renderer_;
   std::unique_ptr<commands::Output> last_update_output_;

diff --git a/src/unix/ibus/gtk_candidate_window_handler_test.cc b/src/unix/ibus/gtk_candidate_window_handler_test.cc
@@ -31,6 +31,7 @@
 
 #include <unistd.h>  // for getpid()
 
+#include "base/coordinates.h"
 #include "protocol/renderer_command.pb.h"
 #include "renderer/renderer_mock.h"
 #include "testing/base/public/gmock.h"
@@ -110,6 +111,40 @@ MATCHER_P(VisibilityEq, visibility, "") {
   return true;
 }
 
+MATCHER_P(PreeditRectangleEq, rect, "") {
+  if (!arg.has_preedit_rectangle()) {
+    *result_listener
+        << "RendererCommand::preedit_rectangle does not exist";
+    return false;
+  }
+  const auto &actual_rect = arg.preedit_rectangle();
+  if (rect.Left() != actual_rect.left()) {
+    *result_listener << "left field does not match\n"
+                     << "  expected: " << rect.Left() << "\n"
+                     << "  actual:   " << actual_rect.left();
+    return false;
+  }
+  if (rect.Top() != actual_rect.top()) {
+    *result_listener << "top field does not match\n"
+                     << "  expected: " << rect.Top() << "\n"
+                     << "  actual:   " << actual_rect.top();
+    return false;
+  }
+  if (rect.Right() != actual_rect.right()) {
+    *result_listener << "right field does not match\n"
+                     << "  expected: " << rect.Right() << "\n"
+                     << "  actual:   " << actual_rect.right();
+    return false;
+  }
+  if (rect.Bottom() != actual_rect.bottom()) {
+    *result_listener << "bottom field does not match\n"
+                     << "  expected: " << rect.Bottom() << "\n"
+                     << "  actual:   " << actual_rect.bottom();
+    return false;
+  }
+  return true;
+}
+
 MATCHER_P(OutputEq, expected, "") {
   if (expected.Utf8DebugString() != arg.Utf8DebugString()) {
     *result_listener
@@ -129,47 +164,73 @@ MATCHER_P(OutputEq, expected, "") {
 }  // namespace
 
 TEST(GtkCandidateWindowHandlerTest, SendUpdateCommandTest) {
+  const Rect kExpectedCursorArea(10, 20, 200, 100);
+
+  IBusEngine engine = {};
+  engine.cursor_area.x = kExpectedCursorArea.Left();
+  engine.cursor_area.y = kExpectedCursorArea.Top();
+  engine.cursor_area.width = kExpectedCursorArea.Width();
+  engine.cursor_area.height = kExpectedCursorArea.Height();
+
   {
     SCOPED_TRACE("visibility check. false case");
     Output output;
     RendererMock *renderer_mock = new RendererMock();
     TestableGtkCandidateWindowHandler gtk_candidate_window_handler(
         renderer_mock);
-    EXPECT_CALL_EXEC_COMMAND(*renderer_mock, VisibilityEq(false));
-    gtk_candidate_window_handler.SendUpdateCommand(output, false);
+    EXPECT_CALL_EXEC_COMMAND(*renderer_mock,
+                             VisibilityEq(false),
+                             PreeditRectangleEq(kExpectedCursorArea));
+    gtk_candidate_window_handler.SendUpdateCommand(&engine, output, false);
   }
   {
     SCOPED_TRACE("visibility check. true case");
     Output output;
     RendererMock *renderer_mock = new RendererMock();
     TestableGtkCandidateWindowHandler gtk_candidate_window_handler(
         renderer_mock);
-    EXPECT_CALL_EXEC_COMMAND(*renderer_mock, VisibilityEq(true));
-    gtk_candidate_window_handler.SendUpdateCommand(output, true);
+    EXPECT_CALL_EXEC_COMMAND(*renderer_mock,
+                             VisibilityEq(true),
+                             PreeditRectangleEq(kExpectedCursorArea));
+    gtk_candidate_window_handler.SendUpdateCommand(&engine, output, true);
   }
   {
     SCOPED_TRACE("return value check. false case.");
     Output output;
     RendererMock *renderer_mock = new RendererMock();
     TestableGtkCandidateWindowHandler gtk_candidate_window_handler(
         renderer_mock);
-    EXPECT_CALL_EXEC_COMMAND(*renderer_mock, VisibilityEq(true))
+    EXPECT_CALL_EXEC_COMMAND(*renderer_mock,
+                             VisibilityEq(true),
+                             PreeditRectangleEq(kExpectedCursorArea))
         .WillOnce(Return(false));
-    EXPECT_FALSE(gtk_candidate_window_handler.SendUpdateCommand(output, true));
+    EXPECT_FALSE(gtk_candidate_window_handler.SendUpdateCommand(
+        &engine, output, true));
   }
   {
     SCOPED_TRACE("return value check. true case.");
     Output output;
     RendererMock *renderer_mock = new RendererMock();
     TestableGtkCandidateWindowHandler gtk_candidate_window_handler(
         renderer_mock);
-    EXPECT_CALL_EXEC_COMMAND(*renderer_mock, VisibilityEq(true))
+    EXPECT_CALL_EXEC_COMMAND(*renderer_mock,
+                             VisibilityEq(true),
+                             PreeditRectangleEq(kExpectedCursorArea))
         .WillOnce(Return(true));
-    EXPECT_TRUE(gtk_candidate_window_handler.SendUpdateCommand(output, true));
+    EXPECT_TRUE(gtk_candidate_window_handler.SendUpdateCommand(
+        &engine, output, true));
   }
 }
 
 TEST(GtkCandidateWindowHandlerTest, UpdateTest) {
+  const Rect kExpectedCursorArea(10, 20, 200, 100);
+
+  IBusEngine engine = {};
+  engine.cursor_area.x = kExpectedCursorArea.Left();
+  engine.cursor_area.y = kExpectedCursorArea.Top();
+  engine.cursor_area.width = kExpectedCursorArea.Width();
+  engine.cursor_area.height = kExpectedCursorArea.Height();
+
   const int sample_idx1 = 0;
   const int sample_idx2 = 1;
   const char *sample_candidate1 = "SAMPLE_CANDIDATE1";
@@ -180,8 +241,10 @@ TEST(GtkCandidateWindowHandlerTest, UpdateTest) {
     RendererMock *renderer_mock = new RendererMock();
     TestableGtkCandidateWindowHandler gtk_candidate_window_handler(
         renderer_mock);
-    EXPECT_CALL_EXEC_COMMAND(*renderer_mock, VisibilityEq(false));
-    gtk_candidate_window_handler.Update(NULL, output);
+    EXPECT_CALL_EXEC_COMMAND(*renderer_mock,
+                             VisibilityEq(false),
+                             PreeditRectangleEq(kExpectedCursorArea));
+    gtk_candidate_window_handler.Update(&engine, output);
   }
   {
     SCOPED_TRACE("If there is at least one candidate, "
@@ -194,8 +257,10 @@ TEST(GtkCandidateWindowHandlerTest, UpdateTest) {
     RendererMock *renderer_mock = new RendererMock();
     TestableGtkCandidateWindowHandler gtk_candidate_window_handler(
         renderer_mock);
-    EXPECT_CALL_EXEC_COMMAND(*renderer_mock, VisibilityEq(true));
-    gtk_candidate_window_handler.Update(NULL, output);
+    EXPECT_CALL_EXEC_COMMAND(*renderer_mock,
+                             VisibilityEq(true),
+                             PreeditRectangleEq(kExpectedCursorArea));
+    gtk_candidate_window_handler.Update(&engine, output);
   }
   {
     SCOPED_TRACE("Update last updated output protobuf object.");
@@ -221,29 +286,49 @@ TEST(GtkCandidateWindowHandlerTest, UpdateTest) {
                              Property(&RendererCommand::output,
                                       OutputEq(output2)))
         .WillOnce(Return(true));
-    gtk_candidate_window_handler.Update(NULL, output1);
+    gtk_candidate_window_handler.Update(&engine, output1);
     EXPECT_THAT(*(gtk_candidate_window_handler.last_update_output_.get()),
                 OutputEq(output1));
-    gtk_candidate_window_handler.Update(NULL, output2);
+    gtk_candidate_window_handler.Update(&engine, output2);
     EXPECT_THAT(*(gtk_candidate_window_handler.last_update_output_.get()),
                 OutputEq(output2));
   }
 }
 
 TEST(GtkCandidateWindowHandlerTest, HideTest) {
+  const Rect kExpectedCursorArea(10, 20, 200, 100);
+
+  IBusEngine engine = {};
+  engine.cursor_area.x = kExpectedCursorArea.Left();
+  engine.cursor_area.y = kExpectedCursorArea.Top();
+  engine.cursor_area.width = kExpectedCursorArea.Width();
+  engine.cursor_area.height = kExpectedCursorArea.Height();
+
   RendererMock *renderer_mock = new RendererMock();
   TestableGtkCandidateWindowHandler gtk_candidate_window_handler(
       renderer_mock);
-  EXPECT_CALL_EXEC_COMMAND(*renderer_mock, VisibilityEq(false));
-  gtk_candidate_window_handler.Hide(NULL);
+  EXPECT_CALL_EXEC_COMMAND(*renderer_mock,
+                           VisibilityEq(false),
+                           PreeditRectangleEq(kExpectedCursorArea));
+  gtk_candidate_window_handler.Hide(&engine);
 }
 
 TEST(GtkCandidateWindowHandlerTest, ShowTest) {
+  const Rect kExpectedCursorArea(10, 20, 200, 100);
+
+  IBusEngine engine = {};
+  engine.cursor_area.x = kExpectedCursorArea.Left();
+  engine.cursor_area.y = kExpectedCursorArea.Top();
+  engine.cursor_area.width = kExpectedCursorArea.Width();
+  engine.cursor_area.height = kExpectedCursorArea.Height();
+
   RendererMock *renderer_mock = new RendererMock();
   TestableGtkCandidateWindowHandler gtk_candidate_window_handler(
       renderer_mock);
-  EXPECT_CALL_EXEC_COMMAND(*renderer_mock, VisibilityEq(true));
-  gtk_candidate_window_handler.Show(NULL);
+  EXPECT_CALL_EXEC_COMMAND(*renderer_mock,
+                           VisibilityEq(true),
+                           PreeditRectangleEq(kExpectedCursorArea));
+  gtk_candidate_window_handler.Show(&engine);
 }
 
 }  // namespace ibus