Repository navigation
Add RawAutocompleteController - #171929
Closed
chrisbobbe wants to merge 8 commits into
Closed
chrisbobbe wants to merge 8 commits into
chrisbobbe wants to merge 8 commits into
Conversation
Currently trivial, but we'll add more here soon.
And keep a [highlightedOptionIndexNotifier], since a caller needs that value.
Currently it doesn't have any listeners, but it will soon.
This isn't nearly ready for merge; it's just to demonstrate a direction that might be helpful for zulip/zulip-flutte#226, and perhaps also for: flutter#123377 (comment) The idea (for zulip/zulip-flutte#226) is to move to a new class RawAutocompleteController the state value for the current list of autocomplete options. Then, to let RawAutocomplete's caller pass an instance of that class, giving the caller control over when that state gets updated. For us, that's better than accepting RawAutocomplete's old behavior, where a change in the TextEditingController was the only trigger for an update. (We want to trigger updates when our MentionAutocompleteView changes.) Ideally, we could add this capability to RawAutocomplete without removing any of its current API: maybe we could make the `controller` param optional, and I guess add asserts to try to prevent buggy mixtures of the old and this new opt-in behavior. Anyway, that that was way too tricky for this demo, so here I've just ripped out all the parts where RawAutocomplete did anything with the TextEditingController. I think our zulip/zulip-flutter#226 doesn't create a need to control the "selection" state, but that state is also moved to the RawAutocompleteController class anyway. Why? - Well, with RawAutocomplete being unaware of the text-input value, as mentioned in the previous paragraph, it can't listen for changes in that value, which it was doing before, to null out the selection state when the text changed. So, now we do that. - Also, I think there's a proposal for class like this RawAutocompleteController to contain the selection state: flutter#123377 (comment) That comment would also have it include the highlighted state, I think (which the auther calls the "hovered" state). Since those are closely related to the options-list state, might as well put them there along with it. For this demo, I hacked down the Material `Autocomplete` widget so that zulip-flutter would build, and I haven't touched any of the Flutter framework tests.
Contributor
|
Hi @chrisbobbe! Thank you for contributing! It looks like this never entered our review queue since it is still marked as a draft. Did you still want to pursue this change? |
Contributor
|
Since we have not heard back for a while, I am going to close this PR. Please do feel free to open a new one if you would like to contribute. Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #159443.
Context: #159443 (comment)