-
Notifications
You must be signed in to change notification settings - Fork 125
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
Pause/Resume gif on image visibility changed #58
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @khersonIT,
Thanks for this PR! It looks pretty good, and I'll be glad to merge it.
I only have minor comments about formatting. Could you please rollback the formatting changes?
WpfAnimatedGif/ImageBehavior.cs
Outdated
typeof (RoutedEventHandler), | ||
typeof (ImageBehavior)); | ||
typeof(RoutedEventHandler), | ||
typeof(ImageBehavior)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor issue: please try not to change formatting of unrelated code.
This is a general rule for open-source contributions, not just for this project. It adds noise to the diff, and the project maintainer might not have the same formatting rules as you.
I find the git add -a
command (interactive add) useful for staging only specific lines in a file. Many GUI tools support this too (SourceTree, GitExtensions, GitKraken...)
controller.Play(); | ||
} | ||
else | ||
controller.Pause(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor issue: while I'm not against omitting braces in general, I prefer to keep the if
and else
blocks consistent. Could you please add braces there?
I use a loading gif on each row of a DataGrid and I experience permanent high CPU usage, so I need this fix too. Hope will be merge and release soon. Thank you. |
@khersonIT I made the requested changes myself so that I could merge. Thanks for the PR! @OmiCron07 I'm merging it now, and will release ASAP |
@OmiCron07 I just released 1.4.18 with this change. It should appear on NuGet soon. |
Chnages related to issue #37.
I encountered the same problem in my project when used your lib. Since my app was used on the slower computers and several gifs was in UI in same time (but only one was visible at once), this fix was critical for me.
It's my first pr from the fork, so let me know if I have to change something.