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

Fix IsNullOrEmptyStateTrigger on SelectedItem #4426

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Fix IsNullOrEmptyStateTrigger on SelectedItem #4426

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7b6794b
Init commit
XAML-Knight Dec 7, 2021
fac2b1b
Change default value from true to null
XAML-Knight Dec 10, 2021
0a7677c
Bring in another control for trigger
XAML-Knight Dec 13, 2021
31f1cc4
unit test not ready for show
XAML-Knight Dec 13, 2021
dae06ea
Light up IsActive DP
XAML-Knight Dec 14, 2021
50a0402
Finishing touches
XAML-Knight Dec 14, 2021
ccb6366
call SetActive in ctor
XAML-Knight Dec 21, 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
29 changes: 27 additions & 2 deletions Microsoft.Toolkit.Uwp.SampleApp/SamplePages/Triggers/IsNullOrEmptyStateTrigger.bind
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,18 @@
</VisualState.Setters>
</VisualState>
</VisualStateGroup>
<VisualStateGroup x:Name="ListViewStates">
<VisualState x:Name="SelectedItemNotNullState" />
<VisualState x:Name="SelectedItemNullState">
<VisualState.StateTriggers>
<triggers:IsNullOrEmptyStateTrigger Value="{Binding SelectedItem, ElementName=SelectList}" IsActive="True"/>
</VisualState.StateTriggers>
<VisualState.Setters>
<Setter Target="RemoveSelection.IsEnabled" Value="False" />
<Setter Target="SelectionEmptyMessage.Visibility" Value="Visible" />
</VisualState.Setters>
</VisualState>
</VisualStateGroup>
</VisualStateManager.VisualStateGroups>

<StackPanel HorizontalAlignment="Center" VerticalAlignment="Center">
Expand All @@ -43,7 +55,7 @@
<TextBlock x:Name="OurTextBoxError" Text="* Required" Foreground="Red" Margin="10 0 0 0" Visibility="Collapsed" VerticalAlignment="Center" />
</StackPanel>

<StackPanel Orientation="Horizontal">
<StackPanel Orientation="Horizontal" Margin="0,40,0,0">
<Button x:Name="AddButton" Content="Add" Margin="0 0 10 0"/>
<Button x:Name="RemoveButton" Content="Remove"/>
</StackPanel>
Expand All @@ -63,7 +75,20 @@
</Style>
</ListBox.ItemContainerStyle>
</ListBox>
<Border BorderBrush="Gold" BorderThickness="1" Margin="0,80,0,0" Width="275">
<StackPanel>
<Button x:Name="RemoveSelection" Content="Deselect" />
<TextBlock x:Name="SelectionEmptyMessage" Text="SelectedItem is empty, select an item" Visibility="Collapsed"
Margin="5,0,0,0"/>
<ListView x:Name="SelectList" HorizontalAlignment="Left">
<ListView.Items>
<TextBlock Text="One" />
<TextBlock Text="Two" />
<TextBlock Text="Three" />
</ListView.Items>
</ListView>
</StackPanel>
</Border>
</StackPanel>

</Grid>
</Page>
21 changes: 20 additions & 1 deletion Microsoft.Toolkit.Uwp.SampleApp/SamplePages/Triggers/IsNullOrEmptyStateTriggerPage.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ public sealed partial class IsNullOrEmptyStateTriggerPage : Page, IXamlRenderLis
private Button _addButton;
private Button _removeButton;
private ListBox _listBox;
private Button _unselectButton;
private ListView _selectListView;

/// <summary>
/// Initializes a new instance of the <see cref="IsNullOrEmptyStateTriggerPage"/> class.
Expand Down Expand Up @@ -52,6 +54,15 @@ public void OnXamlRendered(FrameworkElement control)
}

_listBox = control.FindDescendant("OurList") as ListBox;

_selectListView = control.FindDescendant("SelectList") as ListView;

if (control.FindDescendant("RemoveSelection") is Button btn3)
{
_unselectButton = btn3;

_unselectButton.Click += this.UnselectButton_Click;
}
}

private void AddButton_Click(object sender, RoutedEventArgs e)
Expand All @@ -64,10 +75,18 @@ private void AddButton_Click(object sender, RoutedEventArgs e)

private void RemoveButton_Click(object sender, RoutedEventArgs e)
{
if (_listBox != null)
if (_listBox != null && _listBox.Items.Count > 0)
{
_listBox.Items.RemoveAt(0);
}
}

