[Feature] Custom StateTriggerBase to include an IsActive bool dependency property

[XAML-Knight]

Describe the problem this feature would solve

A few of our triggers use their own IsActive Dependency Property, and so it seems like refactoring the bool property (as useful as it is) into the parent base class (StateTriggerBase) would be in order.

Describe the solution

StateTriggerBase would be the proud new parent of a bool IsActive dependency property (and potentially also an OnActiveChanged event), which would then be inherited by any subsequent child classes.

Describe alternatives you've considered

Repeatedly adding the same boilerplate IsActive dependency property code to each trigger that needs it is not sustainable.

Additional context & Screenshots

Take a look at #1460

[ghost]

Hello, 'XAML-Knight! Thanks for submitting a new feature request. I've automatically added a vote 👍 reaction to help get things started. Other community members can vote to help us prioritize this feature in the future!

[Arlodotexe]

Will need to name it something else to avoid confusing with the platform StateTriggerBase

[Arlodotexe]

Linking microsoft/microsoft-ui-xaml#1460

[Arlodotexe]

Should be noted that StateTrigger already exposes an IsActive dependency property, but it's marked as sealed. I'm curious to know what the intended usage was here...

[dotMorten]

StateTriggerBase isn't part of the community toolkit. The linked WinUI issue above is the correct one to comment on / vote up.

I'm curious to know what the intended usage was here...

The intended usage is that that is how you trigger that specific trigger - by setting it to true. It might have been more clear if it was called WhenTrueStageTrigger ;-)

[Arlodotexe]

StateTriggerBase isn't part of the community toolkit. The linked WinUI issue above is the correct one to comment on.

We're creating our own base class for the toolkit that has this property to reduce the copy-pasted boilerplate code for our own triggers, as described at the top of the issue.

When WinUI figures their side out, we'll revisit our version and update as needed.

[Arlodotexe]

The intended usage is that that is how you trigger that specific trigger - by setting it to true. It might have been more clear if it was called WhenTrueStageTrigger ;-)

That would make it publicly settable which I'm not sure is a good idea

I'm not entirely sure I understand the problem with allowing it to be publicly settable.
There are details to iron out, but letting devs override the state externally seems fine to me at first glance. Something I'm not seeing?

[dotMorten]

I'm not entirely sure I understand the problem with allowing it to be publicly settable.

For StateTrigger it is already publicly settable. For StateTriggerBase, that doesn't make sense, as there are other properties on its subclasses that you are meant to set in order to make it active/inactive. Take the adaptive trigger for instance: It is controlled by the view width, so what does it mean to also set the IsActive property? Will that override the width-based trigger? Which takes precedence?

[Arlodotexe]

For StateTrigger it is already publicly settable. For StateTriggerBase, that doesn't make sense, as there are other properties on its subclasses that you are meant to set in order to make it active/inactive. Take the adaptive trigger for instance: It is controlled by the view width, so what does it mean to also set the IsActive property? Will that override the width-based trigger? Which takes precedence?

These are the "details to iron out" I mentioned 🙂

Setting IsActive would act as a temporary override for the current state. When set, it would just call SetActive() interally.

However, the main point of exposing IsActive is to allow for reading when the trigger is active, and to allow for subscribing to state changes on individual triggers. Handling the setter behavior is secondary.

[michael-hawker]

I think the original intent and problem microsoft/microsoft-ui-xaml#1460 is describing is not around trying to override the IsActive state (if you need to do that just bind whatever to the vanilla StateTrigger) - It's about being able to monitor and understand when the state is set so you can potentially bind something else to it.

So, maybe this is a read-only DP that our own triggers can maintain the state in-sync with when they set the base SetActive call so that it can be tracked externally (if required).

Adding over-ride logic to each and every trigger we make seems a bit complex. I think that's what @dotMorten is trying to get at here.

[Arlodotexe]

Adding over-ride logic to each and every trigger we make seems a bit complex.

I don't think we need this at all, setting IsActive just activates or deactivates the trigger temporarily.

But thinking through the possible scenarios the setter could be used for -- making it readonly sounds good.

[dotMorten]

making it readonly sounds good.

Which is what my proposed design suggests. Subclasses like the StateTrigger can still add a setter to a read-only property.

[dotMorten]

@XAML-Knight I'm curious about this justification:

Repeatedly adding the same boilerplate IsActive dependency property code to each trigger that needs it is not sustainable.

