Serialized show, update and dismiss actions. #22

mobilejohnw · 2015-01-29T19:38:57Z

Kevin,

I have made changes to serialize the show, update, and dismiss actions by using a dispatch_queue and a semaphore. This does not change your api, but does introduce slight semantic change, because now there is a minimum duration for any show or update action. I could add logic to make updates check how long the dialog has been showing, but wanted to keep the flow as simple as possible to start.

Let me know what you think,

John

kevin-hirsch · 2015-01-29T19:46:19Z

Thanks!
I will check it out now and tell you what I think.

kevin-hirsch · 2015-01-29T22:19:36Z

It seems okay, but the updateStatus: method is not functioning anymore after a show because of the blocking semaphore:

   [KVNProgress showWithStatus:@"show"];
   [KVNProgress updateStatus:@"test"]; // never occurs

Also, if this flow appears:

   [KVNProgress showWithStatus:@"show"];
   [KVNProgress dismiss];
   [KVNProgress showWithStatus:@"test"];
   [KVNProgress dismiss];

What the user should see is the "show", then the "test" on the HUD and then, and only then, the dismiss should occur. It is not the case here.

It's a good start, but I don't want to rush before pushing this to master and Cocoapods.
Maybe you can use you own branch as KVNProgress in your app while we make a robust solution from this one?

mobilejohnw · 2015-01-30T12:58:16Z

Kevin,

I expected that we would have to use our own changes until you were
satisfied with them. I'll fix the update flow now and consider how best to
handle your second case. I didn't see unit tests in the project, do you
have a test suite I can validate changes against?

John

On Thu, Jan 29, 2015 at 4:19 PM, Kevin Hirsch notifications@github.com
wrote:

It seems okay, but the updateStatus: method is not functioning anymore
after a show because of the blocking semaphore:

[KVNProgress showWithStatus:@"show"];
[KVNProgress updateStatus:@"test"]; // never occurs

Also, if this flow appears:

[KVNProgress showWithStatus:@"show"];
[KVNProgress dismiss];
[KVNProgress showWithStatus:@"test"];
[KVNProgress dismiss];

What the user should see is the "show", then the "test" on the HUD and
then, and only then, the dismiss should occur. It is not the case here.

It's a good start, but I don't want to rush before pushing this to master
and Cocoapods.
Maybe you can use you own branch as KVNProgress in your app while we make
a robust solution from this one?

—
Reply to this email directly or view it on GitHub
#22 (comment)
.

kevin-hirsch · 2015-01-30T14:05:44Z

Thank you for fixing the first issue!
Unfortunately, I don't have any unit tests in the project, I should.
Usually I test all cases available in the demo app and some more like:

show, show, dismiss
show, dismiss, show, dismiss
show, dismiss, dismiss
show, update, dismiss

mobilejohnw · 2015-01-30T14:09:24Z

Kevin,

I pushed up a fix for the update defect. It wasn't the semaphore, but
rather that the updates were not running through showProgress and thus not
through the dispatch_queue. I queued them and added a style check in
showProgress to skip the wait time when updating within a specific style.

For the other case you bring up I'm not actually sure that swallowing the
first dismiss is the correct (as opposed to current) behavior. Assuming it
is, however, I believe I can do it by setting an "expiration" time and
having dismiss check it and repost itself if it isn't time to go yet.

John

On Thu, Jan 29, 2015 at 4:19 PM, Kevin Hirsch notifications@github.com
wrote:

It seems okay, but the updateStatus: method is not functioning anymore
after a show because of the blocking semaphore:

[KVNProgress showWithStatus:@"show"];
[KVNProgress updateStatus:@"test"]; // never occurs

Also, if this flow appears:

[KVNProgress showWithStatus:@"show"];
[KVNProgress dismiss];
[KVNProgress showWithStatus:@"test"];
[KVNProgress dismiss];

What the user should see is the "show", then the "test" on the HUD and
then, and only then, the dismiss should occur. It is not the case here.

It's a good start, but I don't want to rush before pushing this to master
and Cocoapods.
Maybe you can use you own branch as KVNProgress in your app while we make
a robust solution from this one?

—
Reply to this email directly or view it on GitHub
#22 (comment)
.

kevin-hirsch · 2015-01-30T15:24:40Z

I queued them and added a style check in
showProgress to skip the wait time when updating within a specific style.

Why did you add this?

For the case: show, dismiss, show, dismiss, after thinking about it, you might be correct because if the two shows are on different views, the flow has to dismiss the first show before showing the second one.

