zhanghai / MaterialProgressBar

Material Design ProgressBar with consistent appearance
Apache License 2.0
2.21k stars 279 forks source link

Implement determinate circular progressbar #51

Closed webmaster128 closed 7 years ago

webmaster128 commented 7 years ago

This is an implementation of the determinate circular progress bar as shown here: https://storage.googleapis.com/material-design/publish/material_v_10/assets/0B14F_FSUCc01N2kzc1hlaFR5WlU/061101_Circular_Sheet_xhdpi_005.webm

Closes #24

Screenshots of this implementation: 1 2 3 4 5 6 7 8

zhanghai commented 7 years ago

Thanks, I'll look into the code later.

Another question remains that whether the circular progress should move its starting point along the circumference. Although the spec presented so, it might be confusing to user (If the progress stalls it looks kind of weird, might seem as if an indeterminate progress just get stuck, and inconsistent compared to the horizontal counterparts) and hard to estimate the progress, plus that the spec has no text specification on this except for that video.

Just did a quick research on this:

Library Status
Angular Materal (by Google) Fixed starting point at top
Angular Material 2 (by Google) Fixed starting point at top
Polymer Elements (by Google) N/A
Materialize N/A
Material Components for the web (by Google) Unknown
Material Design Lite (by Google) N/A
Material UI Fixed starting point at 45 deg
webmaster128 commented 7 years ago

I personally like the effect of the moving starting point, which is an innovative way to visually fill the square element and provide visual symmetry even for early progress values.

I don't think a circular progress indicator alone can give the user a precise estimate about the progress value. If that is needed, other UI elements must be used anyway. So I think it is good enough if the user can get a rough idea how far the progress is.

Additionally you can see that the length of the coloured arch is linear to the progress, starting from 0°at 0% to 360° at 100%, even if the arch is moving. So if you observed the element a couple of times, you get a good feeling of how far the progress is.

Given that, I'd love to keep the effect as it is shown in the video provided by Google.

zhanghai commented 7 years ago

I think a circular progress indicator alone can give the user an easy impression of the progress, e.g. if the progress starts at the top, then users can read a quarter, half, three quarters, it's pretty natural, the same as we read clocks.

However if the starting point is moving around, the same experience can not apply any more. Users will need extra efforts to read it, or get accustomed to a new way of interaction that is not seen elsewhere.

So although I'm somewhat a spec follower and lover, I have my reservation about this particular design, which is also never stated literally but only showcased in a video.

zhanghai commented 7 years ago

Other unresolved issues include:

I prefer making a decision upon these design/implementation issues before we merge it into this library.

webmaster128 commented 7 years ago

Whether we should provide the starting point as an option.

Sure, that can be done easily

Whether it should be able to show/hide a track.

You mean a background of the circle? Might be possible but I did not see it in a Material example. So I'd rather skip it until there is a good reason to implement it.

Whether it should be able to show/hide a secondary progress.

I almost finished implementing that but then it was a bit buggy so I discarded it because I did not see it in the Materials documentation and I personally do not need it. But yes, that can be done for consistency.

If we are moving to VectorDrawableCompat (#53) how should the implementation be done.

I have no experience with VectorDrawableCompat and I looked how the current implementations work. Moving to VectorDrawableCompat is going to be an entire rewrite of the core library so I'd leave this as a second step. Having a stable set of features is a good base for evaluating if VectorDrawableCompat can replace Canvas throughout the library.

webmaster128 commented 7 years ago

Is there anything I can do for the progress of this ticket?

zhanghai commented 7 years ago

Still think that the progress should start at top (at least by default), per the two Angular Material versions by Google (see above), and (my own) UX considerations.

If this can be an attribute, I'm wondering what its name should be, app:mpb_???circularIndeterminate???? Really can't think of a good name.

And about the secondary progress -- I think it should better be implemented, but it would be weird if the starting point is moving according to progress -- where should it start if it has a different progress value compared to the primary one?

And I think a track should also be implemented, like the horizontal counterpart.

webmaster128 commented 7 years ago

Great, thanks. I'll take care of that.

What about app:mpb_circularDeterminateStyle = "fixedStartTop" (default), "movingStart"? Is it snake_case or camleCase in the values?

webmaster128 commented 7 years ago

Okay, done implementing the requred features

screenshot_1491577400

zhanghai commented 7 years ago

Thanks for your work! I'll review the code later, due to some personal matters.

webmaster128 commented 7 years ago

Okay cool, thanks :)

webmaster128 commented 7 years ago

So you think it is possible to review and release this within this week? I need the determinate circular progress for an upcoming app release. Otherwise I need to somehow build a fork locally but if that could be avoided would be great.

zhanghai commented 7 years ago

OK, I'll do the code review ASAP.

BTW, I rebased the fork (Needed a commit upstream), so you might need some extra steps if you want to add commits.

And my comments on the diff is only for dicussion -- you don't need to fix it. I'm already fixing them and will push later.

webmaster128 commented 7 years ago

Alright, thanks. Interesting, I did not know that you get the access rights to push to my fork's branch. Otherwise I am familiar with rebasing et cetera, no problem.

zhanghai commented 7 years ago

Finished reviewing the library module except the 15 degree problem.

webmaster128 commented 7 years ago

What is the "15 degree problem"?

zhanghai commented 7 years ago

@webmaster128 https://github.com/DreaminginCodeZH/MaterialProgressBar/pull/51/files/7b6d3d41ae131315032990f49e0975fef23bd0b9#r110824773 It's the only problem left with code now.

webmaster128 commented 7 years ago

@DreaminginCodeZH The link does not point me to any specific line or comment

zhanghai commented 7 years ago

Strange, perhaps you need to wait till the page load is finished. Anyway you can C-F the string:

How did you get this 15? By estimating from the video, or by stepping to find the first frame and measuring in Photoshop etc? Is it precise, or can it be from the fact the at the beginning the progress is thin but quick in motion? Have you done comparison between 0 and 15, and if so is it that 15 wins?

webmaster128 commented 7 years ago

I measured the angle from the first frame that I could capture in the video. I am pretty sure it does not start at 0. Thus I took 15 and was happy with the result. 0 was not tested.

webmaster128 commented 7 years ago

Strange, perhaps you need to wait till the page load is finished. Anyway you can C-F the string:

It is not there. Did you write it as a pending review comment?

zhanghai commented 7 years ago

I found a suspicous frame on the second start of the progress:

vlcsnap-2017-04-11-17h34m51s532-s

I have experimented with 0 instead of 15 and they looked the same, and because I see no sensible reason for the animation to start at 15 (so this is very likely a glitch due to high speed and small area), I'm making it 0 now.

zhanghai commented 7 years ago

Merged, thanks! Will be released later.

zhanghai commented 7 years ago

Released on Maven Central just now, should be available within hours.

webmaster128 commented 7 years ago

Cool, thanks!