Fix IsNullOrEmptyStateTrigger on SelectedItem #4426
Conversation
XAML-Knight
requested review from
Sergio0694,
michael-hawker and
shweaver-MSFT
|
Thanks XAML-Knight for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌 |
ghost
requested a review
from 
ghost
added
the
|
@dotMorten did you want to help review this PR? Thanks! |
There was a problem hiding this comment.
Since the default value the Value property in this trigger is null, I'm betting when you start with a binding that evaluates to null, it never actually calls SetActive(true); Instead I think the constructor of the trigger just needs to call SetActive(true), so its initial state matches the Value property
| /// <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); | ||
| } | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Since the default value is null, it means it is active at creation, so you should call SetActive(true) in the constructor.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
This PR has been marked as "needs attention 👋" and awaiting a response from the team. |
Fixes #4411
PR Type
What kind of change does this PR introduce?
Bugfix
What is the current behavior?
The
IsNullOrEmptyStateTriggerdid not fire correctly when placed on the SelectedItem of a typical items control (i.e., ListBox, ListView, etc). The default value of it'sValuedependency property (which is of typeobject) was set totrue, notnull. The logic behind the trigger is inspecting for null or empty, and the bool valuetrueis neither of these.What is the new behavior?
The default value of the trigger's
Valuedependency property is now set tonull. A boolean dependency property (IsActive) was added, in order to set the initial state of the trigger directly in the XAML.A new section was added to the
IsNullOrEmptyStateTriggerSample Page, highlighting the behavior of the trigger when placed on aSelectedItemproperty.PR Checklist
Please check if your PR fulfills the following requirements:
Other information