From aefddca40c9c0510d024678f50df0795e2ea2187 Mon Sep 17 00:00:00 2001 From: Taku Kudo Date: Thu, 13 Feb 2025 09:41:11 +0000 Subject: [PATCH] - Cleanup ImeContext: Introduces an inner CopyableData to handle copyable and non-copiable data. - Stop passing config to request to create engine_converter. They are set via Set config and SetRequest in the first place. Currently, default objects are passed, which are not used. PiperOrigin-RevId: 726376247 --- src/engine/BUILD.bazel | 1 + src/engine/engine.h | 8 +-- src/engine/engine_converter.cc | 5 ++ src/engine/engine_converter.h | 1 + src/engine/engine_interface.h | 3 +- src/engine/engine_mock.h | 4 +- src/session/BUILD.bazel | 6 +- src/session/ime_context.cc | 100 +++++++++++++-------------- src/session/ime_context.h | 116 ++++++++++++++++---------------- src/session/ime_context_test.cc | 71 ++++++++++--------- src/session/session.cc | 26 ++----- src/session/session.h | 4 +- src/session/session_test.cc | 6 +- 13 files changed, 169 insertions(+), 182 deletions(-) diff --git a/src/engine/BUILD.bazel b/src/engine/BUILD.bazel index 41da22ef5..c1d98257f 100644 --- a/src/engine/BUILD.bazel +++ b/src/engine/BUILD.bazel @@ -404,6 +404,7 @@ mozc_cc_library( "//base:util", "//base:vlog", "//composer", + "//config:config_handler", "//converter:converter_interface", "//converter:segments", "//protocol:candidate_window_cc_proto", diff --git a/src/engine/engine.h b/src/engine/engine.h index e6d231f01..c49b579c4 100644 --- a/src/engine/engine.h +++ b/src/engine/engine.h @@ -93,11 +93,9 @@ class Engine : public EngineInterface { return converter_ ? converter_.get() : minimal_converter_.get(); } - std::unique_ptr CreateEngineConverter( - const commands::Request &request, - const config::Config &config) const override { - return std::make_unique(*GetConverter(), request, - config); + std::unique_ptr CreateEngineConverter() + const override { + return std::make_unique(*GetConverter()); } // Functions for Reload, Sync, Wait return true if successfully operated diff --git a/src/engine/engine_converter.cc b/src/engine/engine_converter.cc index 98996378a..b8ec294d6 100644 --- a/src/engine/engine_converter.cc +++ b/src/engine/engine_converter.cc @@ -50,6 +50,7 @@ #include "base/util.h" #include "base/vlog.h" #include "composer/composer.h" +#include "config/config_handler.h" #include "converter/converter_interface.h" #include "converter/segments.h" #include "engine/candidate_list.h" @@ -105,6 +106,10 @@ int32_t CalculateCursorOffset(absl::string_view committed_text) { } } // namespace +EngineConverter::EngineConverter(const ConverterInterface &converter) + : EngineConverter(converter, commands::Request::default_instance(), + config::ConfigHandler::DefaultConfig()) {} + EngineConverter::EngineConverter(const ConverterInterface &converter, const Request &request, const Config &config) : EngineConverterInterface(), diff --git a/src/engine/engine_converter.h b/src/engine/engine_converter.h index 6a4c6c32e..dedcaa178 100644 --- a/src/engine/engine_converter.h +++ b/src/engine/engine_converter.h @@ -59,6 +59,7 @@ class EngineConverter : public EngineConverterInterface { EngineConverter(const ConverterInterface &converter, const commands::Request &request, const config::Config &config); + explicit EngineConverter(const ConverterInterface &converter); EngineConverter(const EngineConverter &) = delete; EngineConverter &operator=(const EngineConverter &) = delete; diff --git a/src/engine/engine_interface.h b/src/engine/engine_interface.h index 04cd9aaad..da9472fd4 100644 --- a/src/engine/engine_interface.h +++ b/src/engine/engine_interface.h @@ -58,8 +58,7 @@ class EngineInterface { // Creates new session converter. // This method is called per input context. virtual std::unique_ptr - CreateEngineConverter(const commands::Request &request, - const config::Config &config) const = 0; + CreateEngineConverter() const = 0; // Gets the version of underlying data set. virtual absl::string_view GetDataVersion() const = 0; diff --git a/src/engine/engine_mock.h b/src/engine/engine_mock.h index c58d7a487..c6fb1e41a 100644 --- a/src/engine/engine_mock.h +++ b/src/engine/engine_mock.h @@ -48,9 +48,7 @@ class MockEngine : public EngineInterface { public: MOCK_METHOD(absl::string_view, GetDataVersion, (), (const, override)); MOCK_METHOD(std::unique_ptr, - CreateEngineConverter, - (const commands::Request &request, const config::Config &config), - (const, override)); + CreateEngineConverter, (), (const, override)); MOCK_METHOD(bool, Reload, (), (override)); MOCK_METHOD(bool, Sync, (), (override)); MOCK_METHOD(bool, Wait, (), (override)); diff --git a/src/session/BUILD.bazel b/src/session/BUILD.bazel index 6fa373bae..4b65fe8db 100644 --- a/src/session/BUILD.bazel +++ b/src/session/BUILD.bazel @@ -578,11 +578,13 @@ mozc_cc_library( ":key_event_transformer", ":keymap", "//composer", + "//composer:table", "//config:config_handler", "//engine:engine_converter_interface", "//protocol:commands_cc_proto", "//protocol:config_cc_proto", - "@com_google_absl//absl/log:check", + "@com_google_absl//absl/base:no_destructor", + "@com_google_absl//absl/memory", "@com_google_absl//absl/time", ], ) @@ -641,8 +643,10 @@ mozc_cc_test( tags = ["noandroid"], # TODO(b/73698251): disabled due to errors deps = [ ":ime_context", + ":keymap", "//composer", "//composer:table", + "//config:config_handler", "//converter:converter_interface", "//converter:converter_mock", "//converter:segments", diff --git a/src/session/ime_context.cc b/src/session/ime_context.cc index 6a109be0c..016cd681c 100644 --- a/src/session/ime_context.cc +++ b/src/session/ime_context.cc @@ -31,89 +31,83 @@ // a session. #include "session/ime_context.h" +#include +#include -#include "absl/log/check.h" +#include "absl/base/no_destructor.h" +#include "absl/memory/memory.h" +#include "absl/time/time.h" #include "composer/composer.h" +#include "composer/table.h" +#include "config/config_handler.h" #include "engine/engine_converter_interface.h" #include "protocol/commands.pb.h" #include "session/keymap.h" namespace mozc { namespace session { +namespace { -const composer::Composer &ImeContext::composer() const { - DCHECK(composer_.get()); - return *composer_; +const keymap::KeyMapManager &DefaultKeyMapManager() { + static const absl::NoDestructor kDefaultKeyMapManager; + return *kDefaultKeyMapManager; } -composer::Composer *ImeContext::mutable_composer() { - DCHECK(composer_.get()); - return composer_.get(); +} // namespace + +ImeContext::CopyableData::CopyableData() + : create_time(absl::InfinitePast()), + last_command_time(absl::InfinitePast()), + request(&commands::Request::default_instance()), + config(&config::ConfigHandler::DefaultConfig()), + key_map_manager(&DefaultKeyMapManager()), + composer(composer::Table::GetDefaultTable(), *request, *config), + state(NONE) { + key_event_transformer.ReloadConfig(*config); +} + +ImeContext::ImeContext( + std::unique_ptr converter) + : converter_(std::move(converter)) {} + +ImeContext::ImeContext(const ImeContext &src) : data_(src.data_) { + if (src.converter_) { + converter_ = absl::WrapUnique(src.converter().Clone()); + } } void ImeContext::SetRequest(const commands::Request &request) { - request_ = &request; - converter_->SetRequest(*request_); - composer_->SetRequest(*request_); + data_.request = &request; + if (converter_) { + converter_->SetRequest(*data_.request); + } + data_.composer.SetRequest(*data_.request); } const commands::Request &ImeContext::GetRequest() const { - DCHECK(request_); - return *request_; + return *data_.request; } void ImeContext::SetConfig(const config::Config &config) { - config_ = &config; + data_.config = &config; - DCHECK(converter_.get()); - converter_->SetConfig(*config_); + if (converter_) { + converter_->SetConfig(*data_.config); + } - DCHECK(composer_.get()); - composer_->SetConfig(*config_); - - key_event_transformer_.ReloadConfig(*config_); + data_.composer.SetConfig(*data_.config); + data_.key_event_transformer.ReloadConfig(*data_.config); } -const config::Config &ImeContext::GetConfig() const { - DCHECK(config_); - return *config_; -} +const config::Config &ImeContext::GetConfig() const { return *data_.config; } void ImeContext::SetKeyMapManager( const keymap::KeyMapManager &key_map_manager) { - key_map_manager_ = &key_map_manager; + data_.key_map_manager = &key_map_manager; } const keymap::KeyMapManager &ImeContext::GetKeyMapManager() const { - if (key_map_manager_) { - return *key_map_manager_; - } - static const keymap::KeyMapManager *void_key_map_manager = - new keymap::KeyMapManager(); - return *void_key_map_manager; + return *data_.key_map_manager; } - -// static -void ImeContext::CopyContext(const ImeContext &src, ImeContext *dest) { - DCHECK(dest); - - dest->set_create_time(src.create_time()); - dest->set_last_command_time(src.last_command_time()); - - *dest->mutable_composer() = src.composer(); - dest->converter_.reset(src.converter().Clone()); - dest->key_event_transformer_ = src.key_event_transformer_; - - dest->set_state(src.state()); - - dest->SetRequest(*src.request_); - dest->SetConfig(*src.config_); - dest->SetKeyMapManager(src.GetKeyMapManager()); - - *dest->mutable_client_capability() = src.client_capability(); - *dest->mutable_application_info() = src.application_info(); - *dest->mutable_output() = src.output(); -} - } // namespace session } // namespace mozc diff --git a/src/session/ime_context.h b/src/session/ime_context.h index 4ef83d204..135d01356 100644 --- a/src/session/ime_context.h +++ b/src/session/ime_context.h @@ -50,29 +50,25 @@ namespace session { class ImeContext final { public: - ImeContext() - : request_(&commands::Request::default_instance()), - config_(&config::ConfigHandler::DefaultConfig()), - key_map_manager_(nullptr) {} + ImeContext() = default; + explicit ImeContext( + std::unique_ptr converter); + explicit ImeContext(const ImeContext &src); - ImeContext(const ImeContext &) = delete; ImeContext &operator=(const ImeContext &) = delete; - absl::Time create_time() const { return create_time_; } - void set_create_time(absl::Time create_time) { create_time_ = create_time; } + absl::Time create_time() const { return data_.create_time; } + void set_create_time(absl::Time create_time) { + data_.create_time = create_time; + } - absl::Time last_command_time() const { return last_command_time_; } + absl::Time last_command_time() const { return data_.last_command_time; } void set_last_command_time(absl::Time last_command_time) { - last_command_time_ = last_command_time; + data_.last_command_time = last_command_time; } - // Note that before using getter methods, - // |composer_| must be set non-null value. - const composer::Composer &composer() const; - composer::Composer *mutable_composer(); - void set_composer(std::unique_ptr composer) { - composer_ = std::move(composer); - } + const composer::Composer &composer() const { return data_.composer; } + composer::Composer *mutable_composer() { return &data_.composer; } const engine::EngineConverterInterface &converter() const { return *converter_; @@ -80,13 +76,9 @@ class ImeContext final { engine::EngineConverterInterface *mutable_converter() { return converter_.get(); } - void set_converter( - std::unique_ptr converter) { - converter_ = std::move(converter); - } const KeyEventTransformer &key_event_transformer() const { - return key_event_transformer_; + return data_.key_event_transformer; } enum State { @@ -96,8 +88,8 @@ class ImeContext final { COMPOSITION = 4, CONVERSION = 8, }; - State state() const { return state_; } - void set_state(State state) { state_ = state; } + State state() const { return data_.state; } + void set_state(State state) { data_.state = state; } void SetRequest(const commands::Request &request); const commands::Request &GetRequest() const; @@ -109,57 +101,67 @@ class ImeContext final { const keymap::KeyMapManager &GetKeyMapManager() const; const commands::Capability &client_capability() const { - return client_capability_; + return data_.client_capability; } commands::Capability *mutable_client_capability() { - return &client_capability_; + return &data_.client_capability; } const commands::ApplicationInfo &application_info() const { - return application_info_; + return data_.application_info; } commands::ApplicationInfo *mutable_application_info() { - return &application_info_; + return &data_.application_info; } // Note that this may not be the latest info: this is likely to be a snapshot // of during the precomposition state and may not be updated during // composition/conversion state. - const commands::Context &client_context() const { return client_context_; } - commands::Context *mutable_client_context() { return &client_context_; } + const commands::Context &client_context() const { + return data_.client_context; + } + commands::Context *mutable_client_context() { return &data_.client_context; } - const commands::Output &output() const { return output_; } - commands::Output *mutable_output() { return &output_; } - - // Copy |source| context to |destination| context. - // TODO(hsumita): Renames it as CopyFrom and make it non-static to keep - // consistency with other classes. - static void CopyContext(const ImeContext &src, ImeContext *dest); + const commands::Output &output() const { return data_.output; } + commands::Output *mutable_output() { return &data_.output; } private: - // TODO(team): Actual use of |create_time_| is to keep the time when the - // session holding this instance is created and not the time when this - // instance is created. We may want to move out |create_time_| from ImeContext - // to Session, or somewhere more appropriate. - absl::Time create_time_ = absl::InfinitePast(); - absl::Time last_command_time_ = absl::InfinitePast(); + // Separate copyable data and non-copyable data to + // easily overload copy operator. + struct CopyableData { + CopyableData(); - std::unique_ptr composer_; + // TODO(team): Actual use of |create_time| is to keep the time when the + // session holding this instance is created and not the time when this + // instance is created. We may want to move out |create_time| from + // ImeContext to Session, or somewhere more appropriate. + absl::Time create_time; + absl::Time last_command_time; + + // TODO(team): We want to avoid using raw pointer to share + // frequently updated object with large footprint. + // Replace them with copy or std::shared_ptr to prevent dangling pointer. + const commands::Request *request; + const config::Config *config; + const keymap::KeyMapManager *key_map_manager; + + composer::Composer composer; + KeyEventTransformer key_event_transformer; + + State state; + commands::Capability client_capability; + commands::ApplicationInfo application_info; + commands::Context client_context; + + // Storing the last output consisting of the last result and the + // last performed command. + commands::Output output; + }; + + CopyableData data_; + + // converter_ should explicitly be copied via Clone() method. std::unique_ptr converter_; - KeyEventTransformer key_event_transformer_; - - const commands::Request *request_; - const config::Config *config_; - const keymap::KeyMapManager *key_map_manager_; - - State state_ = NONE; - commands::Capability client_capability_; - commands::ApplicationInfo application_info_; - commands::Context client_context_; - - // Storing the last output consisting of the last result and the - // last performed command. - commands::Output output_; }; } // namespace session diff --git a/src/session/ime_context_test.cc b/src/session/ime_context_test.cc index c977fbf74..8892b5da7 100644 --- a/src/session/ime_context_test.cc +++ b/src/session/ime_context_test.cc @@ -35,12 +35,14 @@ #include "absl/time/time.h" #include "composer/composer.h" #include "composer/table.h" +#include "config/config_handler.h" #include "converter/converter_interface.h" #include "converter/converter_mock.h" #include "converter/segments.h" #include "engine/engine_converter.h" #include "protocol/commands.pb.h" #include "protocol/config.pb.h" +#include "session/keymap.h" #include "testing/gmock.h" #include "testing/gunit.h" #include "testing/testing_util.h" @@ -60,13 +62,19 @@ TEST(ImeContextTest, DefaultValues) { EXPECT_EQ(context.create_time(), absl::InfinitePast()); EXPECT_EQ(context.last_command_time(), absl::InfinitePast()); EXPECT_FALSE(context.mutable_converter()); + EXPECT_TRUE(context.mutable_composer()); EXPECT_EQ(context.state(), ImeContext::NONE); EXPECT_PROTO_EQ(commands::Request::default_instance(), context.GetRequest()); + EXPECT_PROTO_EQ(config::ConfigHandler::DefaultConfig(), context.GetConfig()); } TEST(ImeContextTest, BasicTest) { - ImeContext context; - config::Config config; + const commands::Request request; + const config::Config config; + const keymap::KeyMapManager keymap; + + MockConverter converter; + ImeContext context(std::make_unique(converter)); context.set_create_time(absl::FromUnixSeconds(100)); EXPECT_EQ(context.create_time(), absl::FromUnixSeconds(100)); @@ -74,19 +82,15 @@ TEST(ImeContextTest, BasicTest) { context.set_last_command_time(absl::FromUnixSeconds(12345)); EXPECT_EQ(context.last_command_time(), absl::FromUnixSeconds(12345)); - const commands::Request request; - - context.set_composer(std::make_unique(request, config)); - - MockConverter converter; - context.set_converter( - std::make_unique(converter, request, config)); - context.set_state(ImeContext::COMPOSITION); EXPECT_EQ(context.state(), ImeContext::COMPOSITION); context.SetRequest(request); + context.SetConfig(config); + context.SetKeyMapManager(keymap); EXPECT_PROTO_EQ(request, context.GetRequest()); + EXPECT_PROTO_EQ(config, context.GetConfig()); + EXPECT_EQ(&keymap, &context.GetKeyMapManager()); context.mutable_client_capability()->set_text_deletion( commands::Capability::DELETE_PRECEDING_TEXT); @@ -107,6 +111,8 @@ TEST(ImeContextTest, CopyContext) { table.AddRule("na", "な", ""); const commands::Request request; config::Config config; + const keymap::KeyMapManager keymap; + config.set_session_keymap(config::Config::CHROMEOS); MockConverter converter; @@ -120,53 +126,46 @@ TEST(ImeContextTest, CopyContext) { .WillOnce(DoAll(SetArgPointee<1>(segments), Return(true))); { - ImeContext source; - source.set_composer(std::make_unique(table, request, config)); - source.set_converter( - std::make_unique(converter, request, config)); - - ImeContext destination; - destination.set_composer( - std::make_unique(table, request, config)); - destination.set_converter( - std::make_unique(converter, request, config)); + ImeContext source(std::make_unique(converter)); + source.SetRequest(request); + source.SetConfig(config); + source.SetKeyMapManager(keymap); + source.mutable_composer()->SetTable(table); source.set_state(ImeContext::COMPOSITION); source.mutable_composer()->InsertCharacter("a"); source.mutable_composer()->InsertCharacter("n"); - source.SetConfig(config); - std::string composition = source.composer().GetStringForSubmission(); EXPECT_EQ(composition, "あｎ"); - ImeContext::CopyContext(source, &destination); + ImeContext destination(source); + + EXPECT_PROTO_EQ(source.GetRequest(), destination.GetRequest()); + EXPECT_PROTO_EQ(source.GetConfig(), destination.GetConfig()); + EXPECT_EQ(&source.GetKeyMapManager(), &destination.GetKeyMapManager()); EXPECT_EQ(destination.state(), ImeContext::COMPOSITION); - composition = source.composer().GetStringForSubmission(); + composition = destination.composer().GetStringForSubmission(); EXPECT_EQ(composition, "あｎ"); } { constexpr absl::Time kCreateTime = absl::FromUnixSeconds(100); constexpr absl::Time kLastCommandTime = absl::FromUnixSeconds(200); - ImeContext source; + + ImeContext source(std::make_unique(converter)); + source.SetRequest(request); + source.SetConfig(config); + source.SetKeyMapManager(keymap); + source.mutable_composer()->SetTable(table); source.set_create_time(kCreateTime); source.set_last_command_time(kLastCommandTime); - source.set_composer(std::make_unique(table, request, config)); - source.set_converter( - std::make_unique(converter, request, config)); - - ImeContext destination; - destination.set_composer( - std::make_unique(table, request, config)); - destination.set_converter( - std::make_unique(converter, request, config)); source.set_state(ImeContext::CONVERSION); source.mutable_composer()->InsertCharacter("a"); source.mutable_composer()->InsertCharacter("n"); source.mutable_converter()->Convert(source.composer()); - const std::string &kQuick = "早い"; + constexpr absl::string_view kQuick = "早い"; source.mutable_composer()->set_source_text(kQuick); std::string composition = source.composer().GetQueryForConversion(); @@ -177,7 +176,7 @@ TEST(ImeContextTest, CopyContext) { EXPECT_EQ(output.preedit().segment_size(), 1); EXPECT_EQ(output.preedit().segment(0).value(), "庵"); - ImeContext::CopyContext(source, &destination); + ImeContext destination(source); EXPECT_EQ(destination.create_time(), kCreateTime); EXPECT_EQ(destination.last_command_time(), kLastCommandTime); EXPECT_EQ(destination.state(), ImeContext::CONVERSION); diff --git a/src/session/session.cc b/src/session/session.cc index be99441d0..e2da4dec2 100644 --- a/src/session/session.cc +++ b/src/session/session.cc @@ -219,20 +219,13 @@ ImeContext::State GetEffectiveStateForTestSendKey(const commands::KeyEvent &key, } // namespace -// TODO(komatsu): Remove these argument by using/making singletons. Session::Session(EngineInterface *engine) - : engine_(engine), context_(new ImeContext) { - InitContext(context_.get()); -} + : engine_(engine), context_(CreateContext()) {} -void Session::InitContext(ImeContext *context) const { +std::unique_ptr Session::CreateContext() const { + auto context = std::make_unique(engine_->CreateEngineConverter()); context->set_create_time(Clock::GetAbslTime()); - context->set_last_command_time(absl::InfinitePast()); - context->set_composer(std::make_unique( - composer::Table::GetDefaultTable(), context->GetRequest(), - context->GetConfig())); - context->set_converter(engine_->CreateEngineConverter(context->GetRequest(), - context->GetConfig())); + #ifdef _WIN32 // On Windows session is started with direct mode. // FIXME(toshiyuki): Ditto for Mac after verifying on Mac. @@ -240,10 +233,6 @@ void Session::InitContext(ImeContext *context) const { #else // _WIN32 context->set_state(ImeContext::PRECOMPOSITION); #endif // _WIN32 - context->mutable_client_context()->Clear(); - - context->SetConfig(context->GetConfig()); - context->SetKeyMapManager(context->GetKeyMapManager()); // TODO(team): Remove #if based behavior change for cascading window. // Tests for session layer (session_handler_scenario_test, etc) can be @@ -252,14 +241,13 @@ void Session::InitContext(ImeContext *context) const { defined(__wasm__) context->mutable_converter()->set_use_cascading_window(false); #endif // TARGET_OS_IPHONE || __linux__ || __wasm__ + + return context; } void Session::PushUndoContext() { // Copy the current context and push it to the undo stack. - auto prev_context = std::make_unique(); - InitContext(prev_context.get()); - ImeContext::CopyContext(*context_, prev_context.get()); - undo_contexts_.push_back(std::move(prev_context)); + undo_contexts_.emplace_back(std::make_unique(*context_)); // If the stack size exceeds the limitation, purge the oldest entries. while (undo_contexts_.size() > kMultipleUndoMaxSize) { undo_contexts_.pop_front(); diff --git a/src/session/session.h b/src/session/session.h index f06d7f9b7..492eef296 100644 --- a/src/session/session.h +++ b/src/session/session.h @@ -270,14 +270,14 @@ class Session { // i) Session doesn't own the pointer. // ii) The state of underlying converter will change because it manages user // history, user dictionary, etc. - mozc::EngineInterface *engine_; + mozc::EngineInterface *engine_ = nullptr; std::unique_ptr context_; // Undo stack. *begin is the oldest, and *back is the newest. std::deque> undo_contexts_; - void InitContext(ImeContext *context) const; + std::unique_ptr CreateContext() const; void PushUndoContext(); void PopUndoContext(); diff --git a/src/session/session_test.cc b/src/session/session_test.cc index 7de10f30c..6fc712662 100644 --- a/src/session/session_test.cc +++ b/src/session/session_test.cc @@ -500,10 +500,8 @@ class SessionTest : public testing::TestWithTempUserProfile { void InitCreateEngineConverterMock(MockEngine *mock_engine, MockConverter *mock_converter) { EXPECT_CALL(*mock_engine, CreateEngineConverter) - .WillRepeatedly([mock_converter](const commands::Request &request, - const config::Config &config) { - return std::make_unique(*mock_converter, - request, config); + .WillRepeatedly([mock_converter]() { + return std::make_unique(*mock_converter); }); }