Ports the associated phrases feature from the macOS version #107
Conversation
There was a problem hiding this comment.
LGTM overall. I made some comments re the C++ coding convention, and please take a look at #108, since I discovered a crasher due to KeyHandler's using the state callback twice while handing the candidate event for plain Bopomofo mode when associated phrases are enabled.
| @@ -27,7 +27,9 @@ | |||
| #include <array> | |||
| #include <cassert> | |||
| #include <cstdint> | |||
| #include <functional> | |||
There was a problem hiding this comment.
Could you also bring the diff in reading_grid_test.cpp to here so that we can make sure 'findInSpan` has its proper test coverage?
src/InputState.h
Outdated
| @@ -84,6 +84,10 @@ struct Inputting : NotEmpty { | |||
| Inputting(const std::string& buf, const size_t index, | |||
| const std::string_view& tooltipText = "") | |||
| : NotEmpty(buf, index, tooltipText) {} | |||
|
|
|||
| std::unique_ptr<Inputting> copy() { | |||
There was a problem hiding this comment.
Prefer creating a copy constructor and then call std::make_unque<T> at the call site, so that we don't need to depend on std::unique_ptr<T> here:
Inputting(const Inputting& o) : NotEmpty(o.buf, o.index, o.tooltipText) {}| std::string phrase = | ||
| candidateList->candidate(selectedIndex).text().toString(); | ||
| #endif | ||
| choosingCandidate->candidates[selectedCandidateIndex].value; | ||
| std::unique_ptr<InputStates::ChoosingCandidate> copy = |
There was a problem hiding this comment.
If you have the copy constructor, then you can do this:
auto copy = std::make_unique<InputStates::ChoosingCandidate>(*choosingCandidate);
src/KeyHandler.cpp
Outdated
| std::unique_ptr<InputStates::AssociatedPhrasesPlain> newState = | ||
| buildAssociatedPhrasesPlainState(value); | ||
| if (newState != nullptr) { | ||
| stateCallback(std::move(newState)); |
There was a problem hiding this comment.
The stateCallback is no longer valid (it's now a const reference to a dangling object) because the commit state above will cause the candidate object (which owns the callback object) to be destoryed.
Please review #108; once it's merged, please merge with the mainline so that this will work safely.
| @@ -76,6 +76,10 @@ class KeyHandler { | |||
| const InputStates::ChoosingCandidate::Candidate& candidate, | |||
| const StateCallback& stateCallback); | |||
|
|
|||
| void candidateAssociatedPhraseSelected( | |||
There was a problem hiding this comment.
Due to the issue discovered in #108, please make the callback parameter StateCallback stateCallback instead of const StateCallback& stateCallback).
| @@ -118,5 +118,39 @@ std::string SubstringToCodePoints(const std::string& s, size_t cp) { | |||
| return {s.cbegin(), i}; | |||
| } | |||
|
|
|||
| std::string GetCodePoint(const std::string& s, size_t cp) { | |||
There was a problem hiding this comment.
Could we add some tests for this function and the Split function below?
A new settings is added. When the setting is on, a user can see a menu of associated phrases menu each time enter a character in the Plain Bopomofo mode. He or she can also trigger associated phrases using the shift + enter key in the McBopomofo mode.