-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Implement IsTextPredictionEnabled in IEntry Handlers #423
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement IsTextPredictionEnabled in IEntry Handlers #423
Conversation
hartez
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks!
|
/azp run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. Looking good and I just have 2 thoughts.
Not for this PR, but a note for future me and other peoples. We need to create a helper method to test changes. Like init to X, test, set to Y, and confirm it changes. And then do the reverse order.
This is very useful to determine if the "undo" operation works. Like set IsPassword to false, test, true, test and then false again, test.
| editText.InputType |= InputTypes.NumberVariationPassword; | ||
|
|
||
| if (!entry.IsTextPredictionEnabled && ((editText.InputType & InputTypes.TextFlagNoSuggestions) != InputTypes.TextFlagNoSuggestions)) | ||
| editText.InputType |= InputTypes.TextFlagNoSuggestions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am thinking this is unnecessary setting of the native property. Even though it is fast and, maybe we can use a variable and pipe them all. Then at the end, we set it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This enhancement is addressed in #507.
mattleibow
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marking this red just in case I am right and we can't unset the value.
|
@mattleibow can you please take a look now? :) |
|
@almirvuk could you add this test just so we don't ever break this feature in the future: [Theory(DisplayName = "IsTextPredictionEnabled Updates Correctly")]
[InlineData(true, true)]
[InlineData(true, false)]
[InlineData(false, true)]
[InlineData(false, false)]
public async Task IsTextPredictionEnabledUpdatesCorrectly(bool setValue, bool unsetValue)
{
var entry = new EntryStub();
await ValidatePropertyUpdatesValue(
entry,
nameof(IEntry.IsTextPredictionEnabled),
GetNativeIsTextPredictionEnabled,
setValue,
unsetValue);
} |
mattleibow
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great! Besides the test that I think we should add, this is good to merge!
Thanks for the PR and the work.
Added with the last commit :) Thank you @mattleibow |
|
Absolutely stunning! This needs to be merged as soon as CI is green. |
|
/azp run |
|
No pipelines are associated with this pull request. |
Implements #381