private void UnselectButton_Click(object sender, RoutedEventArgs e)
{
if(_selectListView != null && _selectListView.SelectedItem != null)
{
_selectListView.SelectedItem = null;
}
}
}
}
32 changes: 29 additions & 3 deletions Microsoft.Toolkit.Uwp.UI/Triggers/IsNullOrEmptyStateTrigger.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Collections;
using System.Collections.Specialized;
using Microsoft.Toolkit.Uwp.Helpers;
Expand All @@ -28,22 +29,47 @@ public object Value
/// Identifies the <see cref="Value"/> DependencyProperty
/// </summary>
public static readonly DependencyProperty ValueProperty =
DependencyProperty.Register(nameof(Value), typeof(object), typeof(IsNullOrEmptyStateTrigger), new PropertyMetadata(true, OnValuePropertyChanged));
DependencyProperty.Register(nameof(Value), typeof(object), typeof(IsNullOrEmptyStateTrigger), new PropertyMetadata(null, OnValuePropertyChanged));
dotMorten marked this conversation as resolved.
Show resolved Hide resolved

/// <summary>
/// Gets or sets a value indicating whether a trigger is active.
/// </summary>
public bool IsActive
{
get => (bool)GetValue(IsActiveProperty);
set => SetValue(IsActiveProperty, value);
}

/// <summary>
/// Identifies the <see cref="IsActive"/> DependencyProperty
/// Allows user to enable the trigger from XAML
/// </summary>
public static readonly DependencyProperty IsActiveProperty =
DependencyProperty.Register(nameof(IsActive), typeof(bool), typeof(IsNullOrEmptyStateTrigger), new PropertyMetadata(false, OnIsActiveChanged));

private static void OnIsActiveChanged(DependencyObject d, DependencyPropertyChangedEventArgs e)
{
if (e.NewValue != null && e.NewValue is bool)
{
var obj = (IsNullOrEmptyStateTrigger)d;
obj.SetActive((bool)e.NewValue);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

State triggers doesn't use public IsActive properties, but just the base SetActive method.. It's not clear what purpose this one serves? It seems like you can activate this trigger by either setting it to true or the Value to not-null? Which one takes precedence?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When putting in the fix for this issue (#4411), the fix worked except for the initial load of the control/sample page. It's as if the trigger wasn't being evaluated at load time. Hence, the need to at least initially set the trigger to be active. Another hack would have been to just assign Sting.Empty to the SelectedItem in the constructor of the page's code-behind, but I didn't like that approach, and implemented the bool DP instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since the default value is null, it means it is active at creation, so you should call SetActive(true) in the constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the default value is null, it means it is active at creation

This logic holds, until it doesn't (trigger is not turning On, as expected), which is a condition that was discovered as part of this PR (see the issue with SelectedItem, as mentioned above).

However, by calling SetActive(true) in the constructor of the trigger, as suggested, you'd then be giving this trigger an artificial value ("On") by default. What if a developer does not want the trigger to be on by default? (The logic in the trigger may just wind up overwriting this any way, or it may not.) You'd then be requiring the developer to find a way to turn the trigger off (or, at least, there would be ambiguity as far as if they would have to shut it off, or not).

Contrast that whole approach, by just simply giving our developers an easy way to set an initial state for the trigger, by exposing a boolean property. I've gone a little further, and exposed this boolean as a dependency property, which allows for easy setting of the property in the XAML.

Another approach was the hack (again, mentioned above), of just setting SelectedItem to String.Empty in the Sample Page's code-behind- same result (trigger is now turning On initially), but clearly a hack.

Copy link
Contributor

@dotMorten dotMorten Dec 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

However, by calling SetActive(true) in the constructor of the trigger, as suggested, you'd then be giving this trigger an artificial value ("On") by default.

How is that artificial? The Value property is null, which means the trigger should be on, until the Value property changes to something not-null.

What if a developer does not want the trigger to be on by default?

Then don't set Value to null or don't set a trigger on it? I don't understand the use-case here.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe the problem is that we don't have an analogous IsNotNullOrEmptyStateTrigger like we do with Equal/NotEqual?

In the case this trigger is being used, it should be bound to and either that binding is invalid (which is effectively null) or evaluates to some value. The default state of the trigger being on shouldn't be a problem here between the time the trigger is instantiated, the value property is resolved via binding, and the UI is actually displayed.


private static void OnValuePropertyChanged(DependencyObject d, DependencyPropertyChangedEventArgs e)
{
var obj = (IsNullOrEmptyStateTrigger)d;
var val = e.NewValue;

obj.SetActive(IsNullOrEmpty(val));
obj.SetActive(obj.IsActive = IsNullOrEmpty(val));

if (val == null)
{
return;
}

// Try to listen for various notification events
// Starting with INorifyCollectionChanged
// Starting with INotifyCollectionChanged
var valNotifyCollection = val as INotifyCollectionChanged;
if (valNotifyCollection != null)
{
Expand Down