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.

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
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,20 @@
<RowDefinition/>
</Grid.RowDefinitions>
<StackPanel>
<TextBlock FontSize="32" Text="Select Actions"
Margin="0,0,0,4"/>
<TextBlock FontSize="32" Margin="0,0,0,4">
<Run Text="Select up to" />
<Run Text="{Binding MaximumTokens, ElementName=TokenBox, Mode=OneWay}" />
<Run Text="actions" />
</TextBlock>
<controls:TokenizingTextBox
x:Name="TokenBox"
PlaceholderText="Add Actions"
QueryIcon="{ui:SymbolIconSource Symbol=Setting}"
MaxHeight="104"
HorizontalAlignment="Stretch"
TextMemberPath="Text"
TokenDelimiter=",">
TokenDelimiter=","
MaximumTokens="3">
<controls:TokenizingTextBox.SuggestedItemTemplate>
<DataTemplate>
<StackPanel Orientation="Horizontal">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,37 @@ private static void TextPropertyChanged(DependencyObject d, DependencyPropertyCh
typeof(TokenizingTextBox),
new PropertyMetadata(false));

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

private static void OnMaximumTokensChanged(DependencyObject d, DependencyPropertyChangedEventArgs e)
{
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)
{
int tokensToRemove = tokenCount - Math.Max(newMaxTokens, 0);

// Start at the end, remove any extra tokens.
for (var i = tokenCount; i > tokenCount - tokensToRemove; --i)
{
var token = ttb._innerItemsSource.ItemsSource[i - 1];

// Force remove the items. No warning and no option to cancel.
ttb._innerItemsSource.Remove(token);
ttb.TokenItemRemoved?.Invoke(ttb, token);
}
}
}
}

/// <summary>
/// Gets or sets the Style for the contained AutoSuggestBox template part.
/// </summary>
Expand Down Expand Up @@ -303,5 +334,14 @@ public string SelectedTokenText
return PrepareSelectionForClipboard();
}
}

/// <summary>
/// Gets or sets the maximum number of token results allowed at a time.
/// </summary>
public int MaximumTokens
{
get => (int)GetValue(MaximumTokensProperty);
set => SetValue(MaximumTokensProperty, value);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,16 @@ private void ItemsSource_PropertyChanged(DependencyObject sender, DependencyProp
if (ItemsSource != null && ItemsSource.GetType() != typeof(InterspersedObservableCollection))
{
_innerItemsSource = new InterspersedObservableCollection(ItemsSource);

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(MaximumTokens, 0); --i)
{
_innerItemsSource.Remove(_innerItemsSource[i]);
}
}

_currentTextEdit = _lastTextEdit = new PretokenStringContainer(true);
_innerItemsSource.Insert(_innerItemsSource.Count, _currentTextEdit);
ItemsSource = _innerItemsSource;
Expand Down Expand Up @@ -278,18 +288,16 @@ void WaitForLoad(object s, RoutedEventArgs eargs)
}
else
{
// TODO: It looks like we're setting selection and focus together on items? Not sure if that's what we want...
// If that's the case, don't think this code will ever be called?

//// TODO: Behavior question: if no items selected (just focus) does it just go to our last active textbox?
//// Community voted that typing in the end box made sense

// If no items are selected, send input to the last active string container.
// This code is only fires during an edgecase where an item is in the process of being deleted and the user inputs a character before the focus has been redirected to a string container.
if (_innerItemsSource[_innerItemsSource.Count - 1] is ITokenStringContainer textToken)
{
var last = ContainerFromIndex(Items.Count - 1) as TokenizingTextBoxItem; // Should be our last text box
var position = last._autoSuggestTextBox.SelectionStart;
textToken.Text = last._autoSuggestTextBox.Text.Substring(0, position) + args.Character +
last._autoSuggestTextBox.Text.Substring(position);
var text = last._autoSuggestTextBox.Text;
var selectionStart = last._autoSuggestTextBox.SelectionStart;
var position = selectionStart > text.Length ? text.Length : selectionStart;
textToken.Text = text.Substring(0, position) + args.Character +
text.Substring(position);

last._autoSuggestTextBox.SelectionStart = position + 1; // Set position to after our new character inserted

Expand Down Expand Up @@ -432,6 +440,12 @@ public async Task ClearAsync()

internal async Task AddTokenAsync(object data, bool? atEnd = null)
{
if (ReadLocalValue(MaximumTokensProperty) != DependencyProperty.UnsetValue && (MaximumTokens <= 0 || MaximumTokens <= _innerItemsSource.ItemsSource.Count))
{
// No tokens for you
return;
}

if (data is string str && TokenItemAdding != null)
{
var tiaea = new TokenItemAddingEventArgs(str);
Expand Down
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
}
}
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -63,5 +63,57 @@ public void Test_Clear()

Assert.AreEqual(tokenBox.Items.Count, 5, "Cancelled Clear Failed ");
}

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

var treeRoot = XamlReader.Load(
$@"<Page
xmlns=""http://schemas.microsoft.com/winfx/2006/xaml/presentation""
xmlns:x=""http://schemas.microsoft.com/winfx/2006/xaml""
xmlns:controls=""using:Microsoft.Toolkit.Uwp.UI.Controls"">

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

</Page>") as FrameworkElement;

Assert.IsNotNull(treeRoot, "Could not load XAML tree.");

var tokenBox = treeRoot.FindChild("tokenboxname") as TokenizingTextBox;

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");

// 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("TokenItem2", tokenBox.Items[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
}
}