refactor: extract SuggestionUIHandler from GeneralKeyboardIME (Part 11) (#426) - #685
Conversation
74bf7dc to
e5ec7f6
Compare
|
Thanks for the PR, @prince-0408! We'll get this PR in soon after bringing in the autosuggestion PRs 😊 |
|
question: Would you be able to rebase on main, @prince-0408? We'd then get to the review :) note: Sorry that you'll likely need to do so much rebasing here, but great that we can bring all of these in soon! |
d1107db to
c3ac1c3
Compare
Hi @andrewtavis @angrezichatterbox, I have rebased the branch onto the latest This PR is now ready for re-review! 🙌 |
|
Thanks for the update, @prince-0408! We'll get to the review soon. CC also @Roniscend in case you have a moment :) |
| */ | ||
| @Suppress("TooManyFunctions") | ||
| class SuggestionUIHandler( | ||
| private val ime: GeneralKeyboardIME, |
There was a problem hiding this comment.
Could this take KeyboardIMEContext instead of GeneralKeyboardIME, like the other handlers since #693? If the interface is missing a member or two it needs, adding them there would keep the decoupling consistent.
There was a problem hiding this comment.
Could this take KeyboardIMEContext instead of GeneralKeyboardIME, like the other handlers since #693? If the interface is missing a member or two it needs, adding them there would keep the decoupling consistent.
Good catch! Updated SuggestionUIHandler to take KeyboardIMEContext and added themeManager to the interface to maintain consistency with the other handlers.
| val default1 = baseSuggestions.getOrNull(0).orEmpty() | ||
| val default2 = baseSuggestions.getOrNull(1).orEmpty() | ||
| setSuggestionButton(uiManager.binding.conjugateBtn, default1) | ||
| uiManager.pluralBtn?.let { setSuggestionButton(it, default2) } |
There was a problem hiding this comment.
This drops the autoSuggestEmojis check the old code had here
There was a problem hiding this comment.
This drops the autoSuggestEmojis check the old code had here
Nice catch! Restored the autoSuggestEmojis check in handleWordSuggestions and added a unit test covering this scenario.
c3ac1c3 to
9e93350
Compare
|
Hi @Roniscend @andrewtavis @angrezichatterbox, Ready for re-review and merge! 🙌 |
| private val ime: KeyboardIMEContext, | ||
| ) { | ||
| companion object { | ||
| const val NOUN_TYPE_SIZE = 20f |
There was a problem hiding this comment.
The font-size constants are now defined twice, NOUN_TYPE_SIZE is now unused and can be deleted
| @@ -0,0 +1,126 @@ | |||
| // SPDX-License-Identifier: GPL-3.0-or-later | |||
|
|
|||
There was a problem hiding this comment.
The tests mostly check that fields were copied, not how the strip is drawn, Please update them
9e93350 to
149ea9d
Compare
Roniscend
left a comment
Roniscend
left a comment
There was a problem hiding this comment.
@prince-0408 Two things are left the four commandOptionsBar == GONE assertions pass even if the handler does nothing (it starts out gone and disableAutoSuggest() never touches it), so please check the default suggestion text on the buttons instead. And the emoji test no longer covers the gender + no-word-suggestions + emojis case, so could you restore that setup?
4 participants
Description
This PR is Part 11 in modularizing
GeneralKeyboardIMEfor #426.It extracts auto-suggestion layout rendering, button styling, visibility toggling (between command buttons, gender suggestions, and emoji recommendations), and autocomplete button click handlers out of
GeneralKeyboardIME.ktinto a standalone helper classSuggestionUIHandler.Detailed Changes Table
SuggestionUIHandler.ktupdateAutoSuggestText), single/dual suggestion button styling (handleSingleType,handleMultipleNounFormats), word prediction updates (handleWordSuggestions), and autocomplete completions (updateAutocompleteSuggestions,clearAutocomplete).GeneralKeyboardIME.ktinto a focused helper component.SuggestionUIHandlerTest.ktSuggestionUIHandler.GeneralKeyboardIME.ktsuggestionUIHandlerand delegatedupdateAutoSuggestText,updateAutocompleteSuggestions, andclearAutocompleteto it. Removed private helper methods and unused extension functions.GeneralKeyboardIME.ktsize by ~400 lines (from 2,061 lines down to 1,662 lines) while maintaining 100% backward compatibility.Key Benefits
SuggestionUIHandlerTest.kttesting layout toggles and state synchronization.Related Issue
Refactors part of #426