-
Notifications
You must be signed in to change notification settings - Fork 131
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
Improved old window handling, depreciation warning suppression.. #3
Conversation
Were you seeing some issues with some of this code, or did you just feel like changing it? A lot of these changes don't relate to one another. Just looking for your thoughts before I dive into this. |
The code that selects I added the I am also using a custom radial progress view (which is not included in my commit) so to use that with MMProgressHUD it came in handy to use a protocol to allow different types of radial progress views.
And lastly I suppressed compiler warning about depreciation which were quite annoying ;) I know they don't really relate to another but I use this in one of my projects so I went through the code to fix and tweak some things :) |
What kind of compiler flags are you using that |
Let me know if the |
My bad, I was testing the code In some project I had downloaded from the internet :P Turns out the C Language Dialect was set to C99 and setting it back to "Compiler Default" fixed everything! Sorry! |
I'm running the beta and am having some mixed results with |
I'm using Xcode 5 DP 5 and I just tried using |
Hmm, I'm building a binary with the Xcode cli right now and here is get the error with |
Yeah looks like this specific project's C-language dialect was explicitly set to Never run into that issue before with |
@@ -59,6 +59,11 @@ typedef NS_ENUM(NSInteger, MMProgressHUDWindowOverlayMode){ | |||
//}; | |||
|
|||
@interface MMProgressHUD : UIView | |||
|
|||
|
|||
/** A Boolean value that indicates whether or not the HUD has been cancelled manually. */ |
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.
What is the purpose of this new boolean? If you want to know when the action was "cancelled", then you would use the cancelBlock
block. In addition, it appears that self.cancelled
is never explicitly reset to NO
anywhere. Unless you can describe a situation in which you think this is useful, I'd rather not add a new property to the public header that duplicates functionality.
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.
Oops yeah I never reset it! My bad! I was using an individual instance rather than the shared Instance so I didn't need to reset the value to NO
as I release the instance after dismissing it.
The cancelBlock
is very different tho! The cancelBlock
is simply called when the user has cancelled the HUD. The HUD will stay visible after that until its dismissed and therefore I needed a way to check of the HUD has been cancelled to know whether I should update it or not. In most cases your code wouldn't even attempt to update the HUD after the hud and the operation has been cancelled but I have a case where my code still tries to update the HUD even though the HUD and the operation has been cancelled (It may sound a bit stupid but its necessary in my case ;) )
OK I added the new commit! |
@@ -59,6 +59,8 @@ @interface MMProgressHUD () <MMHudDelegate> | |||
|
|||
@implementation MMProgressHUD | |||
|
|||
@synthesize cancelled; |
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.
Let's avoid explicit synthesizers since they aren't necessary anymore.
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.
Ha, I didn't even know that Clang 4.0 has auto synthesis! Cheers for telling me! :P
OK, its pretty solid now. No more bugs and everything works like it should! The progress view can now be fully customized! Let me know what you think |
Can you add me as a collaborator on your branch? With these new changes, I'd like to modify some things to not use assets. |
ok, done |
Actually, ignore that - those assets were mine in the sample project. The diff just marked them that you changed them for some reason. |
I couldn't push back to your repository for some reason. I've made some changes in my branch that you can merge in to yours to add to this PR: https://github.com/mutualmobile/MMProgressHUD/tree/JonasGessner-master |
Oh, sorry I had removed you as collaborator from my fork :P |
I'm impressed with this. I'll give it another pass tonight or tomorrow and we'll see about getting this in. I specifically want to pout over the public header diff and make sure that we don't completely break any public api changes for backwards compatibility. |
Some of the public APIs have changed, specifically all the ones that contained MMPrrogressHUDProgressStyle |
I just pushed a commit to add back backwards compatibility. Take a look and let me know what you think. |
OK its supposed to say "Small improvements" in the commit description :P |
I think it looks good. |
Yeah it seems to be ready now :) Edit: I'm about to push another small commit, hold on |
Is it ok for you that I removed the method? If you want I can also revert it before you merge the pull request |
Yeah removing that method made sense. I think I'll merge this on Monday when I have a computer to give it a full glance over one last time. |
• Added +showWithTitle: and +dismissAfterDelay: • Centralized defines • Separated public from private defines • Moved from float to NSTimeInterval (aka double) for delays
OK I added a few more methods and everything seems to work :) |
What was the reason for these last-minute changes? I would prefer that we make as few changes as possible, unless there is a clear benefit to the new structure you have created. |
I did these changes because I just replaced MBProgressHUD with MMProgressHUD in one of my projects. I immediately noticed that I needed these changes! |
I'd rather not. 8c41613 is basically a revert of ea4830f with some additional fixes. And b8e4401 is more than just useful. MMProgressHUD was using floats for time intervals which is wrong. Time intervals are doubles (NSTimeInterval) and passing floats instead of doubles is never a good idea. That commit moved most defines to a private define file so that by importing MMProgressHUD.h you don't also import loads of unimportant and confusing variables. The |
b8e4401: I'm ok with the "defines" file, but please do not have both a "private" defines and a "public" defines file. If something is meant for public consumption, then it should be in the normal header location. Everything else should be in the "private" header you want and imported on the Why is a public API for a dismiss delay "a must"? The reason it was an internal method was to allow some of the animations to complete and the final state to complete before dismissing the hud. From an implementation stand-point, if you would like to delay the dismiss, then you would simply I'm good with adding 8c41613: I would really like to accept this pull request even though it's gotten a little out of hand and beyond scope of the original pull request. In the future, please restrict pull requests to their original scope, and open up new pull requests for different features. Also note that just because something is working for you right now doesn't mean it will work for everyone else that is depending on this library in their projects. |
Its always a good practice to keep defines separated from your actual classes. Apple does it the same way. However you don't want to have all your defines and variables publicly, thats why I separated the public defines from the private ones. And of course you can use a 8c41613: |
And yeah tis pull request just became larger and larger and maybe it shouldn't have grown that much. Part of the problem is also that GitHub automatically adds new commits that I push to my repo, I can't stop that from happening! But I think the changes I made are useful and not in any way redundant. |
This code would not display the image when displaying the hud for the second time: ```objc [MMProgressHUD setPresentationStyle:MMProgressHUDPresentationStyleFade]; [MMProgressHUD showWithTitle:@"AA" status:@"" image:[UIImage imageNamed:@"1.png"]]; [MMProgressHUD dismissAfterDelay:2.0]; double delayInSeconds = 3.0; dispatch_time_t popTime = dispatch_time(DISPATCH_TIME_NOW, (int64_t)(delayInSeconds * NSEC_PER_SEC)); dispatch_after(popTime, dispatch_get_main_queue(), ^(void){ [MMProgressHUD showWithTitle:@"AA" status:@"" image:[UIImage imageNamed:@"1.png"]]; [MMProgressHUD dismissAfterDelay:1.5]; }); ``` Forcing an update takes care of the bug
Improved old window handling, added depreciation warning suppression and cancelled state detection.