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

DataTrigger: Evaluate initial value on element load #64

Merged
merged 3 commits into from
Dec 12, 2019

Conversation

brianlagunas
Copy link
Collaborator

@brianlagunas brianlagunas commented Dec 7, 2019

Description of Change

Online 43 of the DataTrigger.cs file, added an event handler for the Loaded event of the AssociatedObject. Within this handler, we call EvaluateBindingChange to force the evaluation of the binding value when the element first loads. After the element is loaded and the bindings have been evaluated, we immediately unhook the Loaded event handler.

Note: tests were not added because I was unable to simulate a loaded event on a UI component. This was tested with a running sample.

Bugs Fixed

API Changes

None

Behavioral Changes

When an element first loads, the binding value is now evaluated by the DataTrigger.

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard

Copy link
Contributor

@DVaughan DVaughan left a comment

Choose a reason for hiding this comment

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

If AssociatedObject changes before the element is loaded, then the event unsubscription will be missed.
If EvaluateBindingChange throws an exception, the event should still be unsubscribed.
Here's one suggestion:

protected override void OnAttached()
{
    base.OnAttached();

    //fixes issue #11. We want to evaluate the binding's initial value when the element is first loaded
    if (AssociatedObject is FrameworkElement element)
    {
        void OnElementLoaded(object sender, RoutedEventArgs e)
        {
            try
            {
                this.EvaluateBindingChange(e);
            }
            finally
            {
                element.Loaded -= OnElementLoaded;
            }
        }

        element.Loaded += OnElementLoaded;
    }
}

@brianlagunas
Copy link
Collaborator Author

brianlagunas commented Dec 11, 2019

It is my understanding that the AssociatedObject can only be set when attaching. So if the AssociatedObject is to change before it is loaded, this implies that the previous AssociatedObject must also be detached first. Perhaps we should unsubscribe from the Loaded event in the OnDetaching override instead?

        protected override void OnAttached()
        {
            base.OnAttached();
            //fixes issue #11. We want to evaluate the binding's initial value when the element is first loaded
            if (AssociatedObject is FrameworkElement element)
            {
                element.Loaded += OnElementLoaded;
            }
        }

        protected override void OnDetaching()
        {
            base.OnDetaching();
            if (AssociatedObject is FrameworkElement element)
            {
                element.Loaded -= OnElementLoaded;
            }
        }

        private void OnElementLoaded(object sender, RoutedEventArgs e)
        {
            EvaluateBindingChange(e);
        }

Or we can assume the chances of the AssociatedObject changing before it's loaded is so rare, we don't need to worry about it, and go with what we already have.

@DVaughan
Copy link
Contributor

Unsubscribing from the event in OnDetaching is a good idea.
The element.Loaded event may fire multiple times. Calling EvaluateBindingChange is probably not what we want. I'd say add the OnDetaching method to the last commit.

@brianlagunas
Copy link
Collaborator Author

Okay, I modified the code to not only unsubscribe from the LOaded event in the event handler, but also in the OnDetaching as a fail-safe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants