442419 - allow custom max for progressmeter

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, enhancement)

Description

[Mook (songbird dead account)]

<progressmeter> currently (when determined) has possible values of 0 to 100 in increments of 1.  For a very wide progressmeter, this means the value can be coarse.  I think it would be useful to add a max= attribute (a positive integer) that can be used to determine the total possible value, defaulting to 100 (so there is no behaviour change).

I was thinking of having a custom min= as well, but that gets complicated (needing to bound both values correctly, needing to shift values) when the (XUL widget) user can do that easily.

Comment 1

[Mook (songbird dead account)]

Attachment: add max= attribute to <xul:progressbar>

Patch for Mac (it just has to be special!) and everybody-else.  Includes reftest, but I'm not sure if I did that part right.  (It compares 50/100 against 99/198)

Comment 2

[Neil Deakin]

Comment on attachment 327920 [details] [diff] [review]
add max= attribute to <xul:progressbar>

>diff --git a/layout/xul/base/src/nsProgressMeterFrame.cpp b/layout/xul/base/src/nsProgressMeterFrame.cpp
>-    if (flex < 0) flex = 0;
>-    if (flex > 100) flex = 100;
>+    PRInt32 maxFlex = maxValue.ToInteger(&error);
>+    if (maxValue.IsEmpty()) {
>+      maxFlex = 100;

Seems like this should default to 100 if there was an error parsing.

>+      <property name="max"
>+                onget="return this.getAttribute('max') || '100';"
>+                onset="this.setAttribute('max', Math.max(val, 1));"/>

Similarly here.

Ideally, setting the max to something will also adjust the value to the maximum if it is higher.

Comment 3

[Stephen Lau]

Attachment: Updated patch for tip & for enndeakin's comments

Hi Neil - I've updated Mook's patch so it applies against mozilla-central tip, and tried to take into account your comments for defaulting to 100 in the event of a parse error.
Mind reviewing this again?

Comment 4

[Neil Deakin]

Comment on attachment 340452 [details] [diff] [review]
Updated patch for tip & for enndeakin's comments


>+      <property name="max"
>+                onget="return this.getAttribute('max') || '100';"
>+                onset="if (isNaN(val)) this.setAttribute('max', 100); else this.setAttribute('max', Math.max(val, 1)); if (this.getAttribute('value') > val) this.setAttribute('value', Math.max(val, 1));"/>

This is quite complicated. It should go in a <setter> block, or at least be better spaced out. Also, a setter needs to return 'val' as well. Updating the value should be a matter of just setting this.value = this.value i would think. Something like the following perhaps?

onset="this.setAttribute("max", isNaN(val) ? 100 : Math.max(val, 1);
       this.value = this.value;
       return val;"

Comment 5

[Stephen Lau]

Attachment: updated patch with better onset

Thanks, I've updated to continue using the onset with your cleaner suggestion.  (Didn't occur to me that simply doing this.value = this.value would invoke the getter's logic for capping it at the max)

Comment 6

[Serge Gautherie (:sgautherie)]

Comment on attachment 344702 [details] [diff] [review]
updated patch with better onset

>--- a/layout/xul/base/src/nsProgressMeterFrame.cpp
>+++ b/layout/xul/base/src/nsProgressMeterFrame.cpp
>@@ -129,15 +129,27 @@ nsProgressMeterFrame::AttributeChanged(P
>+    if (!NS_SUCCEEDED(error) || maxValue.IsEmpty()) {

s/!NS_SUCCEEDED/NS_FAILED/ !?

Comment 7

[Stephen Lau]

Attachment: tiny update to use NS_FAILED instead of !NS_SUCCEEDED

Good idea Serge, here's a new patch that uses NS_FAILED instead of NS_SUCCEEDED. I assume this doesn't need new r/sr since it's so trivial.

Comment 8

[(no longer active)]

Pushed to mozilla-central: <http://hg.mozilla.org/mozilla-central/rev/0c2c2c895e5d>

Comment 9

[Dave Townsend [:mossop]]

Pushed a bustage fix for this: http://hg.mozilla.org/mozilla-central/rev/b9dd40cbf22d

Comment 10

[Dave Townsend [:mossop]]

Oops, missed a bit: http://hg.mozilla.org/mozilla-central/rev/5fdc78311d6b

Comment 11

[Stephen Lau]

augh, sorry bout that. Thanks Mossop!

Comment 12

[Markus Stange [:mstange]]

It looks like the added reftest hasn't ever been run because toolkit/content/tests/reftests/reftest.list isn't included in layout/reftests/reftest.list.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Markus Stange [:mstange] from comment #12)
> It looks like the added reftest hasn't ever been run because
> toolkit/content/tests/reftests/reftest.list isn't included in
> layout/reftests/reftest.list.

I'll fix that in bug 989688.