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

dismissWithDelay it's broken #647

Closed
cesare-montresor opened this issue Jul 23, 2016 · 8 comments
Closed

dismissWithDelay it's broken #647

cesare-montresor opened this issue Jul 23, 2016 · 8 comments
Milestone

Comments

@cesare-montresor
Copy link

I don't know if it works for you but it never doest for me, I always endup cooking an extension to achieve a custom length:

extension SVProgressHUD{
static func dismissAfter(seconds:NSTimeInterval){
let delayTime = dispatch_time(DISPATCH_TIME_NOW, Int64(seconds * Double(NSEC_PER_SEC)))
dispatch_after(delayTime, dispatch_get_main_queue()) {
SVProgressHUD.dismiss();
}
}
}

Rather simple but it just works (perhaps I misunderstand the purpose of dismissWithDelay)

My 2 cents ^_^

@karmaios
Copy link

me to ~

@honkmaster
Copy link
Member

Working on a fix. However, PR are always welcome.

@honkmaster honkmaster added this to the 2.1 milestone Jul 31, 2016
@honkmaster honkmaster added the bug label Jul 31, 2016
@honkmaster
Copy link
Member

honkmaster commented Jul 31, 2016

Should be fixed with the latest commit, please check the current master and provide feedback.

@honkmaster
Copy link
Member

No feedback at all? This is sad.

Questions and issues like this, are the reason people burn out on open source. Don't let others do your work, give good questions, follow issue guidelines, provide a demo project or tell what you have already done to solve the problem. Also give feedback if required! This is a matter of respect for the programmers that make a lot of efforts on writing open source components.

@cesare-montresor
Copy link
Author

cesare-montresor commented Aug 23, 2016

Dear Honkmaster,

apologies for the lack of feedback, I didn't integrated the latest version of SVProgressHud on the project I was working, I kept going with my patched version as I was close to the deadline.
I tested it today in the iOS Demo project, using master and I noticed the bug is still happening, I'll be glad to provide you with mode details about how to replicate it.

I modified slightly the ViewController.m, showWithStatus, in the following way:


- (void)showWithStatus {
    [SVProgressHUD showWithStatus:@"Doing Stuff"];
    [SVProgressHUD dismissWithDelay:1.0]; //Added line
}

What I noticed is that the HUD doesn't appear at all if used in this way, however, if the HUD is already visible (ex: trigger by "show" button) the it appears correctly and the delay it's respected.

I had a quick look to your code and I boiled down the issue to the method in SVProgressHub.m:1032
- (void)dismissWithDelay:(NSTimeInterval)delay completion:(SVProgressHUDDismissCompletion)completion;

Specifically in line 1095 where you have animateWithDuration the dealy it is not respected.
After thinkering a bit with the code of the animation I found out that by removing UIViewAnimationOptionBeginFromCurrentState from the animation options looks like it's working, however I guess if you put it there was needed somewhere else.

Between this and the fact that if you use breakpoints, everything works fine, everything suggest that there is a concurrency issue between the 2 operations (show and dismiss).

Now as I'm writing it came to my mind to try to decouple the delay from the animation as it appears it messup the timing is used in conjunction with UIViewAnimationOptionBeginFromCurrentState.
So I wrapped up the whole thing into a dispatch_after, here is the resulting code (from SVProgressHub.m:1092):


            dispatch_time_t dipatchTime = dispatch_time(DISPATCH_TIME_NOW, (int64_t)(delay * NSEC_PER_SEC));
            dispatch_after(dipatchTime, dispatch_get_main_queue(), ^{
                if (strongSelf.fadeOutAnimationDuration > 0) {
                    // Animate appearance

                    [UIView animateWithDuration:strongSelf.fadeOutAnimationDuration
                                          delay:0
                                        options:(UIViewAnimationOptions) (UIViewAnimationOptionAllowUserInteraction | UIViewAnimationCurveEaseIn | UIViewAnimationOptionBeginFromCurrentState )
                                     animations:^{
                                         animationsBlock();
                                     } completion:^(BOOL finished) {
                                         completionBlock();
                                     }];

                } else {
                    animationsBlock();
                    completionBlock();
                }
            });

Looks like it's working fine, however I have no idea if it has implications in other part of the project.

... I hope we can make peace ^_^

@honkmaster
Copy link
Member

Man, my rant had an effect :). Thank you very much for the feedback, this is useful. From my understanding UIViewAnimationOptionBeginFromCurrentState is / was required to not start all over if an dismiss is already in place. However, I am not sure if this is really required, as I sometimes modified it with no real effect. But yes, this is the "bad line of code". I am currently at an other project with no access to the repo, will evaluate in the afternoon.

cesare-montresor added a commit to cesare-montresor/SVProgressHUD that referenced this issue Aug 23, 2016
@honkmaster
Copy link
Member

Funny thing btw (from my memories): I also tried something like

[SVProgressHUD showWithStatus:@"Doing Stuff"];
[SVProgressHUD dismissWithDelay:1.0];

while fixing some related bugs and it was working for me. Ok, more feedback later.

@cesare-montresor
Copy link
Author

Ho yes it did, I reminded me when I'm overworked of it feels :P
I packed everything in a pull request, easier to read and perhaps to integrate, let me know how it goes ^_^

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

No branches or pull requests

3 participants