Closed Bug 442419 Opened 12 years ago Closed 11 years ago

allow custom max for progressmeter


(Toolkit :: XUL Widgets, enhancement)

Not set





(Reporter: mook, Assigned: mook)





(2 files, 2 obsolete files)

<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.
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)
Attachment #327920 - Flags: review?
Attachment #327920 - Flags: review? → review?(enndeakin)
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.
Attachment #327920 - Flags: review?(enndeakin) → review+
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?
Attachment #340452 - Flags: review?(enndeakin)
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;"
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)
Attachment #340452 - Attachment is obsolete: true
Attachment #344702 - Flags: review?(enndeakin)
Attachment #340452 - Flags: review?(enndeakin)
Attachment #344702 - Flags: review?(enndeakin) → review+
Attachment #327920 - Attachment is obsolete: true
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()) {

Whiteboard: [c-n: wait for comment 6, or not ?]
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.
Pushed to mozilla-central: <>
Closed: 11 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [c-n: wait for comment 6, or not ?]
Target Milestone: --- → mozilla1.9.1b2
augh, sorry bout that. Thanks Mossop!
Blocks: songbird
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.
(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.
You need to log in before you can comment on or make changes to this bug.