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

RotatorTile fix for flickering #1601

Merged
merged 3 commits into from
Nov 10, 2017
Merged

RotatorTile fix for flickering #1601

merged 3 commits into from
Nov 10, 2017

Conversation

nmetulev
Copy link
Contributor

@nmetulev nmetulev commented Nov 7, 2017

Issue: #1597 and #1411

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build or CI related changes
[ ] Documentation content changes
[ ] Sample app changes
[ ] Other... Please describe:

What is the current behavior?

See issues #1597 and #1411

PR Checklist

Please check if your PR fulfills the following requirements:

What is the new behavior?

The RotatorTile no longer flickers

Does this PR introduce a breaking change?

[ ] Yes
[x] No


// make sure DataContext on _currentElement has had a chance to update the binding
// avoids flicker on rotation
await System.Threading.Tasks.Task.Delay(50);
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to use the dispatcher here to queue up in the UI thread for processing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried that, it didn't have an effect at all :(

Copy link
Member

Choose a reason for hiding this comment

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

Normal or Low priority?

If we need to keep this delay, we should subtract it from the rotation time, eh?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, only tried normal, let me try low. On the subtraction, don't think it's needed as it's random anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, even low priority doesn't help :(

Copy link
Member

Choose a reason for hiding this comment

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

Bummer, that's frustrating. I know Idle usually causes bad things in UWP land.

For the timing though, it's only random is the ExtraRandomDuration is non-zero. Otherwise, it's fixed, so we would want to subtract the Delay time (as a const) from the base RotationDelay.

We should probably clarify in the doc comments for the property too that there's a minimum delay.

Copy link
Contributor

Choose a reason for hiding this comment

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

Go for it and we should be good!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Task.Delay doesn't really add any timing to the duration, it happens in parallel. When the rotation completes (after the duration) it just waits 50ms before switching the two items. The duration is not affect by this at all.

I did add a quick snippet about the duration in the documentation though.

@azchohfi azchohfi self-requested a review November 8, 2017 00:36
@michael-hawker michael-hawker merged commit 3c46559 into master Nov 10, 2017
@windowstoolkitbot
Copy link

This PR is linked to unclosed issues. Please check if one of these issues should be closed: #1411, #1597

@nmetulev nmetulev deleted the nmetulev/rotatortile branch November 10, 2017 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants