search

mdinacci / MDRadialProgress

A custom UIView useful to represent progress in discrete steps.
Other
947 stars 159 forks source link

Delete unnecessary condition and fix a bug if progressCounter = 0 #13

Closed marcaudefroy closed 10 years ago

marcaudefroy commented 10 years ago

Hello Marco,

Congratulations, your control has evolved. :)

I corrected an unnecessary condition that causes a bug if the progressCounter property is equal to 0. First, if progressCounter is equal to 0, the condition "if (self.progressCounter > 0)" is equal to false which causes the radial to not be drawn. I delete the condition(property is an unsigned integer so self.progressCounter >= 0 is always true). I tested with your example project and this does not seem to bring regression.

A remark : Between line 205 and line 215. (in "self.progressCounter < self progressTotal" condition), it's not clear. Is you launch an analyze, you will see that endAngle is never read. In addition, your startAngle variable is equal to originAngle variable in method CGContextAddArc so I think that the arc will not be drawn. Maybe I didn't understand this part of code.

Marc

mdinacci commented 10 years ago

Hello Marc,

I added that check to fix issue #12 . It sounded right to me not drawing the slices if there is no progress. What kind of bug is causing to you ?

Regarding the remark, you're right, endAngle is never read. I'll clean it up as soon as I can.

marcaudefroy commented 10 years ago

In my case, I want to draw the slices even if there is no progress (I use your your component to a statistical diagram). But objectively, I think that the state of component with progressCounter equal to 0 must be considered because, in the case of "an progress bar", it's the initial state.

Perhaps you must add a property to given the choice?

mdinacci commented 10 years ago

Hi Marc,

I didn't think about your use case, you have a point. A property would be a quick and effective solution but I think it's possible to remove the check and fix #12 in a way that won't make neither the check nor the property necessary. I'll have to think about it. For the moment unfortunately I can't accept your patch because it will re-open #12, but I'll leave this issue open until both problems are solved.

Thanks.

mdinacci commented 10 years ago

Hi Marc,

I pushed an update that should fix your issue and #12. I'm in the process of making a new release, let me know if that work for you. If it doesn't please reopen this issue.

marcaudefroy commented 10 years ago

It works! :) Thanks!