search

CommunityToolkit / WindowsCommunityToolkit

The Windows Community Toolkit is a collection of helpers, extensions, and custom controls. It simplifies and demonstrates common developer tasks building .NET apps with UWP and the Windows App SDK / WinUI 3 for Windows 10 and Windows 11. The toolkit is part of the .NET Foundation.
https://docs.microsoft.com/windows/communitytoolkit/
Other
5.82k stars 1.37k forks source link

TokenizingTextBox exposes InterspersedObservableCollection instead of ObservableCollection #4248

Open michael-hawker opened 2 years ago

michael-hawker commented 2 years ago

Describe the bug

The original intent of the TokenizingTextBox was to not expose the internal collection type of InterspersedObservableCollection, and by providing an ObservableCollection to the ItemsSource property, a developer could retrieve only the tokenized elements from the control for their datamodel (or provide them as a pre-population/rehydration on a form).

A developer could then access the Text or raw Items properties to enumerate all partial strings with the Tokens if needed.

However, in the current implementation, we overwrite the ItemsSource value with our InterspersedObservableCollection reference here:

https://github.com/CommunityToolkit/WindowsCommunityToolkit/blob/03c3f3c262ea395e66098b8de7945f3651a6bf37/Microsoft.Toolkit.Uwp.UI.Controls.Input/TokenizingTextBox/TokenizingTextBox.cs#L62

And more importantly here:

https://github.com/CommunityToolkit/WindowsCommunityToolkit/blob/03c3f3c262ea395e66098b8de7945f3651a6bf37/Microsoft.Toolkit.Uwp.UI.Controls.Input/TokenizingTextBox/TokenizingTextBox.cs#L94

This means if you try to setup the scenario as we were in the current Sample app or Graph Sample App (see https://github.com/CommunityToolkit/Graph-Controls/issues/149) binding to the ItemsSource property of the TTB directly, you'd get the internal copy of the InterspersedObservableCollection instead of the inner ObservableCollection of only tokened items.

Steps to Reproduce

We can actually observe this in our own sample app (but it was invisible) so we missed it during initial development.

Steps to reproduce the behavior:

  1. Open the TTB page
  2. Go to the Live Visual Tree tools and select the "Current Edit" text
  3. In the Live Visual Tree find the ItemsControl next that's supposed to show the tokenized items.
  4. Note that there'll be a ContentPresenter showing a empty TextBlock, this is a PretokenStingContainer element which shouldn't be part of our collection (only ever exposed on Items and for ToString to get value).

We should expect the ItemsControl to be empty at this time.

We can also further show this corruption by performing a few steps.

  1. Add any two elements and some text:

image

  1. Click on the first token
  2. Type some visible characters, note now that we see the first letter that was set to a PretokenStringContainer within our collection 😲

image

This should not be the case!

Expected behavior

Binding to the ItemsSource even if we don't provide an external collection should only provide the tokenized items. We should only be able to observe the PretokenStringContainer within the Items collection which a developer would check for their own type or call ToString to get the raw values of typed text.

Screenshots

image

Environment

NuGet Package(s): Input

Package Version(s): 7.1.0-rc1

Windows 10 Build Number:

App min and target version:

Device form factor:

Visual Studio version:

Additional context

Updated the Graph Sample for now to avoid this issue and show the more practical pattern here: https://github.com/CommunityToolkit/Graph-Controls/pull/160

Should still resolve this issue here.

I believe we did this the way we did initially as since we're inheriting from ListViewBase, we use ItemsPresenter to present the items here:

https://github.com/CommunityToolkit/WindowsCommunityToolkit/blob/03c3f3c262ea395e66098b8de7945f3651a6bf37/Microsoft.Toolkit.Uwp.UI.Controls.Input/TokenizingTextBox/TokenizingTextBox.xaml#L111-L112

ItemsPresenter grabs the ItemsSource automatically... it's not something we can set. I'm not sure how we override it either, so this is going to need some thought... 🤔

michael-hawker commented 2 years ago

I've at least started on a fix for the sample itself to work-around the issue here: d528479b09037a85682b0346e12454b0369225d9

I think it's going to be tricky to resolve out-right though as ItemsSource is automatically picked up by the ItemsPresenter, so we need it to be the full collection vs. what we want exposed through the property externally...

I'm not sure what alternatives we may have here to work around this. I did just try to swap to an ItemsControl, but that crashed in the XAML layer somewhere for unknown reasons...

❓

Thinking due to the complexity here (and that it took us a year+ to hit ourselves) that we should move out of 7.1.