Allow tone key to revise prior reading (#753) - #754
Conversation
Summary of ChangesHello @zonble, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement to the Bopomofo input method, allowing users to correct or change the tone of a previously entered syllable without retyping the entire reading. By detecting a lone tone key press, the system now dynamically updates the preceding syllable, improving the efficiency and user experience of tone input and correction. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully implements the feature to allow revising the tone of a prior reading. The logic seems sound and follows the description. I've provided a couple of review comments. One is a minor naming suggestion for the new API to improve consistency. The other is a more significant suggestion to use stack allocation instead of manual memory management with new and delete for a temporary object, which is a safer and more idiomatic C++ practice.
There was a problem hiding this comment.
Pull request overview
This PR implements feature request #753, allowing users to revise the tone of a previously entered BPMF reading by pressing a tone key when the reading buffer contains only a tone marker. The implementation adds a new API method to BopomofoReadingBuffer and logic in KeyHandler to detect and handle tone key presses that modify prior readings.
Key Changes
- Added
setsSyllable()method toBopomofoReadingBufferclass to allow direct syllable assignment - Implemented tone revision logic that creates a temporary buffer, reconstructs the prior syllable with the new tone, validates it against the language model, and updates the grid
- Refactored the
isValidKeycheck to avoid redundant calls
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| Source/Engine/Mandarin/Mandarin.h | Added new setsSyllable() method to enable direct syllable assignment in BopomofoReadingBuffer |
| Source/KeyHandler.mm | Implemented tone key revision feature with validation and grid update logic; refactored duplicate isValidKey calls |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature that allows users to change the tone of a previous Bopomofo reading by pressing a tone key. The implementation involves adding a new method to BopomofoReadingBuffer to set a syllable without its tone, and updating KeyHandler to manage this new behavior. New tests have been added to cover this functionality.
My review focuses on improving code quality and maintainability. I've suggested correcting typos in test function names and refactoring duplicated code in the new tests. I also pointed out a minor performance improvement in the key handler logic. Overall, the changes are well-implemented and the new feature is a good addition.
| // the current cursor. | ||
| if (_bpmfReadingBuffer->hasToneMarkerOnly() && _grid->readings().size() > 0 && _grid->cursor() > 0) { | ||
| size_t cursor = _grid->cursor() - 1; | ||
| std::string reading = _grid->readings()[cursor]; |
There was a problem hiding this comment.
To avoid a potentially unnecessary string copy, you can declare reading as a const reference.
| std::string reading = _grid->readings()[cursor]; | |
| const std::string& reading = _grid->readings()[cursor]; |
There was a problem hiding this comment.
我對直接對所有用戶開啟這個功能，有幾個疑慮。
我覺得這功能應該要透過一個進階設定（我偏向透過終端機自行開啟的進階隱藏設定）來開啟。這樣表明：這個功能有侷限，但可以便利特定打字習慣。這個功能也不保證總是和蘋果內建輸入法有完全一樣的行為。
再來，這個功能只支援標準配置以及倚天配置的用戶，因為許氏鍵盤以及倚天 26 鍵的聲調仰賴先前輸入的狀態。好比說在倚天 26 鍵下，bef 跟 bek 代表「ㄅㄧˊ」跟「ㄅㄧˋ」，但是打 befk 並不會把「鼻」改成「必」，因為 k 在初始狀態下代表「ㄎ」而不是四聲。拼音一樣不可能支援，因為 Mandarin 並沒有設計讓拼音單獨輸入聲調。
另外，這個功能顯然得依照漢音「游標前選字」的邏輯才會成立，但這樣我們等於造成了 mental model 的不一致：我們已經有了用 Tab 快速循環選字的功能，而 Tab 循環選字的邏輯是跟著游標前/後選字的邏輯一致的。然後這個「按聲調，快速更改前一個字聲調」的功能，顯然只對漢音式選字的用戶有意義。我甚至擔心對新注音選字模式的用戶來說，這個功能反而增加了一條不易理解的 implicit behavior。這是我覺得這個功能只能放在用戶自行開啟進階設定的原因。
回到開頭說的，這個功能雖然容許單一一項蘋果內建輸入法的輸入習慣，但終究我們做了跟內建輸入法很不一樣的選擇。內建注音在緩衝區為空的時候，可以直接輸入調符就輸出（附帶一提，這是內建輸入法很容易「打出我不要的調符」的原因），但緩衝區有字的時候，就沒有把調符單獨輸入的可能了（因為總是會搭上其他聲母或韻母而組字，或是修改前一個字的聲調）。我對於 implicit behavior 也有疑慮，因為聲調鍵跟其他按鍵 (Tab, Ctrl-Space) 不一樣，都是在輸入過程中常（誤）按到的鍵，我擔憂直接對所有人開啟這個功能，會造成對此功能無需求用戶的困擾。
我的想法是，我們實作這一項需求很好，但是否該把這個功能的侷限說明清楚（列為從終端機開啟的選項，一定程度上表明了這個侷限），也減少日後維護的複雜度。
|
|
||
| const BPMF syllable() const { return syllable_; } | ||
|
|
||
| void setSyllableRemovingTone(BPMF syllable) { |
There was a problem hiding this comment.
為了這個只會用到一次的 feature 而在底層程式庫加入這個 helper，似乎沒有必要。請看我下面的留言。
| if (!reading.empty() && reading[0] != '_') { | ||
| Formosa::Mandarin::BopomofoReadingBuffer tmpBuffer(_bpmfReadingBuffer->keyboardLayout()); | ||
| Formosa::Mandarin::BopomofoSyllable syllable = Formosa::Mandarin::BopomofoSyllable::FromComposedString(reading); | ||
| tmpBuffer.setSyllableRemovingTone(syllable); |
There was a problem hiding this comment.
我會考慮換一個作法，這樣就不需要在 Mandarin.h 中加入那樣一個一次性、為單一 feature 設計的 helper method：
- 取得現在的 keyboard layout
- 呼叫
BopomofoKeyboardLayout::keySequenceFromSyllable() - 建立新的
BopomofoReadingBuffer物件，然後把上一步得到的 key sequence 透過combineKey()丟進去 - 再呼叫
combineKey()把最新的聲調符號組合進去，這樣等同取代現有聲調
無論哪種作法，倚天 26 鍵、許氏跟拼音都無法支援（請看我頂層的留言），但至少上述作法可以免除在 Mandarin.h 中放入新 helper。
|
Added a new key to enabled the feature |
lukhnos
left a comment
lukhnos
left a comment
There was a problem hiding this comment.
LGTM, thanks! Just one request to change the evaluation order and one nit.
The PR implements the feature request in #753. It allows users to change the tone of a prior BPMF reading.
When a user presses a tone key without any other syllabels, the key handler tries to convert the reading before the cursor to syllabel and then append the new tone key. Finally, it delete the original reading and insert a new one.
The PR add a new API to allow chaning the syllabel of
BopomofoReadingBuffer.