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

iOS and Android timers should be runnable from any thread and execute… #374

Merged
merged 6 commits into from Jan 11, 2017

Conversation

Projects
None yet
7 participants
@adrianknight89
Contributor

adrianknight89 commented Sep 23, 2016

Description of Change

This PR ensures that device timer can be started from any thread and executed on the UI thread. For a detailed discussion, see https://bugzilla.xamarin.com/show_bug.cgi?id=22706 and http://forums.xamarin.com/discussion/comment/74535

Bugs Fixed

@dnfclas

This comment has been minimized.

Show comment
Hide comment
@dnfclas

dnfclas Sep 23, 2016

Hi @adrianknight89, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

dnfclas commented Sep 23, 2016

Hi @adrianknight89, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

@TheRealAdamKemp

This comment has been minimized.

Show comment
Hide comment
@TheRealAdamKemp

TheRealAdamKemp Sep 23, 2016

The Windows code is here:

public void StartTimer(TimeSpan interval, Func<bool> callback)

It looks correct to me. There is a WindowsTimer internal class right after StartTimer, and that internal class appears to be unused. Seems like a good candidate for deletion, but maybe I'm not searching in the right place.

There are similar _Timer classes in Android and iOS, and I'm not sure whether those are used or not. They're built on the .Net Timer class, which bothers me.

TheRealAdamKemp commented Sep 23, 2016

The Windows code is here:

public void StartTimer(TimeSpan interval, Func<bool> callback)

It looks correct to me. There is a WindowsTimer internal class right after StartTimer, and that internal class appears to be unused. Seems like a good candidate for deletion, but maybe I'm not searching in the right place.

There are similar _Timer classes in Android and iOS, and I'm not sure whether those are used or not. They're built on the .Net Timer class, which bothers me.

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Sep 23, 2016

Contributor

I wasn't able to find any references either, so I removed those Timer classes in addition to adding minor refactoring. XF team can comment on the changes.

Contributor

adrianknight89 commented Sep 23, 2016

I wasn't able to find any references either, so I removed those Timer classes in addition to adding minor refactoring. XF team can comment on the changes.

@samhouts

This comment has been minimized.

Show comment
Hide comment
@samhouts

samhouts Sep 26, 2016

Member

@adrianknight89 Please rebase on master. Thanks!

Member

samhouts commented Sep 26, 2016

@adrianknight89 Please rebase on master. Thanks!

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Sep 26, 2016

Contributor

@samhouts Bear with me. First time I had to do rebase since I'm new to git. Hope it worked?

Contributor

adrianknight89 commented Sep 26, 2016

@samhouts Bear with me. First time I had to do rebase since I'm new to git. Hope it worked?

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Sep 27, 2016

Member

You need to add the .bak to gitignore

Member

rmarinho commented Sep 27, 2016

You need to add the .bak to gitignore

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Sep 27, 2016

Contributor

See now.

Contributor

adrianknight89 commented Sep 27, 2016

See now.

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Dec 6, 2016

Member

@adrianknight89 can you rebase so i can run on our CI

Member

rmarinho commented Dec 6, 2016

@adrianknight89 can you rebase so i can run on our CI

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89
Contributor

adrianknight89 commented Dec 6, 2016

@rmarinho Done.

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Dec 20, 2016

Member

needs rebase to pass tests

Member

rmarinho commented Dec 20, 2016

needs rebase to pass tests

@hartez

This comment has been minimized.

Show comment
Hide comment
@hartez

hartez Dec 30, 2016

Member

@adrianknight89 Can you rebase this again? The UI test failures are from a test that was broken for a while; rebasing should picked up the fixed test and we can run it through again.

Member

hartez commented Dec 30, 2016

@adrianknight89 Can you rebase this again? The UI test failures are from a test that was broken for a while; rebasing should picked up the fixed test and we can run it through again.

@hartez hartez self-assigned this Dec 30, 2016

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Dec 30, 2016

Contributor

Done.

Contributor

adrianknight89 commented Dec 30, 2016

Done.

@hartez

This comment has been minimized.

Show comment
Hide comment
@hartez

hartez Jan 3, 2017

Member

@adrianknight89
On first review, this looks good (and it's passing all the existing UI tests), but it definitely needs a UI test of its own. I'd suggest just adapting the repro project that Adam Kemp attached to 28953.

Member

hartez commented Jan 3, 2017

@adrianknight89
On first review, this looks good (and it's passing all the existing UI tests), but it definitely needs a UI test of its own. I'd suggest just adapting the repro project that Adam Kemp attached to 28953.

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Jan 4, 2017

Member

Hey @adrianknight89 can you check if it's possible to add a UITest like @hartez referred ? thanks.

Member

rmarinho commented Jan 4, 2017

Hey @adrianknight89 can you check if it's possible to add a UITest like @hartez referred ? thanks.

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Jan 5, 2017

Contributor

DId you mean an automated test? I added a manual one.

Contributor

adrianknight89 commented Jan 5, 2017

DId you mean an automated test? I added a manual one.

@jassmith jassmith merged commit f78b328 into xamarin:master Jan 11, 2017

5 of 6 checks passed

iOS10-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS10 : Tests failed: 1 …
Details
Android-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run Android 6.0.1 : Tests passed: 351, i…
Details
OSX-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: OSX Debug : Running
Details
Windows-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: Windows Debug : Tests passed: 3689, ignored: 10
Details
iOS8-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified IOS8 : Tests passed: 344…
Details
iOS9-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS9 : Tests passed: 346…
Details
@TheRealAdamKemp

This comment has been minimized.

Show comment
Hide comment

TheRealAdamKemp commented Jan 11, 2017

🎉

@samhouts samhouts added this to the 2.3.4 milestone Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment