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 12 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 MaxTokens, 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=","
MaxTokens="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="MaxTokens"/> property.
/// </summary>
public static readonly DependencyProperty MaxTokensProperty = DependencyProperty.Register(
nameof(MaxTokens),
typeof(int),
typeof(TokenizingTextBox),
new PropertyMetadata(null, OnMaxTokensChanged));

private static void OnMaxTokensChanged(DependencyObject d, DependencyPropertyChangedEventArgs e)
{
if (d is TokenizingTextBox ttb && ttb.ReadLocalValue(MaxTokensProperty) != 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 MaxTokens
{
get => (int)GetValue(MaxTokensProperty);
set => SetValue(MaxTokensProperty, 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(MaxTokensProperty) != DependencyProperty.UnsetValue && _innerItemsSource.ItemsSource.Count > MaxTokens)
{
// Reduce down to the max as necessary.
for (var i = _innerItemsSource.ItemsSource.Count - 1; i >= Math.Max(MaxTokens, 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(MaxTokensProperty) != DependencyProperty.UnsetValue && MaxTokens <= 0)
{
// No tokens for you
return;
}

if (data is string str && TokenItemAdding != null)
{
var tiaea = new TokenItemAddingEventArgs(str);
Expand All @@ -451,6 +465,26 @@ 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 @@ -459,6 +493,26 @@ 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
Original file line number Diff line number Diff line change
Expand Up @@ -63,5 +63,48 @@ public void Test_Clear()

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

[TestCategory("Test_TokenizingTextBox_General")]
[UITestMethod]
public void Test_MaxTokens()
{
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"" MaxTokens=""{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.");

var startingItemsCount = tokenBox.Items.Count;

tokenBox.AddTokenItem("TokenItem1");
tokenBox.AddTokenItem("TokenItem2");

Assert.AreEqual(startingItemsCount + maxTokens, tokenBox.Items.Count, "Token Add failed");
Assert.AreEqual("TokenItem1", tokenBox.Items[0]);
Assert.AreEqual("TokenItem2", tokenBox.Items[1]);

tokenBox.AddTokenItem("TokenItem3");

Assert.AreEqual(startingItemsCount + maxTokens, tokenBox.Items.Count, "Token Replace 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]);

tokenBox.MaxTokens = 1;

Assert.AreEqual(1, tokenBox.Items.Count);
}
}
}