Last Comment Bug 634086 - Use native rendering for indeterminate progress bar (GTK)
: Use native rendering for indeterminate progress bar (GTK)
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Gtk (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla6
Assigned To: Mounir Lamouri (:mounir)
:
:
Mentors:
Depends on: 634088
Blocks: 633207
  Show dependency treegraph
 
Reported: 2011-02-14 14:37 PST by Mounir Lamouri (:mounir)
Modified: 2011-05-11 05:40 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Example of what is expected (1.59 KB, text/plain)
2011-02-17 01:31 PST, Mounir Lamouri (:mounir)
no flags Details
Patch v1 (7.10 KB, patch)
2011-02-17 08:32 PST, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v1.1 (7.85 KB, patch)
2011-03-14 10:14 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v2 (7.33 KB, patch)
2011-03-17 17:07 PDT, Mounir Lamouri (:mounir)
karlt: review-
karlt: feedback+
Details | Diff | Splinter Review
Inter diff (5.43 KB, patch)
2011-04-20 08:08 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v3 (7.19 KB, patch)
2011-04-20 08:11 PDT, Mounir Lamouri (:mounir)
karlt: review+
Details | Diff | Splinter Review
Inter diff (2.13 KB, patch)
2011-05-04 07:34 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2011-02-14 14:37:09 PST
AFAIUI, it would required using this:
http://library.gnome.org/devel/gtk/unstable/GtkProgressBar.html#gtk-progress-bar-pulse
Comment 1 Mounir Lamouri (:mounir) 2011-02-15 05:58:17 PST
So, it seems like it harder than expected because of gtk_paint_box.
First of all, given that we only have one instance of the progress bar widget, we would need at least two instance (one to use when the widget is in indeterminate state and one when it is not). Even, that would be quite ugly when there would be more than one indeterminate progress bar in the page (they would have the exact same rendering).
In addition, when I set any property (like gtk_progress_bar_set_fraction(GTK_PROGRESS_BAR(gProgressWidget), 0.1f);), this is not reflected when painting with gtk_paint_box. I assume this is related to gtk_paint_box because I've been able to write a small gtk program showing an indeterminate progress bar.

Karl and Roc, do you have any advice?
Comment 2 Mounir Lamouri (:mounir) 2011-02-17 01:31:20 PST
Created attachment 513069 [details]
Example of what is expected

That's what I would like to do. After a discussion on IRC, we think that the best idea is to simulate the behavior you can see in this file.
Comment 3 Mounir Lamouri (:mounir) 2011-02-17 06:43:00 PST
BTW, if you want to try the example:
gcc gtk.c `pkg-config --cflags --libs gtk+-2.0` -o gtk-progress-test && ./gtk-progress-test
Comment 4 Mounir Lamouri (:mounir) 2011-02-17 08:32:33 PST
Created attachment 513145 [details] [diff] [review]
Patch v1

I don't know if I should add a 'progresschunk-indeterminate' appearance or only have the widget code doing what should be done. I tend to think that we don't need 'progresschunk-indeterminate'. Let me know if I should change my patch and add an appearance.
Comment 5 Karl Tomlinson (:karlt) 2011-02-20 21:11:08 PST
With gtk_progress_bar_pulse(), the intention is that the progress bar only changes when some progress has been made.  Thus, even though the user has no idea where the goal line is, they know how fast they are heading towards it.

Can the native drawing code be provided with some counter to indicate how many steps of progress have been made?
Comment 6 Mounir Lamouri (:mounir) 2011-02-21 01:16:19 PST
(In reply to comment #5)
> With gtk_progress_bar_pulse(), the intention is that the progress bar only
> changes when some progress has been made.  Thus, even though the user has no
> idea where the goal line is, they know how fast they are heading towards it.

That is indeed what gtk_progress_bar_pulse idea but I think a lot of uses are just to show something moving to let the user know there is a progress, like here. I don't think using this moving progress bar is inappropriate.

> Can the native drawing code be provided with some counter to indicate how many
> steps of progress have been made?

I don't think that would be a good idea. In the indeterminate state, we just don't know the progress (that's when the bar is moving back and forward) and in the other case (when we know the progress), I would prefer to leave that to the authors: they know the progress so they can show it as a counter if they think it's relevant.
Comment 7 Karl Tomlinson (:karlt) 2011-02-21 14:56:20 PST
(In reply to comment #6)
> That is indeed what gtk_progress_bar_pulse idea but I think a lot of uses are
> just to show something moving to let the user know there is a progress, like
> here. I don't think using this moving progress bar is inappropriate.

Careful with your words here.  As implemented here, something moving does not indicate progress; it merely indicates time passing (waiting).

It would be nice if progress and stall could be distinguished, but that depends on what the progress element provides.  The spec seems confused about the meaning of "progress":

"The progress is either indeterminate, indicating that progress is being made but that it is not clear how much more work remains to be done before the task is complete (e.g. because the task is waiting for a remote host to respond)."
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-element.html#the-progress-element

If the task is "waiting for a remote host", progress is *not* being made.

However, given that the indeterminate progress element doesn't provide any distinction between progress and lack of progress, a moving bar for both conditions is probably the best we can do.
Comment 8 Mounir Lamouri (:mounir) 2011-02-28 09:43:41 PST
If I understand what you say correctly, you are doing a distinction between:
1. progress element with a known progress (@value) but an unknown max? So we know we are progressing but we have no idea when it's going to stop ;
2. progress element with an unknown value which means we have no idea where the progress is.

I think the idea of the progress element in the indeterminate state is "something is happening but we can't really tell you more about where we are". Are OS's really doing a difference between 1 and 2? On MacOS X, there is only one type of indeterminate progress bar which has some animation (but not really moving). On GTK, I saw this kind of moving progress bar and another kind like in MacOS X (but only once). On Windows (at least 7), it's like the one implemented here.
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-02-28 14:55:04 PST
Comment on attachment 513145 [details] [diff] [review]
Patch v1

You don't need my review here, I trust Karl :-)
Comment 10 Karl Tomlinson (:karlt) 2011-02-28 14:58:38 PST
(In reply to comment #8)
> If I understand what you say correctly, you are doing a distinction between:
> 1. progress element with a known progress (@value) but an unknown max? So we
> know we are progressing but we have no idea when it's going to stop ;
> 2. progress element with an unknown value which means we have no idea where the
> progress is.

Yes.  I claim 2 is not progress at all but merely indicates waiting for something to happen.

> Are OS's really doing a difference between 1 and 2? On MacOS X, there is only
> one type of indeterminate progress bar which has some animation (but not really
> moving). On GTK, I saw this kind of moving progress bar and another kind like
> in MacOS X (but only once). On Windows (at least 7), it's like the one
> implemented here.

gtk_progress_bar_pulse is for 1.  GtkSpinner seems the appropriate GTK widget for 2, but that is fairly new, so I guess there have been apps using GtkProgressBar for 2.
http://library.gnome.org/devel/gtk/unstable/GtkSpinner.html

I don't know what other OSes provide.

What you have here is probably sensible given the constraints of the spec.  I merely haven't finished the review yet because of FF4 work.
Comment 11 Mounir Lamouri (:mounir) 2011-02-28 17:41:07 PST
(In reply to comment #10)
> What you have here is probably sensible given the constraints of the spec.  I
> merely haven't finished the review yet because of FF4 work.

I just needed to know if some changes were required. You can take your time for the review, it's far from blocking me right now ;)
Comment 12 Mounir Lamouri (:mounir) 2011-03-14 10:14:29 PDT
Created attachment 519170 [details] [diff] [review]
Patch v1.1

I minor bug was living in the previous patch.
Comment 13 Mounir Lamouri (:mounir) 2011-03-17 17:07:26 PDT
Created attachment 520079 [details] [diff] [review]
Patch v2

Much more simple patch that let us set the speed and size of the moving bar depending of the entire widget size or make them constants for all progress bars. For the moment, the size is 20% of the widget and the speed is make so that a cycle is a bit less than 2 secs.
Comment 14 Mounir Lamouri (:mounir) 2011-04-09 00:40:13 PDT
Karl, any ETA for the review?
Comment 15 Karl Tomlinson (:karlt) 2011-04-19 23:43:17 PDT
Comment on attachment 520079 [details] [diff] [review]
Patch v2

>+                             gboolean indeterminate)

Boolean parameters are not so good for readability at the call site.  Here it
is easy enough to avoid by passing the GtkThemeWidgetType through to
moz_gtk_progress_chunk_paint and testing against
MOZ_GTK_PROGRESS_CHUNK_INDETERMINATE there.

>+      const gint barWidth = rect->width*0.2;

With formulas that take integers as input and produce an integer as output, it
is more conventional to stick with integer arithmetic.  With rounding to
nearest, that would be (width + 2) / 5.

Add a comment explaining that the constant is based on the default value of 5
for the GtkProgressBar activity-blocks property.

>+      const gint progressWidth = 2 * (rect->width - barWidth);

Perhaps "progressTravel", or "travel".

>+      const gdouble interval = progressWidth / pixelsPerMillisecond;

Use the constant 1600 (or 2000 for 2 seconds, if you like) instead of
calculating it each time.

I would call this "period".  One advantage is that it is clearly different
from the NSPR "interval".

>+      const gdouble ratio = (gdouble)modf(
>+          PR_IntervalToMilliseconds(PR_IntervalNow())/interval, &tempValue);
>+      const gint dx = (gint)(progressWidth * ratio);

I'll note that the right thing will not happen when the PRIntervalTime wraps
unless the period measured in NSPR ticks is a power of 2, but that should
happen less often than once every twelve hours, so it's not worth worrying
about.

Again, integer arithmetic could be used here.  There's probably multiple
options but how about this?:

PRUint32 period = 1600;
PRUint32 t = PR_IntervalToMilliseconds(PR_IntervalNow()) % period;
gint dx = travel * t  / period;

>+  // Indeterminate progress bar are animated.
>+  if (aWidgetType == NS_THEME_PROGRESSBAR_CHUNK ||
>+      aWidgetType == NS_THEME_PROGRESSBAR_CHUNK_VERTICAL) {
>+    nsIFrame* stateFrame = aFrame->GetParent();
>+    nsEventStates states =  GetContentState(stateFrame, aWidgetType);
>+    if (IsIndeterminateProgress(stateFrame, states)) {

It's simpler to check gtkWidgetType == MOZ_GTK_PROGRESS_CHUNK_INDETERMINATE.

>+      if (!QueueAnimatedContentForRefresh(aFrame->GetContent(), 60)) {

30 frames per second should be plenty here.

Can you move this to the end of DrawWidgetBackground, please?
I'd like to separate this from the drawing and safeState code.
Comment 16 Mounir Lamouri (:mounir) 2011-04-20 08:08:44 PDT
Created attachment 527272 [details] [diff] [review]
Inter diff
Comment 17 Mounir Lamouri (:mounir) 2011-04-20 08:11:01 PDT
Created attachment 527273 [details] [diff] [review]
Patch v3

You can use the interdiff to speed up the review.

And I didn't understand why you said I could get the barWidth with (width+2)/5, width/5 seems to be what we want here, isn't it?
Comment 18 Karl Tomlinson (:karlt) 2011-04-24 23:48:35 PDT
Comment on attachment 527273 [details] [diff] [review]
Patch v3

Review of attachment 527273 [details] [diff] [review]:

In reply to comment #17)
> And I didn't understand why you said I could get the barWidth with (width+2)/5,
> width/5 seems to be what we want here, isn't it?

(width+2)/5 would round to nearest, while width/5 would round down.

I see gtk rounds down, so it makes sense to follow that:

 MAX (2, widget->allocation.width / pbar->activity_blocks)

Note however, the minimum of 2.  I don't know where 2 would come from, but it
may be worth either adding a minimum of at least 1 or skipping the paint if
the width is zero, so as not to surprise theme engines that may divide by zero.

::: widget/src/gtk2/gtk2drawing.c
@@ -2239,3 @@
 {
     GtkStyle* style;
-

Leave this blank line in place.  It is common in C to separate declarations and statements with a blank line, and usually we try to avoid unnecessary whitespace changes.

@@ +2255,5 @@
+
+      /* The bar is using a fifth of the element size, based on GtkProgressBar
+       * activity-blocks property. */
+      const gint barWidth = rect->width / 5;
+      const gdouble pixelsPerMillisecond = rect->width / 1000.0;

pixelsPerMillisecond is unused and so can be removed

@@ +2260,5 @@
+
+      /* Represents the travel that has to be done for a complete cycle. */
+      const gint travel = 2 * (rect->width - barWidth);
+
+      /* period equals to travel / pixelsPerMillisecond which is

and so this comment will need to be updated.

@@ +2263,5 @@
+
+      /* period equals to travel / pixelsPerMillisecond which is
+       * equivalent to 1600. */
+      const gint period = 1600;
+      const gdouble ratio = PR_IntervalToMilliseconds(PR_IntervalNow()) % period;

|ratio| should be integer.  I would call it |t| because it is not really a ratio.

|period| might as well be PRUint32 because, on a system where unsigned int has 32 bits of precision, gint would be promoted to PRUint32 to match the other operand of the % operation.
Comment 19 Mounir Lamouri (:mounir) 2011-05-04 07:34:50 PDT
Created attachment 530001 [details] [diff] [review]
Inter diff

With review comments.
Comment 20 Karl Tomlinson (:karlt) 2011-05-04 13:50:35 PDT
Please remove "const gdouble pixelsPerMillisecond".
Comment 21 Mounir Lamouri (:mounir) 2011-05-05 06:11:51 PDT
(In reply to comment #20)
> Please remove "const gdouble pixelsPerMillisecond".

Sorry about that. Fixed locally.
Comment 22 Mounir Lamouri (:mounir) 2011-05-09 05:40:05 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/0695497a4b52
Comment 23 Shawn Wilsher :sdwilsh 2011-05-09 16:11:30 PDT
Backed out in http://hg.mozilla.org/mozilla-central/rev/dd9ba28d2bd9 to resolve bug 655860.
Comment 24 Mounir Lamouri (:mounir) 2011-05-10 07:06:25 PDT
Re-landed:
http://hg.mozilla.org/mozilla-central/rev/3859797a8fea

Note You need to log in before you can comment on or make changes to this bug.