Failure when there's no target

Sep 15, 2010 at 4:45 PM

I have a UserControl where the DataContext/Action.Target is bound to a selection from a list. This selection is the target of some Actions within the UserControl. But the selection and therefore the user controls target might be null and then Micro throws an exception because it can't find the action method. Caliburn only checked the method when it was going to be invoked, which IMHO makes much more sense.

And there's a related issue: Micro does not update the context, even if Action.Target changes.

Coordinator
Sep 15, 2010 at 5:00 PM

Do you have a recommendation for how to update the context when the Action.Target changes?

Sep 15, 2010 at 5:39 PM

One way would be to call PrepareContext() each time Invoke() is called. I'm not sure, if this has a noticeable performance impact, but I guess it hasn't.

Another way to keep the context up-to-date could be to subscribe to HandlerProperty changes (This would at least help when Action.Target* is explicitly used).

I didn't had much time yet to "dive" into the code, but I'll take a closer look tomorrow - there are also some other issues that bugged me in Caliburn that I hope are solved (or can be solved) in Micro.

Coordinator
Sep 15, 2010 at 5:42 PM

I'm pondering creating an internal binding using a DP on ActionMessage to Target. If I can do that, then any time the Target changes I can call PrepareContext. We have to call PrepareContext as soon as the target is available because that affects guard conditions.

Sep 16, 2010 at 1:41 PM
Edited Sep 16, 2010 at 1:42 PM

Ok, here are some of my changes:

http://github.com/e-tobi/Caliburn-Micro-Extensions/commits/e-tobi%2FActionMessage-Improvments

Feel free to pull.

Especially take a look at this commit:

http://github.com/e-tobi/Caliburn-Micro-Extensions/commit/7a57fa85ef80230dcddb7f7ea9c1d541ae32b8c0

It binds to the HandleProperty and updates the context each time the Target(WithoutContext) changes.
...took a while to figure out, how to make this work on SL and WPF with the same code :-(

The second commit just makes the exception messages a little bit more useful and the third one makes Micro defer the
checking of context.Target/Method to invocation time, so I can happily have null's assigned to Action.Target until I actually try to run the action

 

Coordinator
Sep 16, 2010 at 2:27 PM

Thanks! I'll dig into this shortly.

Coordinator
Sep 16, 2010 at 2:53 PM

Looking at the code...it seems that this will only work if the handler changes directly on the element to which the ActionMessage is attached. Is this your assumption? What if the Action.Target is defined higher up the tree and it changes there, affecting everything below it?

Sep 16, 2010 at 2:55 PM

Now I finally even managed to push to the mercurial fork (Setting up a Git mirror on GitHub just took me 10 minutes - getting mercurial working about an hour :-(

https://hg01.codeplex.com/forks/etobi/caliburnmicroetobi

Sep 16, 2010 at 5:05 PM
Edited Sep 16, 2010 at 5:11 PM
EisenbergEffect wrote:

Looking at the code...it seems that this will only work if the handler changes directly on the element to which the ActionMessage is attached. Is this your assumption? What if the Action.Target is defined higher up the tree and it changes there, affecting everything below it?

Yes, that was my assumption and was just what I needed and the simplest thing I could do.

I'm not sure it would be a good idea to just bind to all elements in the tree (Wouldn't work anyways if multiple elements in the tree use Action.Target). And it's not trivial to figure out, which elements use Action.Target, if Action.Target can be bound to null.

The following modification to my patch basically works. It searches the tree upwards and binds to the first element that has set the HandlerProperty (which is marked by HasHandlerProperty).

https://hg01.codeplex.com/forks/etobi/caliburnmicroetobi/rev/4d90c5ee0e27

Coordinator
Sep 16, 2010 at 5:19 PM

Ok. That's pretty similar to what I was thinking of doing. I'm not sure that we can do much better than that. I'm doubtful that the node on which the target is declared would ever change in normal use, so I think this could work. Is it working well enough for your scenario?

Coordinator
Sep 16, 2010 at 5:22 PM

Also, do we really need the HasHandlerProperty? or could we just check to see whether HandlerProperty returns null? BTW Thanks for working on this and thinking through it with me!

Sep 16, 2010 at 7:09 PM
EisenbergEffect wrote:

I'm doubtful that the node on which the target is declared would ever change in normal use, so I think this could work. Is it working well enough for your scenario?

Yes, it works fine, I just haven't verified yet, that this doesn't cause any memory leaks.

Also, do we really need the HasHandlerProperty? or could we just check to see whether HandlerProperty returns null?

Yes it is needed because Action.Target might be bound to a property that just returns null. See my example from the beginning. A ListBox and you e.g. have a Button with Action.Target={Binding listBox.SelectedItem}. SelectedItem might be null and Action.Target changes when selecting items from the ListBox.

That's also the reason for those two commits:

https://hg01.codeplex.com/forks/etobi/caliburnmicroetobi/rev/6319b90976c0
https://hg01.codeplex.com/forks/etobi/caliburnmicroetobi/rev/ec97fac7e16a

Coordinator
Sep 16, 2010 at 7:11 PM

Ah. Good thinking. Let me know what you find regarding memory. If it looks like it's going to be a safe change, I'll go ahead and put that in.

Sep 17, 2010 at 12:22 PM

Ok, as far as I can tell there seems to be no memory leak issue with my modifications.

And I also found a way to get rid of the HasDefaultHandler property (see Action.HasTargetSet()).  Besides this, if Target/TargetWithoutContext is
explicitly set and it's null, it will not continue trying to find a matching method higher in the hierarchy.

https://hg01.codeplex.com/forks/etobi/caliburnmicroetobi

 

Coordinator
Sep 17, 2010 at 2:12 PM

Fantastic! This is a wonderful contribution. Thanks very much for you hard work! I'll get it in there as soon as I have the time :)

Coordinator
Sep 17, 2010 at 4:14 PM

Ok. I got these changes pushed up :) Thanks again!

Coordinator
Sep 17, 2010 at 4:50 PM

Uh oh. It looks like databinding to attached props may cause Silverlight to crash...Investigating.

Coordinator
Sep 17, 2010 at 6:51 PM

I came up with a workaround for Silverlight. But, it doesn't work in WP7. So, it's implemented for 2 out of 3 platforms. That's good enough. There's really nothing that can be done about WP7. The SL workaround was pretty outrageous to begin with...

Sep 17, 2010 at 8:32 PM

Can't test this until Monday and just looking at the code and your changes, I have no idea, why it causes trouble with SL. But I'm glad you could find a workaround! Thanks!

Coordinator
Sep 17, 2010 at 9:15 PM

I did a lot of research on this and determined that it is a bug in Silverlight. I've created an issue on Connect and cc'd some people who can do something about this....in a future version ;(