Skip to content

feat: implement 3-state W2U toggle#5

Merged
nhktmdzhg merged 5 commits into
LotusInputMethod:masterfrom
hthienloc:feat/w2u-modular
Apr 14, 2026
Merged

feat: implement 3-state W2U toggle#5
nhktmdzhg merged 5 commits into
LotusInputMethod:masterfrom
hthienloc:feat/w2u-modular

Conversation

@hthienloc
Copy link
Copy Markdown

Implement a unified 3-state W2U toggle (Disabled, Middle-Only, Everywhere). Middle-only mode ignores 'w' at the start of words to prevent interference with English characters while providing Vietnamese shortcuts elsewhere.

@hthienloc hthienloc marked this pull request as draft April 13, 2026 23:25
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a modular W2U (w to ư) logic by adding a w2uMode field and a SetW2UMode method to the BambooEngine, replacing the previous flag-based implementation. Feedback includes a critical note regarding a regression where the new mode is not initialized from existing flags in NewEngine, which would disable the feature for current users. Additionally, it is recommended to simplify the boolean logic for canApplyW2U, replace magic numbers with named constants for better maintainability, and add input validation to the SetW2UMode method.

Comment thread bamboo.go Outdated
Comment on lines +134 to +141
canApplyW2U := false
if e.w2uMode == 1 {
if len(composition) > 0 {
canApplyW2U = true
}
} else if e.w2uMode == 2 {
canApplyW2U = true
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The logic for determining canApplyW2U can be simplified for better readability.

Important: There is a potential regression here. The previous implementation checked the Ew2uEnabled flag. Since w2uMode defaults to 0 and is not initialized from the flags in NewEngine, existing users who rely on Ew2uEnabled (which is part of EstdFlags) will find the W2U feature disabled by default. Please ensure w2uMode is correctly initialized in NewEngine based on the provided flags.

Suggested change
canApplyW2U := false
if e.w2uMode == 1 {
if len(composition) > 0 {
canApplyW2U = true
}
} else if e.w2uMode == 2 {
canApplyW2U = true
}
canApplyW2U := (e.w2uMode == 2) || (e.w2uMode == 1 && len(composition) > 0)

Comment thread bamboo.go Outdated
composition []*Transformation
inputMethod InputMethod
flags uint
w2uMode int // 0: Disabled, 1: Middle-Only, 2: Everywhere
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using magic numbers (0, 1, 2) for w2uMode makes the code harder to read and maintain. It is recommended to define named constants or a custom type for these modes to improve clarity.

Comment thread bamboo.go
@hthienloc hthienloc marked this pull request as ready for review April 13, 2026 23:50
@nhktmdzhg nhktmdzhg merged commit 86aa425 into LotusInputMethod:master Apr 14, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants