Windowscommunitytoolkit: Fix DispatcherHelper open issues

Created on 13 Nov 2016  路  13Comments  路  Source: windows-toolkit/WindowsCommunityToolkit

There are still two things that should be fixed in DispatcherHelper:

  1. The current fix for #609 will silently swallow errors. #609 has been closed already, so I am opening a new issue. We should use "fail fast" to make the dev aware of errorneous code as soon as possible, instead of silently returning default(T) which might be noticed very late. The exact same error can be provoked in Task.Run. Task.Run will return a cancelled task, resulting in TaskCanceledException. We should either do the same, or throw a different, more meaningful exception. There is no way to give a specific error message when cancelling a task, so the only way to give a more meaningful message to the dev would be to use a different exception instead of cancelling the task.

  2. 609 is not completely solved, the same problem still exists here:

`
public static Task ExecuteOnUIThreadAsync(CoreApplicationView viewToExecuteOn, Func function, CoreDispatcherPriority priority = CoreDispatcherPriority.Normal)
{
return ExecuteOnUIThreadAsync(
viewToExecuteOn,
async () =>
{
await function().ConfigureAwait(false);
return null;
}, priority);
}

`

If function() returns null, we get a NullReferenceException. We should check the result and use the same error handling here as in AwaitableRunAsync().

picture lukasf

All 13 comments

I can make a PR if we agree on how to handle first point.

lukasf picture lukasf on 13 Nov 2016

I see It should be clear for the developer which function he is using fist then we can go for this throwing exception approach because it we just throw an error it will be confusing for developers.
We need to agree on #610 to solve both in a single PR but sure that's my opinion.

IbraheemOsama picture IbraheemOsama on 13 Nov 2016

Well, as you know, I disagree that #610 needs to be fixed :). I just want to make sure that these two things get addressed, no matter what happens with #610. I can make a PR for these two issues, but I won't make one for #610 because I don't like it :)

lukasf picture lukasf on 13 Nov 2016
😄1

@lukasf Maybe you go with a PR then decide what to do with #610

IbraheemOsama picture IbraheemOsama on 13 Nov 2016

Lol :)

IbraheemOsama picture IbraheemOsama on 13 Nov 2016

Okay PR is there. I opted for custom exception for issue one, because it will give a more meaningful message to the developer.

lukasf picture lukasf on 13 Nov 2016
👍1

Okay, look like you beat me to get the PR first, but both of them has been solved in my commit on my fork.

https://github.com/Code-ScottLe/UWPCommunityToolkit/commit/33802cc0742fe7415267001421674e5d754fbb9a

I also reduce quite bit of redundant code as well and it is looking fine from my end. What do you want to do? I can either submit a new PR or try to migrate my changes into your PR too.

PS: mine commit contain quite a bit more code and also adding in extension method for the CoreApplicationView like we previously discussed.

Code-ScottLe picture Code-ScottLe on 14 Nov 2016
👍1

@ScottIsAFool I like the way how you cleaned up the code, thanks! One question: There were already extension methods for CoreApplicationView, called ExecuteOnUIThreadAsync(). You removed them and added new ones called ExecuteAsync(). Was this rename on purpose? If you ask me, I'd rather not introduce one more different name for the same thing, we have so many names here already, and now even one more? I would personally prefer to leave it as ExecuteOnUIThreadAsync (because that's still what it does, only that you now specify on which view to execute).

Is it okay for you if I take over your improvements into my PR? Originally I wanted to propose that you create a PR with your changes and we close my PR. But then I found even two more optimizations which I would like to introduce, on top of yours (further reduce the number of awaits and limit the number of lambdas). I could update my PR with all those improvements now, if that is fine for you.

lukasf picture lukasf on 14 Nov 2016

@lukasf did you mean @Code-ScottLe? :)

ScottIsAFool picture ScottIsAFool on 14 Nov 2016

@ScottIsAFool Oops, right, sorry! :) Ping @Code-ScottLe

lukasf picture lukasf on 14 Nov 2016

@Code-ScottLe Since time is running out a bit on 1.2, I just went ahead and updated my PR with your improvements plus a bunch of additional improvements I found. I hope that is ok with you. I also added a comment there on naming, please check the updated PR and let's continue the discussion over there.

lukasf picture lukasf on 14 Nov 2016

Sorry, just woke up. Time zone works quite a bit differently haha. But yeah, no problem, probably save @deltakosh a PR to clean up. We always have my branch there if something went wrong and we need a point to revert over.

Code-ScottLe picture Code-ScottLe on 14 Nov 2016

I appreciate guys! I'll have a look asap on PR and see if we can merge it for 1.2

deltakosh picture deltakosh on 14 Nov 2016
Was this page helpful?
0 / 5 - 0 ratings

Related issues

shweaver-MSFT picture shweaver-MSFT  路  3Comments
alvinashcraft picture alvinashcraft  路  3Comments
tibitoth picture tibitoth  路  3Comments
karmaecrivain94 picture karmaecrivain94  路  3Comments
Kyaa-dost picture Kyaa-dost  路  3Comments