Skip to content
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.

Sign up for GitHub

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

Added support for limiting max tokens in TokenizingTextBox #4163

Merged
merged 16 commits into from
Aug 13, 2021
+177 −16
Merged

Added support for limiting max tokens in TokenizingTextBox #4163

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
fafbf5b
Added support for TokenSelectionMode in TokenizingTextBox
shweaver-MSFT Aug 4, 2021
fbeb2c4
Added TokenItemRemoved events
shweaver-MSFT Aug 4, 2021
93ca18a
Replaced TokenSelectionMode with MaxTokens property on TokenizingTextBox
shweaver-MSFT Aug 5, 2021
9ee8b96
Removed TokenSelectionMode enum
shweaver-MSFT Aug 5, 2021
b4fecf3
Accounting for negative MaxToken value
shweaver-MSFT Aug 5, 2021
358ada4
Added MaxTokens test
shweaver-MSFT Aug 6, 2021
f0d8b2a
Made MaxTokens non-nullable and added checks for DP.UnsetValue
shweaver-MSFT Aug 6, 2021
1309ca1
Update Microsoft.Toolkit.Uwp.UI.Controls.Input/TokenizingTextBox/Toke…
shweaver-MSFT Aug 6, 2021
c7b55c6
Updated OnMaxTokensChanged method and improved MaxTokens test
shweaver-MSFT Aug 9, 2021
07f19d5
Bug fixes
shweaver-MSFT Aug 9, 2021
01c413b
bugfix bug fixes
shweaver-MSFT Aug 10, 2021
a7bb51b
Added binding to MaxTokens value in sample
shweaver-MSFT Aug 10, 2021
c3903ab
Updated TokenizingTextBox.MaxTokens test
shweaver-MSFT Aug 10, 2021
86cd4d6
Updated Max to Maximum and added token counter
shweaver-MSFT Aug 12, 2021
2d118e8
Updated GetValue to ReadLocalValue
shweaver-MSFT Aug 13, 2021
ccefe38
Removing Math reference
shweaver-MSFT Aug 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Microsoft.Toolkit.Uwp.SampleApp/SamplePages/TokenizingTextBox/TokenizingTextBoxXaml.bind
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
<StackPanel>
<TextBlock FontSize="32" Margin="0,0,0,4">
<Run Text="Select up to" />
<Run Text="{Binding MaxTokens, ElementName=TokenBox, Mode=OneWay}" />
<Run Text="{Binding MaximumTokens, ElementName=TokenBox, Mode=OneWay}" />
<Run Text="actions" />
</TextBlock>
<controls:TokenizingTextBox
Expand All @@ -43,7 +43,7 @@
HorizontalAlignment="Stretch"
TextMemberPath="Text"
TokenDelimiter=","
MaxTokens="3">
MaximumTokens="3">
<controls:TokenizingTextBox.SuggestedItemTemplate>
<DataTemplate>
<StackPanel Orientation="Horizontal">
Expand Down
18 changes: 9 additions & 9 deletions Microsoft.Toolkit.Uwp.UI.Controls.Input/TokenizingTextBox/TokenizingTextBox.Properties.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,17 +158,17 @@ private static void TextPropertyChanged(DependencyObject d, DependencyPropertyCh
new PropertyMetadata(false));

/// <summary>
/// Identifies the <see cref="MaxTokens"/> property.
/// Identifies the <see cref="MaximumTokens"/> property.
/// </summary>
public static readonly DependencyProperty MaxTokensProperty = DependencyProperty.Register(
nameof(MaxTokens),
public static readonly DependencyProperty MaximumTokensProperty = DependencyProperty.Register(
nameof(MaximumTokens),
typeof(int),
typeof(TokenizingTextBox),
new PropertyMetadata(null, OnMaxTokensChanged));
new PropertyMetadata(null, OnMaximumTokensChanged));

private static void OnMaxTokensChanged(DependencyObject d, DependencyPropertyChangedEventArgs e)
private static void OnMaximumTokensChanged(DependencyObject d, DependencyPropertyChangedEventArgs e)
{
if (d is TokenizingTextBox ttb && ttb.ReadLocalValue(MaxTokensProperty) != DependencyProperty.UnsetValue && e.NewValue is int newMaxTokens)
if (d is TokenizingTextBox ttb && ttb.ReadLocalValue(MaximumTokensProperty) != DependencyProperty.UnsetValue && e.NewValue is int newMaxTokens)
{
var tokenCount = ttb._innerItemsSource.ItemsSource.Count;
if (tokenCount > 0 && tokenCount > newMaxTokens)
Expand Down Expand Up @@ -338,10 +338,10 @@ public string SelectedTokenText
/// <summary>
/// Gets or sets the maximum number of token results allowed at a time.
/// </summary>
public int MaxTokens
public int MaximumTokens
{
get => (int)GetValue(MaxTokensProperty);
set => SetValue(MaxTokensProperty, value);
get => (int)GetValue(MaximumTokensProperty);
set => SetValue(MaximumTokensProperty, value);
}
}
}
46 changes: 3 additions & 43 deletions Microsoft.Toolkit.Uwp.UI.Controls.Input/TokenizingTextBox/TokenizingTextBox.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,10 @@ private void ItemsSource_PropertyChanged(DependencyObject sender, DependencyProp
{
_innerItemsSource = new InterspersedObservableCollection(ItemsSource);

if (ReadLocalValue(MaxTokensProperty) != DependencyProperty.UnsetValue && _innerItemsSource.ItemsSource.Count > MaxTokens)
if (ReadLocalValue(MaximumTokensProperty) != DependencyProperty.UnsetValue && _innerItemsSource.ItemsSource.Count > MaximumTokens)
{
// Reduce down to the max as necessary.
for (var i = _innerItemsSource.ItemsSource.Count - 1; i >= Math.Max(MaxTokens, 0); --i)
for (var i = _innerItemsSource.ItemsSource.Count - 1; i >= Math.Max(MaximumTokens, 0); --i)
{
_innerItemsSource.Remove(_innerItemsSource[i]);
}
Expand Down Expand Up @@ -440,7 +440,7 @@ public async Task ClearAsync()

internal async Task AddTokenAsync(object data, bool? atEnd = null)
{
if (ReadLocalValue(MaxTokensProperty) != DependencyProperty.UnsetValue && MaxTokens <= 0)
if (ReadLocalValue(MaximumTokensProperty) != DependencyProperty.UnsetValue && (MaximumTokens <= 0 || MaximumTokens <= _innerItemsSource.ItemsSource.Count))
{
// No tokens for you
return;
Expand All @@ -465,26 +465,6 @@ internal async Task AddTokenAsync(object data, bool? atEnd = null)
// If we've been typing in the last box, just add this to the end of our collection
if (atEnd == true || _currentTextEdit == _lastTextEdit)
{
if (ReadLocalValue(MaxTokensProperty) != DependencyProperty.UnsetValue && _innerItemsSource.ItemsSource.Count >= MaxTokens)
{
// Remove tokens from the end until below the max number.
for (var i = _innerItemsSource.Count - 2; i >= 0; --i)
{
var item = _innerItemsSource[i];
if (item is not ITokenStringContainer)
{
_innerItemsSource.Remove(item);
TokenItemRemoved?.Invoke(this, item);

// Keep going until we are below the max.
if (_innerItemsSource.ItemsSource.Count < MaxTokens)
{
break;
}
}
}
}

_innerItemsSource.InsertAt(_innerItemsSource.Count - 1, data);
}
else
Expand All @@ -493,26 +473,6 @@ internal async Task AddTokenAsync(object data, bool? atEnd = null)
var edit = _currentTextEdit;
var index = _innerItemsSource.IndexOf(edit);

if (ReadLocalValue(MaxTokensProperty) != DependencyProperty.UnsetValue && _innerItemsSource.ItemsSource.Count >= MaxTokens)
{
// Find the next token and remove it, until below the max number of tokens.
for (var i = index; i < _innerItemsSource.Count; i++)
{
var item = _innerItemsSource[i];
if (item is not ITokenStringContainer)
{
_innerItemsSource.Remove(item);
TokenItemRemoved?.Invoke(this, item);

// Keep going until we are below the max.
if (_innerItemsSource.ItemsSource.Count < MaxTokens)
{
break;
}
}
}
}

// Insert our new data item at the location of our textbox
_innerItemsSource.InsertAt(index, data);

Expand Down
47 changes: 47 additions & 0 deletions ...t.Toolkit.Uwp.UI.Controls.Input/TokenizingTextBox/TokenizingTextBoxItem.AutoSuggestBox.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@

using Windows.Foundation;
using Windows.System;
using Windows.UI;
using Windows.UI.Xaml;
using Windows.UI.Xaml.Controls;
using Windows.UI.Xaml.Data;
using Windows.UI.Xaml.Input;
using Windows.UI.Xaml.Media;

namespace Microsoft.Toolkit.Uwp.UI.Controls
{
Expand All @@ -16,9 +18,11 @@ namespace Microsoft.Toolkit.Uwp.UI.Controls
/// </summary>
[System.Diagnostics.CodeAnalysis.SuppressMessage("StyleCop.CSharp.ReadabilityRules", "SA1124:Do not use regions", Justification = "Organization")]
[TemplatePart(Name = PART_AutoSuggestBox, Type = typeof(AutoSuggestBox))] //// String case
[TemplatePart(Name = PART_TokensCounter, Type = typeof(TextBlock))]
public partial class TokenizingTextBoxItem
{
private const string PART_AutoSuggestBox = "PART_AutoSuggestBox";
private const string PART_TokensCounter = "PART_TokensCounter";

private AutoSuggestBox _autoSuggestBox;

Expand Down Expand Up @@ -231,6 +235,8 @@ private void AutoSuggestBox_GotFocus(object sender, RoutedEventArgs e)
#region Inner TextBox
private void OnASBLoaded(object sender, RoutedEventArgs e)
{
UpdateTokensCounter(this);

// Local function for Selection changed
void AutoSuggestTextBox_SelectionChanged(object box, RoutedEventArgs args)
{
Expand Down Expand Up @@ -329,6 +335,47 @@ private void AutoSuggestTextBox_PreviewKeyDown(object sender, KeyRoutedEventArgs
Owner.SelectAllTokensAndText();
}
}

private void UpdateTokensCounter(TokenizingTextBoxItem ttbi)
{
var maxTokensCounter = (TextBlock)_autoSuggestBox?.FindDescendant(PART_TokensCounter);
if (maxTokensCounter == null)
{
return;
}

void OnTokenCountChanged(TokenizingTextBox ttb, object value = null)
{
var itemsSource = ttb.ItemsSource as InterspersedObservableCollection;
var currentTokens = itemsSource.ItemsSource.Count;
var maxTokens = ttb.MaximumTokens;

maxTokensCounter.Text = $"{currentTokens}/{maxTokens}";
maxTokensCounter.Visibility = Visibility.Visible;

maxTokensCounter.Foreground = (currentTokens == maxTokens)
? new SolidColorBrush(Colors.Red)
: _autoSuggestBox.Foreground;
}

ttbi.Owner.TokenItemAdded -= OnTokenCountChanged;
ttbi.Owner.TokenItemRemoved -= OnTokenCountChanged;

// I would have like to compared to DependencyProperty.UnsetValue, but MaximumTokensProperty value is returning 0 even though we didn't set it!
michael-hawker marked this conversation as resolved.
Show resolved Hide resolved
// This means that the token counter will not show up for a specified maximum value of 0. However, it's a pretty uncommon scenario to offer a picker
// with no ability to add items. If the case does arrive where the ttb should be unusable by design, developers should disable the control instead or setting the maximum to 0.
if (Content is ITokenStringContainer str && str.IsLast && ttbi?.Owner != null && (int)ttbi.Owner.GetValue(TokenizingTextBox.MaximumTokensProperty) > 0)
{
ttbi.Owner.TokenItemAdded += OnTokenCountChanged;
ttbi.Owner.TokenItemRemoved += OnTokenCountChanged;
OnTokenCountChanged(ttbi.Owner);
}
else
{
maxTokensCounter.Visibility = Visibility.Collapsed;
maxTokensCounter.Text = string.Empty;
}
}
#endregion
}
}
11 changes: 9 additions & 2 deletions ...Toolkit.Uwp.UI.Controls.Input/TokenizingTextBox/TokenizingTextBoxItem.AutoSuggestBox.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@
<ColumnDefinition Width="*" />
<ColumnDefinition Width="Auto" />
<ColumnDefinition Width="Auto" />
<ColumnDefinition Width="Auto" />
</Grid.ColumnDefinitions>
<Grid.RowDefinitions>
<RowDefinition Height="Auto" />
Expand Down Expand Up @@ -176,7 +177,7 @@
ZoomMode="Disabled" />
<ContentControl x:Name="PlaceholderTextContentPresenter"
Grid.Row="1"
Grid.ColumnSpan="3"
Grid.ColumnSpan="2"
Margin="{TemplateBinding BorderThickness}"
Padding="{TemplateBinding Padding}"
Content="{TemplateBinding PlaceholderText}"
Expand All @@ -194,9 +195,15 @@
IsTabStop="False"
Style="{StaticResource TokenizingTextBoxDeleteButtonStyle}"
Visibility="Collapsed" />

<TextBlock Name="PART_TokensCounter"
Grid.Row="1"
Grid.Column="2"
Margin="0,4,0,0" />

<Button x:Name="QueryButton"
Grid.Row="1"
Grid.Column="2"
Grid.Column="3"
Width="{TemplateBinding Height}"
MinWidth="30"
VerticalAlignment="Stretch"
Expand Down
1 change: 0 additions & 1 deletion Microsoft.Toolkit.Uwp.UI.Controls.Input/TokenizingTextBox/TokenizingTextBoxItem.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

using Windows.Foundation;
using Windows.System;
using Windows.UI.Core;
using Windows.UI.Xaml;
using Windows.UI.Xaml.Controls;
using Windows.UI.Xaml.Controls.Primitives;
Expand Down
18 changes: 13 additions & 5 deletions UnitTests/UnitTests.UWP/UI/Controls/Test_TokenizingTextBox_General.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ public void Test_Clear()

[TestCategory("Test_TokenizingTextBox_General")]
[UITestMethod]
public void Test_MaxTokens()
public void Test_MaximumTokens()
{
var maxTokens = 2;

Expand All @@ -76,7 +76,7 @@ public void Test_MaxTokens()
xmlns:x=""http://schemas.microsoft.com/winfx/2006/xaml""
xmlns:controls=""using:Microsoft.Toolkit.Uwp.UI.Controls"">

<controls:TokenizingTextBox x:Name=""tokenboxname"" MaxTokens=""{maxTokens}"">
<controls:TokenizingTextBox x:Name=""tokenboxname"" MaximumTokens=""{maxTokens}"">
</controls:TokenizingTextBox>

</Page>") as FrameworkElement;
Expand All @@ -87,23 +87,31 @@ public void Test_MaxTokens()

Assert.IsNotNull(tokenBox, "Could not find TokenizingTextBox in tree.");

// Items includes the text fields as well, so we can expect at least one item to exist initially, the input box.
// Use the starting count as an offset.
var startingItemsCount = tokenBox.Items.Count;

// Add two items.
tokenBox.AddTokenItem("TokenItem1");
tokenBox.AddTokenItem("TokenItem2");

// Make sure we have the appropriate amount of items and that they are in the appropriate order.
Assert.AreEqual(startingItemsCount + maxTokens, tokenBox.Items.Count, "Token Add failed");
Assert.AreEqual("TokenItem1", tokenBox.Items[0]);
Assert.AreEqual("TokenItem2", tokenBox.Items[1]);

// Attempt to add an additional item, beyond the maximum.
tokenBox.AddTokenItem("TokenItem3");

Assert.AreEqual(startingItemsCount + maxTokens, tokenBox.Items.Count, "Token Replace failed");
// Check that the number of items did not change, because the maximum number of items are already present.
Assert.AreEqual(startingItemsCount + maxTokens, tokenBox.Items.Count, "Token Add succeeded, where it should have failed.");
Assert.AreEqual("TokenItem1", tokenBox.Items[0]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weren't we expecting this to be TokenItem2 as TokenItem1 was the oldest?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TokenItem2 is the most recently added (right-most), so it gets replaced when TokenItem3 is added because MaxTokens is 2. TokenItem1 is the oldest (we added it first), so it is the left-most token and remains unaffected.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I thought we had discussed a FIFO approach vs. a LIFO. Want me to poll our community to get a litmus test? I can generalize it to try and be more objective like:

If you had a collection of elements [1, 2, 3] with a maximum size (3), but had to add a new item at the end ("4"). Which of the following would you expect?

  1. Oldest Item is Removed: List is now [2, 3, 4]
  2. Newest Item is Removed: List is now [1, 2, 4]
  3. Something else?

(Because I think we at least agreed we should do something over nothing, eh?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reached out to the UWP Community Discord and asked question here (with more options about inserting): https://discord.com/channels/372137812037730304/663434534087426129/875096129828708412

General consensus was adding a new item should be prevented and we should probably just indicate in the box the count of items in the limit:

image

(mock up in sample app overlaid a new TextBlock over the TokenizingTextBox (by wrapping in a Grid))

<TextBlock Text="2/3" HorizontalAlignment="Right" VerticalAlignment="Center" Margin="0,0,32,2"/>

(though not sure how that'd display if it goes multi-line... and we can probably arrange better in actual control template of course, but wanted to provide visual)

P2: Turn red when at max.
P2: Behavior over-ridable by dev (I'm wondering if the add event still fires and the dev could potentially remove a token of their choosing before the new one would check the max... that way they could implement any of the behaviors we've discussed with the insertion behavior...).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love it! I also think it is so cool that we can simply poll the community on the fly and get feedback! I'll work on adding the counter :) The P2s look interesting too. I can probably get the red-at-max feature handled, but the max-item-behavior is a bit more work. I'll create an issue so we won't forget.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh also, I am still planning on reducing the number of tokens from the end if the MaxTokens value is reduced below the current count. However, I don't expect changing the MaxTokens value on the fly to be a common scenario.

Assert.AreEqual("TokenItem3", tokenBox.Items[1]);
Assert.AreEqual("TokenItem2", tokenBox.Items[1]);

tokenBox.MaxTokens = 1;
// Reduce the maximum number of tokens.
tokenBox.MaximumTokens = 1;

// The last token should be removed to account for the reduced maximum.
Assert.AreEqual(startingItemsCount + 1, tokenBox.Items.Count);
Assert.AreEqual("TokenItem1", tokenBox.Items[0]);
}
shweaver-MSFT marked this conversation as resolved.
Show resolved Hide resolved
Expand Down