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

Implement determinate circular progressbar #51

Merged
merged 32 commits into from Apr 11, 2017

Conversation

webmaster128
Copy link
Contributor

@webmaster128 webmaster128 commented Mar 27, 2017

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
Copy link
Owner

zhanghai commented Mar 30, 2017

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
Copy link
Contributor Author

webmaster128 commented Mar 30, 2017

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
Copy link
Owner

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
Copy link
Owner

Other unresolved issues include:

  • Whether we should provide the starting point as an option.
  • Whether it should be able to show/hide a track.
  • Whether it should be able to show/hide a secondary progress.
  • If we are moving to VectorDrawableCompat (Switch to AnimatedVectorDrawableCompat? #53) how should the implementation be done.

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

@webmaster128
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@zhanghai
Copy link
Owner

zhanghai commented Apr 6, 2017

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
Copy link
Contributor Author

webmaster128 commented Apr 7, 2017

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
Copy link
Contributor Author

Okay, done implementing the requred features

screenshot_1491577400

@zhanghai
Copy link
Owner

zhanghai commented Apr 8, 2017

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

@webmaster128
Copy link
Contributor Author

Okay cool, thanks :)

@webmaster128
Copy link
Contributor Author

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.

@webmaster128
Copy link
Contributor Author

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.

dcmTintedSecondaryBackground
);

new Thread() {
Copy link
Owner

@zhanghai zhanghai Apr 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't kick off threads for animation in Android; In most cases one should use ObjectAnimator, and it is much more smoother. I'll refactor this later.

@zhanghai
Copy link
Owner

zhanghai commented Apr 11, 2017

Finished reviewing the library module except the 15 degree problem.

@webmaster128
Copy link
Contributor Author

What is the "15 degree problem"?

@zhanghai
Copy link
Owner

zhanghai commented Apr 11, 2017

@webmaster128
Copy link
Contributor Author

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

@zhanghai
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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?

mEndAngleMax = 360;
break;
case MaterialProgressBar.DETERMINATE_CIRCULAR_STYLE_MOVINSTART:
// leading and trailing angles start at 15° or 5 minutes
Copy link
Owner

@zhanghai zhanghai Apr 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? (Yes I forgot to submit this review : (

@zhanghai
Copy link
Owner

zhanghai commented Apr 11, 2017

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 zhanghai merged commit 2a841cf into zhanghai:master Apr 11, 2017
@zhanghai
Copy link
Owner

Merged, thanks! Will be released later.

@zhanghai
Copy link
Owner

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

@webmaster128
Copy link
Contributor Author

Cool, thanks!

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

Successfully merging this pull request may close these issues.

None yet

2 participants