Fix Text initialization and updating for TokenizingTextBox #4786
Conversation
This file contains 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
michael-hawker
added
bug 🐛
|
Thanks michael-hawker for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌 |
ghost
requested a review
from
michael-hawker
changed the title
|
Ah, right if I change the release stuff in the solution than it'll mess with the CI and try and build the sample app there too... will fix. |
michael-hawker
force-pushed
the
llama/tokenizingtextbox-fix4749
branch
from
1758c71 to
37d381f
Compare
|
@michael-hawker Tested the repro step you mentioned by setting the Textproperty of the TokenizingTextBox, and it seems to bind correctly! |
niels9001
reviewed
Oct 18, 2022
| @@ -89,7 +89,8 @@ private void ItemsSource_PropertyChanged(DependencyObject sender, DependencyProp | |||
| } | |||
| } | |||
|
|
|||
| _currentTextEdit = _lastTextEdit = new PretokenStringContainer(true); | |||
| // Add our text box at the end of items and set it's default value to our initial text, fix for #4749 | |||
There was a problem hiding this comment.
Super minor nit: its vs it's. So please ignore :)
niels9001
approved these changes
Oct 18, 2022
michael-hawker
commented
Oct 18, 2022
Microsoft.Toolkit.Uwp.UI.Controls.Input/TokenizingTextBox/TokenizingTextBox.cs
Outdated
Show resolved
Hide resolved
(Don't set Release as that effects CI)
See https://devblogs.microsoft.com/nuget/announcing-nuget-6-3-transitive-dependencies-floating-versions-and-re-enabling-signed-package-verification/#new-warnings-when-duplicate-packagereference-packageversion-or-packagedownload-are-found for info about increased warning level. Also see dotnet/sdk#24747 Not sure where the duplicate reference is coming from though...
… use proper async tests Also splits out Clear test into two separate tests.
…nityToolkit#4749 Think we maybe used to bind to the text directly, but there's no Text property directly on the TokenizingTextBoxItem, so this binding is meaningless. It doesn't effect the behavior of the textbox in practical usage, but somehow was doing something which was masking the problem for us to be able to detect within a test case. Removing it to reduce confusion. The text sync between the parent TokenizingTextBox collection of items (and the current edit) and the item is maintained through code-behind with text changing events. Though work is being done to resolve issues in this sync process. See issue CommunityToolkit#4749
Hook up initialization and change detection of parent Text property to update corresponding inner text of TokenizingTextBoxItem Failing tests from previous commit now pass.
…g token wasn't being set to box (aggressive if) Also realized we would no longer raise the text changed event in that scenario, so changed logic for text changed event. We'll need to create integration tests for these keyboard driven scenarios in the new test setup from Labs (when it's finished) in 8.0. These would make good keyboard driver tests.
michael-hawker
force-pushed
the
llama/tokenizingtextbox-fix4749
branch
from
b8d4300 to
cd1afd0
Compare
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 #4749
Addresses issues with how
Textis handled with theTokenizingTextBoxinitialization and changes.PR Type
What kind of change does this PR introduce?
What is the current behavior?
Initial text of the TokenizingTextBox was not being properly setup via an invalid binding. This would mean it would not clear when the clear method was called, nor could its initial text be set and appear in the box at runtime.
This could be reproduced in the Sample App
What is the new behavior?
PR Checklist
Please check if your PR fulfills the following requirements:
Other information