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

Add IsOnMainThread to IMvxMainThreadDispatcher #2791

Closed
sdebruyn opened this issue Apr 12, 2018 · 8 comments
Closed

Add IsOnMainThread to IMvxMainThreadDispatcher #2791

sdebruyn opened this issue Apr 12, 2018 · 8 comments
Assignees
Labels
t/feature Feature request type
Milestone

Comments

@sdebruyn
Copy link
Contributor

The IMvxMainThreadDispatcher now only allows you to run an action on the UI thread, regardless of the current thread.

However, sometimes you'd want to run different code depending on the thread you're on (e.g. when you're working with a synchronized, thread-safe observable collection).

So a property on the IMvxMainThreadDispatcher indicating if the current thread is the main thread would be very helpful.

@martijn00
Copy link
Contributor

Sounds like a good addition! Can you make a PR for this?

@martijn00 martijn00 changed the title Feature request: add IsOnMainThread to IMvxMainThreadDispatcher Add IsOnMainThread to IMvxMainThreadDispatcher Apr 12, 2018
@martijn00 martijn00 added t/feature Feature request type up-for-grabs Issues up for grabs by anyone labels Apr 12, 2018
@david-laundav
Copy link
Contributor

I will take a look at this issue.

@david-laundav
Copy link
Contributor

So, I have been thinking a bit, and considered to expose a property called IsOnMainThread in the IMvxMainThreadAsyncDispatcher. Basically, this would do as follows:

public bool IsOnMainThread => Thread.CurrentThread.ManagedThreadId == 1;

I have although read that the main thread may not always be 1.

So, I considered persisting the mainThreadId when the instance is created, but found that the class was
abstract.

Any ideas?

@martijn00
Copy link
Contributor

That class doesn't need to be abstract i think. You could just remove that and add the code.

@david-laundav
Copy link
Contributor

david-laundav commented Apr 20, 2018

I am nearly done with my effort, @Cheesebaron and @martijn00, but wanted to know if you guys think it is the right direction to go:
https://github.com/david-laundav/MvvmCross/commits/feature/2791-Add-IsOnMainThread

As a note, it does build ;)

@nickrandolph
Copy link
Contributor

@david-laundav I don't think that we should be looking at the threadid. Currently each platform has their own implementation of checking to see if the current execution context is on the UI thread. Can you make the base property abstract and get each platform to implement it so that it's consistent?
Going forward if you can work off the v6.1 branch and submit a PR that'd help us with reviewing the changes. Thanks again for the submission

@nickrandolph nickrandolph added this to the 6.1.0 milestone Apr 24, 2018
@nickrandolph nickrandolph removed the up-for-grabs Issues up for grabs by anyone label Apr 24, 2018
@david-laundav
Copy link
Contributor

What is your proposal, @nickrandolph? I have spoken to @Cheesebaron about the approach, and both of us concurred that using the ThreadId would be the most efficient approach.

Please refer to the following:
https://forums.xamarin.com/discussion/comment/200008/#Comment_200008

@nickrandolph
Copy link
Contributor

Resolved by #2921

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t/feature Feature request type
Development

No branches or pull requests

4 participants