Skip to content

Commit

Permalink
Fix #779: Remove src/unix/ibus/selection_monitor.cc
Browse files Browse the repository at this point in the history 
selection_monitor.cc has been a fallback for features built on top of
surrounding text APIs to continue working even when
IbusEngineWrapper::GetSurroundingText() doesn't return correct
anchor_pos.

The way how selection_monitor.cc worked around was to keep monitoring
clipboards via by X11 APIs. However I believe it's time to remove this
workaround. My rationales are:

 * Having a runtime dependency on xcb from ibus_mozc is getting more
   and more questionable, especially when the industry is heading to
   Wayland. There was actually an issue in Ubuntu 21.10 [1] because of
   this dependency.
 * If an IME API doesn't work well, ideally such an issue should be
   addressed in the right component, rather than trying to work around
   it in each IME.

 [1]: https://bugs.launchpad.net/ubuntu/+source/mozc/+bug/1946969

#codehealth

PiperOrigin-RevId: 551164079
  • Loading branch information
@yukawa @hiroyuki-komatsu
yukawa authored and hiroyuki-komatsu committed Jul 26, 2023
1 parent c777896 commit 88fb0c6
Show file tree
Hide file tree
Showing 8 changed files with 5 additions and 764 deletions.
4 changes: 2 additions & 2 deletions .github/workflows/linux.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ jobs:
- name: Setup
run: |
sudo apt-get update
sudo apt-get install -y libibus-1.0-dev qtbase5-dev libgtk2.0-dev libxcb-xfixes0-dev
sudo apt-get install -y libibus-1.0-dev qtbase5-dev libgtk2.0-dev
echo "CC=clang-12" >> $GITHUB_ENV
echo "CXX=clang++-12" >> $GITHUB_ENV
#
Expand Down Expand Up @@ -59,7 +59,7 @@ jobs:
- name: Setup
run: |
sudo apt-get update
sudo apt-get install -y libibus-1.0-dev qtbase5-dev libgtk2.0-dev libxcb-xfixes0-dev
sudo apt-get install -y libibus-1.0-dev qtbase5-dev libgtk2.0-dev
echo "CC=clang-14" >> $GITHUB_ENV
echo "CXX=clang++-14" >> $GITHUB_ENV
#
Expand Down
2 changes: 1 addition & 1 deletion docker/ubuntu20.04/Dockerfile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ RUN apt upgrade -y
## Common packages for linux build environment
RUN apt-get install -y clang libc++-dev libc++abi-dev python3 pkg-config git curl bzip2 unzip make ninja-build
## Packages for linux desktop version
RUN apt-get install -y libibus-1.0-dev libglib2.0-dev qtbase5-dev libgtk2.0-dev libxcb-xfixes0-dev
RUN apt-get install -y libibus-1.0-dev libglib2.0-dev qtbase5-dev libgtk2.0-dev
## Packages for misc tools
RUN apt-get install -y nano

Expand Down
30 changes: 0 additions & 30 deletions src/unix/ibus/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,6 @@ mozc_cc_library(
),
defines = [
"ENABLE_QT_RENDERER",
"MOZC_ENABLE_X11_SELECTION_MONITOR",
],
deps = [
":candidate_window_handler",
Expand All @@ -203,7 +202,6 @@ mozc_cc_library(
":ibus_property_handler",
":ibus_utils",
":ibus_wrapper",
":x11_selection_monitor",
"//base:clock",
"//base:const",
"//base:file_util",
Expand Down Expand Up @@ -369,31 +367,3 @@ mozc_cc_test(
"//testing:gunit_main",
],
)

mozc_cc_library(
name = "x11_selection_monitor",
srcs = mozc_select(linux = ["selection_monitor.cc"]),
hdrs = mozc_select(linux = ["selection_monitor.h"]),
defines = ["MOZC_ENABLE_X11_SELECTION_MONITOR"],
linkopts = mozc_select(
oss_linux = [
"-lxcb",
"-lxcb-xfixes",
],
),
deps = [
"//base:logging",
"//base:port",
"//base:thread2",
"//base:util",
"@com_google_absl//absl/synchronization",
"@com_google_absl//absl/time",
] + mozc_select(
linux = [
"//third_party/libxcb",
"//third_party/libxcb:includes",
"//third_party/libxcb:libxcb-xfixes",
],
oss_linux = [],
),
)
56 changes: 0 additions & 56 deletions src/unix/ibus/ibus.gyp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,6 @@
'gen_out_dir': '<(SHARED_INTERMEDIATE_DIR)/<(relative_dir)',
'ibus_mozc_icon_path%': '/usr/share/ibus-mozc/product_icon.png',
'ibus_mozc_path%': '/usr/lib/ibus-mozc/ibus-engine-mozc',
# enable_x11_selection_monitor represents if ibus_mozc uses X11 selection
# monitor or not.
'enable_x11_selection_monitor%': 1,
},
'targets': [
{
Expand Down Expand Up @@ -187,13 +184,6 @@
'message_translator',
'path_util',
],
'conditions': [
['enable_x11_selection_monitor==1', {
'dependencies': [
'selection_monitor',
],
}],
],
},
{
'target_name': 'gen_ibus_mozc_files',
Expand Down Expand Up @@ -277,52 +267,6 @@
}],
],
},
{
# Meta target to set up build environment for ibus. Required 'cflags'
# and 'link_settings' will be automatically injected into any target
# which directly or indirectly depends on this target.
'target_name': 'xcb_build_environment',
'type': 'none',
'variables': {
'target_libs': [
'xcb',
'xcb-xfixes',
],
},
'all_dependent_settings': {
'cflags': [
'<!@(pkg-config --cflags <@(target_libs))',
],
'link_settings': {
'libraries': [
'<!@(pkg-config --libs-only-l <@(target_libs))',
],
'ldflags': [
'<!@(pkg-config --libs-only-L <@(target_libs))',
],
},
'conditions': [
['enable_x11_selection_monitor==1', {
'defines': [
'MOZC_ENABLE_X11_SELECTION_MONITOR=1'
],
}],
],
},
},
{
'target_name': 'selection_monitor',
'type': 'static_library',
'sources': [
'selection_monitor.cc',
],
'dependencies': [
'../../base/absl.gyp:absl_synchronization',
'../../base/absl.gyp:absl_time',
'../../base/base.gyp:base',
'xcb_build_environment',
],
},
{
'target_name': 'candidate_window_handler',
'type': 'static_library',
Expand Down
26 changes: 2 additions & 24 deletions src/unix/ibus/mozc_engine.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@
#include "unix/ibus/path_util.h"
#include "unix/ibus/preedit_handler.h"
#include "unix/ibus/property_handler.h"
#include "unix/ibus/selection_monitor.h"
#include "unix/ibus/surrounding_text_util.h"
#include "absl/flags/flag.h"
#include "absl/strings/string_view.h"
Expand Down Expand Up @@ -110,7 +109,6 @@ struct SurroundingTextInfo {
};

bool GetSurroundingText(IbusEngineWrapper *engine,
SelectionMonitorInterface *selection_monitor,
SurroundingTextInfo *info) {
if (!(engine->CheckCapabilities(IBUS_CAP_SURROUNDING_TEXT))) {
VLOG(1) << "Give up CONVERT_REVERSE due to client_capabilities: "
Expand All @@ -122,18 +120,6 @@ bool GetSurroundingText(IbusEngineWrapper *engine,
const absl::string_view surrounding_text =
engine->GetSurroundingText(&cursor_pos, &anchor_pos);

#ifdef MOZC_ENABLE_X11_SELECTION_MONITOR
if (cursor_pos == anchor_pos && selection_monitor != nullptr) {
const SelectionInfo &info = selection_monitor->GetSelectionInfo();
uint new_anchor_pos = 0;
if (SurroundingTextUtil::GetAnchorPosFromSelection(
surrounding_text, info.selected_text, cursor_pos,
&new_anchor_pos)) {
anchor_pos = new_anchor_pos;
}
}
#endif // MOZC_ENABLE_X11_SELECTION_MONITOR

if (!SurroundingTextUtil::GetSafeDelta(cursor_pos, anchor_pos,
&info->relative_selected_length)) {
LOG(ERROR) << "Too long text selection.";
Expand Down Expand Up @@ -218,16 +204,10 @@ MozcEngine::MozcEngine()
: last_sync_time_(Clock::GetTime()),
key_event_handler_(new KeyEventHandler),
client_(CreateAndConfigureClient()),
#ifdef MOZC_ENABLE_X11_SELECTION_MONITOR
selection_monitor_(SelectionMonitorFactory::Create(1024)),
#endif // MOZC_ENABLE_X11_SELECTION_MONITOR
preedit_handler_(new PreeditHandler()),
use_mozc_candidate_window_(UseMozcCandidateWindow()),
mozc_candidate_window_handler_(new renderer::RendererClient()),
preedit_method_(config::Config::ROMAN) {
if (selection_monitor_ != nullptr) {
selection_monitor_->StartMonitoring();
}
if (use_mozc_candidate_window_) {
mozc_candidate_window_handler_.RegisterGSettingsObserver();
}
Expand Down Expand Up @@ -389,8 +369,7 @@ bool MozcEngine::ProcessKeyEvent(IbusEngineWrapper *engine, uint keyval,

commands::Context context;
SurroundingTextInfo surrounding_text_info;
if (GetSurroundingText(engine, selection_monitor_.get(),
&surrounding_text_info)) {
if (GetSurroundingText(engine, &surrounding_text_info)) {
context.set_preceding_text(surrounding_text_info.preceding_text);
context.set_following_text(surrounding_text_info.following_text);
}
Expand Down Expand Up @@ -591,8 +570,7 @@ bool MozcEngine::ExecuteCallback(IbusEngineWrapper *engine,
}
break;
case commands::SessionCommand::CONVERT_REVERSE: {
if (!GetSurroundingText(engine, selection_monitor_.get(),
&surrounding_text_info)) {
if (!GetSurroundingText(engine, &surrounding_text_info)) {
return false;
}
session_command.set_text(surrounding_text_info.selection_text);
Expand Down
2 changes: 0 additions & 2 deletions src/unix/ibus/mozc_engine.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ namespace ibus {
class CandidateWindowHandlerInterface;
class KeyEventHandler;
class LaunchToolTest;
class SelectionMonitorInterface;

// Implements EngineInterface and handles signals from IBus daemon.
// This class mainly does the two things:
Expand Down Expand Up @@ -137,7 +136,6 @@ class MozcEngine : public EngineInterface {
uint64_t last_sync_time_;
std::unique_ptr<KeyEventHandler> key_event_handler_;
std::unique_ptr<client::ClientInterface> client_;
std::unique_ptr<SelectionMonitorInterface> selection_monitor_;

std::unique_ptr<PropertyHandler> property_handler_;
std::unique_ptr<PreeditHandler> preedit_handler_;
Expand Down
Loading

0 comments on commit 88fb0c6

Please sign in to comment.