I have to say I find the code a little bit complicated to read now, thus to maintain. I'm not a fan of doing things such:

   [NSThread sleepForTimeInterval:delay];

   dispatch_async([self sharedView].queue, ^{
      dispatch_sync(dispatch_get_main_queue(), ^{
      // update progress

Would it not be better to have something like operations that are just waiting for executing (being showed) with a "smart" queue that will handle the order of executing those operations, taking care of the minimum display times and, of course, of the synchronization of all operations to avoid issues like a never dismissing HUD: #15.
It also could simplify the code that is not easy to read (as it is now released or in your pull request) by dividing it in 2 easily readable and understandable parts:

handle the show/update/dismiss execution/animation
handle the timing between those operations

What do you think about this idea?

mobilejohnw · 2015-01-30T16:06:11Z

Kevin,

Yes, that sounds good. That means rewriting to use an NSOperationQueue and
that means it won't happen today as I have too much else to get done, but
I'll do it, because now I'm invested ;-) I may get it done on the weekend.

John

On Fri, Jan 30, 2015 at 9:24 AM, Kevin Hirsch notifications@github.com
wrote:

I queued them and added a style check in
showProgress to skip the wait time when updating within a specific style.

Why did you add this?

For the case: show, dismiss, show, dismiss, after thinking about it, you
might be correct because if the two shows are on different views, the flow
has to dismiss the first show before showing the second one.

I have to say I find the code a little bit complicated to read now, thus
to maintain. I'm not a fan of doing things such:

[NSThread sleepForTimeInterval:delay];

dispatch_async([self sharedView].queue, ^{
dispatch_sync(dispatch_get_main_queue(), ^{
// update progress

Would it not be better to have something like operations that are just
waiting for executing (being showed) with a "smart" queue that will
handle the order of executing those operations, taking care of the minimum
display times and, of course, of the synchronization of all operations to
avoid issues like a never dismissing HUD: #15
#15.
It also could simplify the code that is not easy to read (as it is now
released or in your pull request) by dividing it in 2 easily readable and
understandable parts:

handle the show/update/dismiss execution/animation

handle the timing between those operations

What do you think about this idea?

—
Reply to this email directly or view it on GitHub
#22 (comment)
.

kevin-hirsch · 2015-01-30T16:35:32Z

Yes, that means write a similar queue like NSOperationQueue and that it will not be ready today.
I was not asking you to do it. Actually, I was planning on implementing it next week (because I'm away from my computer this weekend) ;)

I was thinking of maybe a custom queue object that will mimic the NSOperationQueue. This queue will launch the next show/dismiss/update only after the previous one has been started, animated and showed its minimum amount of time. And every operation (show/dismiss/update) would be able to notify the queue that its state changed from started (setting up) to executing (showing) so the queue can launch the next planned operation after the minimum display time of executing operation.
An operation would have properties like:

type: enum (show, success, error, dismiss) => will be used by the queue to know which minimum display time to use
state: enum (waiting, started, executing, finished) => that will be changed by the queue
operationBlock: the block containing the code for showing, updating or dismissing
The queue will have like a FIFO internal queue that would execute and pop every operation at the correct time.

Does it seems a good implementation to you?

mobilejohnw · 2015-01-31T20:41:01Z

Kevin,

Sorry I didn't get back to you earlier, yesterday was a busy day. Your
design seems reasonable to me, although I wonder if you couldn't get a
similar effect with a standard NSOperationQueue and just a custom
NSOperation class with some of the properties you list. The operation
already has state management, and you could use queue priority to "push"
dismiss actions back when adding an operation to the queue.

I'll think about this some more, and possibly try a new implementation if I
have time this weekend.

John

On Fri, Jan 30, 2015 at 10:35 AM, Kevin Hirsch notifications@github.com
wrote:

Yes, that means write a similar queue like NSOperationQueue and that it
will not be ready today.
I was not asking you to do it. Actually, I was planning on implementing it
next week (because I'm away from my computer this weekend) ;)

I was thinking of maybe a custom queue object that will mimic the
NSOperationQueue. This queue will launch the next show/dismiss/update
only after the previous one has been started, animated and showed its
minimum amount of time. And every operation (show/dismiss/update) would be
able to notify the queue that its state changed from started (setting
up) to executing (showing) so the queue can launch the next planned
operation after the minimum display time of executing operation.
An operation would have properties like:

type: enum (show, success, error, dismiss) => will be used by the
queue to know which minimum display time to use

state: enum (waiting, started, executing, finished) => that will be
changed by the queue

operationBlock: the block containing the code for showing, updating
or dismissing The queue will have like a FIFO internal queue that would
execute and pop every operation at the correct time.

Does it seems a good implementation to you?

—
Reply to this email directly or view it on GitHub
#22 (comment)
.

Serialized show, update and dismiss actions.

b21fc09

Modifed updates to also be queued to serialize them.

58a9e69

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serialized show, update and dismiss actions. #22

Serialized show, update and dismiss actions. #22

mobilejohnw commented Jan 29, 2015

kevin-hirsch commented Jan 29, 2015

kevin-hirsch commented Jan 29, 2015

mobilejohnw commented Jan 30, 2015

kevin-hirsch commented Jan 30, 2015

mobilejohnw commented Jan 30, 2015

kevin-hirsch commented Jan 30, 2015

mobilejohnw commented Jan 30, 2015

kevin-hirsch commented Jan 30, 2015

mobilejohnw commented Jan 31, 2015

Serialized show, update and dismiss actions. #22

Are you sure you want to change the base?

Serialized show, update and dismiss actions. #22

Conversation

mobilejohnw commented Jan 29, 2015

kevin-hirsch commented Jan 29, 2015

kevin-hirsch commented Jan 29, 2015

mobilejohnw commented Jan 30, 2015

kevin-hirsch commented Jan 30, 2015

mobilejohnw commented Jan 30, 2015

kevin-hirsch commented Jan 30, 2015

mobilejohnw commented Jan 30, 2015

kevin-hirsch commented Jan 30, 2015

mobilejohnw commented Jan 31, 2015