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

Rename IsRunning to IsAnimationEnabled on SKAnimatedSurfaceView #125

Merged
merged 6 commits into from
Jul 4, 2022

Conversation

bijington
Copy link
Contributor

Description of Change

This PR renames the IsRunning property to IsAnimationEnabled to hopefully provide a more meaningful name to the consumer.

Bugs Fixed

  • Related to issue #

API Changes

Changed:

  • bool SKAnimatedSurfaceView.IsRunning => bool SKAnimatedSurfaceView.IsAnimationEnabled

Behavioral Changes

Changed the default binding mode for the above property from BindingMode.TwoWay to BindingMode.OneWay. The old way caused an issue in testing plus the property is only intended to change whether the control is animating and therefore does not need to update the source.

PR Checklist

  • Has tests (if omitted, state reason in description) - I haven't work out how to test this part yet
  • Has samples (if omitted, state reason in description)
  • Rebased on top of main at time of PR
  • Changes adhere to coding standard
  • Updated documentation

bijington added 2 commits July 2, 2022 14:27
Also made the default binding mode BindingMode.OneWay as it only needs to be driven by the source and not update it.
@bijington bijington changed the title Feature/is animation enabled Rename IsRunning to IsAnimationEnabled on SKAnimatedSurfaceView Jul 2, 2022
mattleibow
mattleibow previously approved these changes Jul 2, 2022
Copy link
Contributor

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

Thanks for this and the great discussion!

@mattleibow
Copy link
Contributor

I see the build is failing for the forms sample app. One last holdout for IsRunning.

@bijington
Copy link
Contributor Author

I see the build is failing for the forms sample app. One last holdout for IsRunning.

Doh! I didn't check the Forms part! I'll try and sort that.

For unit tests I wanted to validate that the starting and stopping moved the animation progress but I couldn't get it to actually start. I'll try and get that done too

@bijington
Copy link
Contributor Author

@mattleibow hopefully this push will fix the build. I have failed getting a unit test in though. I wanted check that IsAnimationEnabled prevented any progress updates but by calling CallUpdate as the other tests do, that part doesn't rely on checks against IsAnimationEnabled I think to trigger it otherwise I would need a mock Dispatcher unless you have any ideas?

@mattleibow
Copy link
Contributor

I believe I broke it at some point. I'll try figure it out.

@mattleibow
Copy link
Contributor

I believe when I added events I added them in the wrong order: #126

@mattleibow mattleibow merged commit d6b3412 into mono:main Jul 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants