Skip to content

Commit

Permalink
Remove global variables from GoogleJapaneseInputController.
Browse files Browse the repository at this point in the history
This CL removes global variables from GoogleJapaneseInputController and replaces them with local variables. This is a part of the effort to improve the code quality of GoogleJapaneseInputController.

The `Can*` functions return an opposite bool value from `IsBannedApplication`.

#codehealth

PiperOrigin-RevId: 679013091
  • Loading branch information
hiroyuki-komatsu committed Sep 26, 2024
1 parent 2ff5397 commit dd7b112
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 70 deletions.
3 changes: 0 additions & 3 deletions src/mac/GoogleJapaneseInputController.h
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,4 @@

/** Sets the RendererInterface to use in the controller. */
- (void)setRenderer:(std::unique_ptr<mozc::renderer::RendererInterface>)newRenderer;

/** create instances for global objects which will be referred from the controller instances. */
+ (void)initializeConstants;
@end
109 changes: 45 additions & 64 deletions src/mac/GoogleJapaneseInputController.mm
Original file line number Diff line number Diff line change
Expand Up @@ -77,20 +77,14 @@
using SetOfString = std::set<std::string, std::less<>>;

namespace {
// set of bundle IDs of applications on which Mozc should not open urls.
const SetOfString *gNoOpenLinkApps = nullptr;
const SetOfString *gNoSelectedRangeApps = nullptr;
const SetOfString *gNoDisplayModeSwitchApps = nullptr;
const SetOfString *gNoSurroundingTextApps = nullptr;

// TODO(horo): This value should be get from system configuration.
// DoubleClickInterval can be get from NSEvent (MacOSX ver >= 10.6)
const NSTimeInterval kDoubleTapInterval = 0.5;
constexpr NSTimeInterval kDoubleTapInterval = 0.5;

const int kMaxSurroundingLength = 20;
constexpr int kMaxSurroundingLength = 20;
// In some apllications when the client's text length is large, getting the
// surrounding text takes too much time. So we set this limitation.
const int kGetSurroundingTextClientLengthLimit = 1000;
constexpr int kGetSurroundingTextClientLengthLimit = 1000;

constexpr absl::string_view kRomanModeId = "com.apple.inputmethod.Roman";
constexpr absl::string_view kKatakanaModeId = "com.apple.inputmethod.Japanese.Katakana";
Expand Down Expand Up @@ -148,8 +142,36 @@ CompositionMode GetCompositionMode(absl::string_view mode_id) {
}
}

bool IsBannedApplication(const SetOfString *bundleIdSet, const absl::string_view bundleId) {
return bundleIdSet == nullptr || bundleIdSet->find(bundleId) != bundleIdSet->end();
bool CanOpenLink(absl::string_view bundle_id) {
// Should not open links during screensaver.
return bundle_id != "com.apple.securityagent";
}

bool CanSelectedRange(absl::string_view bundle_id) {
// Do not call selectedRange: method for the following
// applications because it could lead to application crash.
const bool is_supported =
bundle_id != "com.microsoft.Excel" &&
bundle_id != "com.microsoft.Powerpoint" &&
bundle_id != "com.microsoft.Word";
return is_supported;
}

bool CanDisplayModeSwitch(absl::string_view bundle_id) {
// Do not call selectInputMode: method for the following
// applications because it could cause some unexpected behavior.
// MS-Word: When the display mode goes to ASCII but there is no
// compositions, it goes to direct input mode instead of Half-ASCII
// mode. When the first composition character is alphanumeric (such
// like pressing Shift-A at first), that character is directly
// inserted into application instead of composition starting "A".
return bundle_id != "com.microsoft.Word";
}

bool CanSurroundingText(absl::string_view bundle_id) {
// Disables the surrounding text feature for the following application
// because calling attributedSubstringFromRange to it is very heavy.
return bundle_id != "com.evernote.Evernote";
}
} // namespace

Expand Down Expand Up @@ -244,46 +266,6 @@ - (NSMenu *)menu {
return menu_;
}

+ (void)initializeConstants {
SetOfString *noOpenlinkApps = new (std::nothrow) SetOfString;
if (noOpenlinkApps) {
// should not open links during screensaver.
noOpenlinkApps->insert("com.apple.securityagent");
gNoOpenLinkApps = noOpenlinkApps;
}

SetOfString *noSelectedRangeApps = new (std::nothrow) SetOfString;
if (noSelectedRangeApps) {
// Do not call selectedRange: method for the following
// applications because it could lead to application crash.
noSelectedRangeApps->insert("com.microsoft.Excel");
noSelectedRangeApps->insert("com.microsoft.Powerpoint");
noSelectedRangeApps->insert("com.microsoft.Word");
gNoSelectedRangeApps = noSelectedRangeApps;
}

// Do not call selectInputMode: method for the following
// applications because it could cause some unexpected behavior.
// MS-Word: When the display mode goes to ASCII but there is no
// compositions, it goes to direct input mode instead of Half-ASCII
// mode. When the first composition character is alphanumeric (such
// like pressing Shift-A at first), that character is directly
// inserted into application instead of composition starting "A".
SetOfString *noDisplayModeSwitchApps = new (std::nothrow) SetOfString;
if (noDisplayModeSwitchApps) {
noDisplayModeSwitchApps->insert("com.microsoft.Word");
gNoDisplayModeSwitchApps = noDisplayModeSwitchApps;
}

SetOfString *noSurroundingTextApps = new (std::nothrow) SetOfString;
if (noSurroundingTextApps) {
// Disables the surrounding text feature for the following application
// because calling attributedSubstringFromRange to it is very heavy.
noSurroundingTextApps->insert("com.evernote.Evernote");
gNoSurroundingTextApps = noSurroundingTextApps;
}
}

#pragma mark IMKStateSetting Protocol
// Currently it just ignores the following methods:
// Modes, showPreferences, valueForTag
Expand Down Expand Up @@ -383,10 +365,10 @@ - (void)setupClientBundle:(id)sender {
- (void)setupCapability {
Capability capability;

if (IsBannedApplication(gNoSelectedRangeApps, clientBundle_)) {
capability.set_text_deletion(Capability::NO_TEXT_DELETION_CAPABILITY);
} else {
if (CanSelectedRange(clientBundle_)) {
capability.set_text_deletion(Capability::DELETE_PRECEDING_TEXT);
} else {
capability.set_text_deletion(Capability::NO_TEXT_DELETION_CAPABILITY);
}

mozcClient_->set_client_capability(capability);
Expand Down Expand Up @@ -444,7 +426,7 @@ - (void)switchMode:(CompositionMode)new_mode client:(id)sender {
}

- (void)switchDisplayMode {
if (IsBannedApplication(gNoDisplayModeSwitchApps, clientBundle_)) {
if (!CanDisplayModeSwitch(clientBundle_)) {
return;
}

Expand All @@ -463,7 +445,7 @@ - (void)commitText:(const char *)text client:(id)sender {

- (void)launchWordRegisterTool:(id)client {
::setenv(mozc::kWordRegisterEnvironmentName, "", 1);
if (!IsBannedApplication(gNoSelectedRangeApps, clientBundle_)) {
if (CanSelectedRange(clientBundle_)) {
NSRange selectedRange = [client selectedRange];
if (selectedRange.location != NSNotFound && selectedRange.length != NSNotFound &&
selectedRange.length > 0) {
Expand All @@ -477,7 +459,7 @@ - (void)launchWordRegisterTool:(id)client {
}

- (void)invokeReconvert:(const SessionCommand *)command client:(id)sender {
if (IsBannedApplication(gNoSelectedRangeApps, clientBundle_)) {
if (!CanSelectedRange(clientBundle_)) {
return;
}

Expand Down Expand Up @@ -514,7 +496,7 @@ - (void)invokeReconvert:(const SessionCommand *)command client:(id)sender {
}

- (void)invokeUndo:(id)sender {
if (IsBannedApplication(gNoSelectedRangeApps, clientBundle_)) {
if (!CanSelectedRange(clientBundle_)) {
return;
}

Expand Down Expand Up @@ -556,7 +538,7 @@ - (void)processOutput:(const mozc::commands::Output *)output client:(id)sender {

// Handles deletion range. We do not even handle it for some
// applications to prevent application crashes.
if (output->has_deletion_range() && !IsBannedApplication(gNoSelectedRangeApps, clientBundle_)) {
if (output->has_deletion_range() && CanSelectedRange(clientBundle_)) {
if ([composedString_ length] == 0 && replacementRange_.location == NSNotFound) {
NSRange selectedRange = [sender selectedRange];
const mozc::commands::DeletionRange &deletion_range = output->deletion_range();
Expand Down Expand Up @@ -799,10 +781,9 @@ - (void)openLink:(NSURL *)url {
// On some application like login window of screensaver, opening
// link behavior should not happen because it can cause some
// security issues.
if (IsBannedApplication(gNoOpenLinkApps, clientBundle_)) {
return;
if (CanOpenLink(clientBundle_)) {
[[NSWorkspace sharedWorkspace] openURL:url];
}
[[NSWorkspace sharedWorkspace] openURL:url];
}

- (BOOL)fillSurroundingContext:(mozc::commands::Context *)context client:(id<IMKTextInput>)client {
Expand Down Expand Up @@ -924,8 +905,8 @@ - (BOOL)handleEvent:(NSEvent *)event client:(id)sender {
}
keyEvent.set_mode(mode_);

if ([composedString_ length] == 0 && !IsBannedApplication(gNoSelectedRangeApps, clientBundle_) &&
!IsBannedApplication(gNoSurroundingTextApps, clientBundle_)) {
if ([composedString_ length] == 0 && CanSelectedRange(clientBundle_) &&
CanSurroundingText(clientBundle_)) {
[self fillSurroundingContext:&context client:sender];
}
if (!mozcClient_->SendKeyWithContext(keyEvent, context, &output)) {
Expand Down
1 change: 0 additions & 1 deletion src/mac/GoogleJapaneseInputController_test.mm
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,6 @@ void SetUp() override {
method_setImplementation(method, reinterpret_cast<IMP>(openURL_test));
gOpenURLCount = 0;

[GoogleJapaneseInputController initializeConstants];
SetUpController();
}

Expand Down
2 changes: 0 additions & 2 deletions src/mac/main.mm
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,6 @@ int main(int argc, char *argv[]) {
[imkServer registerRendererConnection];
}

[GoogleJapaneseInputController initializeConstants];

// Start the converter server at this time explicitly to prevent the
// slow-down of the response for initial key event.
{
Expand Down

0 comments on commit dd7b112

Please sign in to comment.