- 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
This commit is contained in:
Taku Kudo
2025-02-13 09:41:11 +00:00
committed by Hiroyuki Komatsu
parent e9f158cfdf
commit aefddca40c
13 changed files with 169 additions and 182 deletions

View File

@ -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",

View File

@ -93,11 +93,9 @@ class Engine : public EngineInterface {
return converter_ ? converter_.get() : minimal_converter_.get();
}
std::unique_ptr<engine::EngineConverterInterface> CreateEngineConverter(
const commands::Request &request,
const config::Config &config) const override {
return std::make_unique<engine::EngineConverter>(*GetConverter(), request,
config);
std::unique_ptr<engine::EngineConverterInterface> CreateEngineConverter()
const override {
return std::make_unique<engine::EngineConverter>(*GetConverter());
}
// Functions for Reload, Sync, Wait return true if successfully operated

View File

@ -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(),

View File

@ -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;

View File

@ -58,8 +58,7 @@ class EngineInterface {
// Creates new session converter.
// This method is called per input context.
virtual std::unique_ptr<engine::EngineConverterInterface>
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;

View File

@ -48,9 +48,7 @@ class MockEngine : public EngineInterface {
public:
MOCK_METHOD(absl::string_view, GetDataVersion, (), (const, override));
MOCK_METHOD(std::unique_ptr<engine::EngineConverterInterface>,
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));

View File

@ -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",

View File

@ -31,89 +31,83 @@
// a session.
#include "session/ime_context.h"
#include <memory>
#include <utility>
#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<keymap::KeyMapManager> 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<engine::EngineConverterInterface> 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

View File

@ -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<engine::EngineConverterInterface> 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> 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<engine::EngineConverterInterface> 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
// Separate copyable data and non-copyable data to
// easily overload copy operator.
struct CopyableData {
CopyableData();
// 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();
// 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;
std::unique_ptr<composer::Composer> composer_;
std::unique_ptr<engine::EngineConverterInterface> converter_;
KeyEventTransformer key_event_transformer_;
// 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;
const commands::Request *request_;
const config::Config *config_;
const keymap::KeyMapManager *key_map_manager_;
composer::Composer composer;
KeyEventTransformer key_event_transformer;
State state_ = NONE;
commands::Capability client_capability_;
commands::ApplicationInfo application_info_;
commands::Context client_context_;
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_;
commands::Output output;
};
CopyableData data_;
// converter_ should explicitly be copied via Clone() method.
std::unique_ptr<engine::EngineConverterInterface> converter_;
};
} // namespace session

View File

@ -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<EngineConverter>(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<Composer>(request, config));
MockConverter converter;
context.set_converter(
std::make_unique<EngineConverter>(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<Composer>(table, request, config));
source.set_converter(
std::make_unique<EngineConverter>(converter, request, config));
ImeContext destination;
destination.set_composer(
std::make_unique<Composer>(table, request, config));
destination.set_converter(
std::make_unique<EngineConverter>(converter, request, config));
ImeContext source(std::make_unique<EngineConverter>(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, "あn");
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, "あn");
}
{
constexpr absl::Time kCreateTime = absl::FromUnixSeconds(100);
constexpr absl::Time kLastCommandTime = absl::FromUnixSeconds(200);
ImeContext source;
ImeContext source(std::make_unique<EngineConverter>(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<Composer>(table, request, config));
source.set_converter(
std::make_unique<EngineConverter>(converter, request, config));
ImeContext destination;
destination.set_composer(
std::make_unique<Composer>(table, request, config));
destination.set_converter(
std::make_unique<EngineConverter>(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);

View File

@ -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<ImeContext> Session::CreateContext() const {
auto context = std::make_unique<ImeContext>(engine_->CreateEngineConverter());
context->set_create_time(Clock::GetAbslTime());
context->set_last_command_time(absl::InfinitePast());
context->set_composer(std::make_unique<composer::Composer>(
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<ImeContext>();
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<ImeContext>(*context_));
// If the stack size exceeds the limitation, purge the oldest entries.
while (undo_contexts_.size() > kMultipleUndoMaxSize) {
undo_contexts_.pop_front();

View File

@ -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<ImeContext> context_;
// Undo stack. *begin is the oldest, and *back is the newest.
std::deque<std::unique_ptr<ImeContext>> undo_contexts_;
void InitContext(ImeContext *context) const;
std::unique_ptr<ImeContext> CreateContext() const;
void PushUndoContext();
void PopUndoContext();

View File

@ -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<engine::EngineConverter>(*mock_converter,
request, config);
.WillRepeatedly([mock_converter]() {
return std::make_unique<engine::EngineConverter>(*mock_converter);
});
}