Why are the triggers doing this in the first place?

[Arlodotexe]

But thinking through the possible scenarios the setter could be used for -- making it readonly sounds good.

Should be noted that users can still set IsActive since dependency properties aren't truly readonly in UWP. Maybe still handle the changed callback and SetActive() for advanced scenarios that come up.

[Arlodotexe]

making it readonly sounds good.

Which is what my proposed design suggests. Subclasses like the StateTrigger can still add a setter to a read-only property.

I have to agree with @michael-hawker's comment in the WinUI issue, maybe StateTrigger and StateTriggerBase should be a single class.

[dotMorten]

Should be noted that users can still set IsActive since dependency properties aren't truly readonly in UWP

@Arlodotexe As already noted here: microsoft/microsoft-ui-xaml#1460 (comment)

Since the StateTriggerBase isn't in the CommunityToolkit but in UWP and WinUI3, should this issue be closed?

[dotMorten]

maybe StateTrigger and StateTriggerBase should be a single class.

Again as noted in the other issue, that would be a breaking change. Secondly there are different purposes to the two. They aren't the same thing, and shouldn't be merged, as they serve different purposes. Having IsActive settable on the base class would be a huge problem for all other state triggers, as they would no longer have control over when they are active or not.

[dotMorten]

@Arlodotexe You thumbed my comment down. Do you care to explain why? Is there something the community toolkit can do here about a class that is defined in a completely different product, or am I misunderstanding something else that you want the community toolkit to do?

[Arlodotexe]

@Arlodotexe As already noted here

We aren't trying to replace the inbox StateTriggerBase -- we're creating our own base class for the toolkit that has this property to reduce the copy-pasted boilerplate code for our own triggers, as described at the top of the issue.

The thumb down was about closing the issue based on your linked comment - much of our discussion here has been to address that specific comment.

[dotMorten]

@Arlodotexe Gotcha. I misunderstood the bit about introducing a second base class.
In that case it might be good to dive a bit deeper in to this question then.

[dotMorten]

Actually just looked at all the state triggers in this repo. Not a single one I can see has an IsActive property, which makes me wonder about this justification:

Repeatedly adding the same boilerplate IsActive dependency property code to each trigger that needs it is not sustainable.

[XAML-Knight]

@XAML-Knight I'm curious about this justification:

Repeatedly adding the same boilerplate IsActive dependency property code to each trigger that needs it is not sustainable.

Why are the triggers doing this in the first place?

In my case, with IsNullOrEmptyStateTrigger, I needed a way to initially set the trigger to true, for when a control loaded and wasn't applying the trigger logic like I expected it to. I exposed IsActive as a dependency property, so that it could easily be set from XAML.

Another trigger, ControlSizeTrigger, also provides an IsActive property, although it's just a .NET property (not a dependency property). After inspecting the history, this IsActive property was propped up to facilitate some unit testing.

[dotMorten]

In my case, with IsNullOrEmptyStateTrigger, I needed a way to initially set the trigger to true

Did you see my comment in your PR about just calling base.SetActive(true); in the constructor instead? You shouldn't have to have the developers have to set two properties.

[XAML-Knight]

Actually just looked at all the state triggers in this repo. Not a single one I can see has an IsActive property, which makes me wonder about this justification:

I'd suggest you take a second look at the ControlSizeTrigger - granted, it's IsActive property is just a .NET property, and not a dependency property (or is this the next thing we'll be arguing about??).

[michael-hawker]

@XAML-Knight the ControlSizeTrigger is a private set:

WindowsCommunityToolkit/Microsoft.Toolkit.Uwp.UI/Triggers/ControlSizeTrigger.cs

Line 139 in 213d3f2

public bool IsActive { get; private set; }

This new base would allow us to refactor this. But it's doing it for the same reason so the state can be inspected from the outside vs. manipulating its logic.

Also, we're not arguing, we're having a discussion. If you feel otherwise, please let me know.

I think the main point (which we probably want to update in the issue description) is that we want to have the following:

• a common class we can share between our triggers.
• it exposes an observable property called IsActive which is read-only so state of the trigger can be observed but not manipulated

I know normally we don't implement INotifyPropertyChange at the UI layer but maybe that's easier as dependency properties in UWP don't support read-only like WPF did (see microsoft/microsoft-ui-xaml#3139).