129327 - Outliner needs to support progressmeter

Status: RESOLVED FIXED

(Core :: XUL, defect, P3)

Description

[Blake Ross]

We have to support progressmeters in cells for download manager.

We could get away with using tree like we are now, except that it has a bad
column resizing bug; I wouldn't want to ship with it.  The bug is visible in the
Subscribe dialog: try to resize using the splitter between Subscribe and
Messages.  We could try to set reasonable defaults for each column and disable
column resizing, but that would be flaky at best.  We could also try to fix the
tree bug.

Comment 1

[Blake Ross]

Jan says he already has some code for editable cells that will likely make this
easier to fix.

Comment 2

[Peter Trudelle]

nsbeta1+

Comment 3

[Jan Varga [:janv]]

First, do we also need support for undetermined progressmeters?

Here is my idea how to implement support for progressmeters and actually for
other special content too:

XUL syntax:

<outliner ref="NC:DownloadsRoot" datasources="rdf:null">
  <outlinercols>
    <outlinercol id="name" label="&view.header.name.label;"/>
    <outlinercol id="progress" type="progressmeter"
                 label="&view.header.progress.label;"/>
  </outlinercols>
  <template>
    <outlinerchildren>
      <outlineritem uri="rdf:*">
        <outlinerrow>
          <outlinercell label="rdf:http://home.netscape.com/NC-rdf#Name"/>
          <outlinercell label="rdf:http://home.netscape.com/NC-rdf#Progress"/>
        </outlinerrow>
      </outlineritem>
    </outlinerchildren>
  </template>
</outliner>

type attribute could be:
 text
 checkbox
 progressmeter


a new CSS pseudo class which would support these properties:
outlinerchildren:-moz-outliner-progressmeter {
  background-color
  margin
  color
  border
  padding
}

maybe also:
-moz-border-[top|right|bottom|left]-colors 


Implementation:
- a new member property for nsOutlinerCOlumn: mType (eText, eCheckbox,
eProgressmeter)

- a new switch construct at the end of the PaintCell() method
switch(aColumn->mType) {
  default:
  case eText: PaintText(); break;
  case eCheckbox: PaintCheckbox(); break;
  case eProgressmeter: PaintProgressmeter(); break;
}

- PaintProgressmeter() method
This new method would query view for cell value. This value could be
from the range 0-100.
Then it would paint only part of the available cell rect. This part would be
adequate to queried value (0-100% of the cell rect)


Blake, would it satisfy your needs ?
Hyatt, does it sound reasonable to you ?

Comment 4

[Jan Varga [:janv]]

Hmm, I think there is a way to support undetermined progressmeters too.

XUL sysntax:
<outlinercol progressmode="undermined"/>

Implementation:
- install a new timer if we have undermined progressmeters
- when timer fires invalidate all columns with undermined progressmeters

Comment 5

[Jan Varga [:janv]]

uhm, I meant "undetermined", I should take a rest :)

Comment 6

[Blake Ross]

This is fine for my needs.  This is somewhat limiting as one column can only
contain one type of element.  In Download Manager, when a download finishes,
"Finished" text takes the place of the progressmeter (or "Cancelled," "Failed,"
etc. as appropriate).  But that's ok, because I use a simplistic xbl widget
<downloadstate/> for that.

Comment 7

[Blake Ross]

Er, nevermind, brain freeze -- that's not ok, since I obviously can't use the
downloadstate widget in outliner.

Comment 8

[Jan Varga [:janv]]

Shouldn't be a problem. We can just paint a text when we get it instead of an
integer value.
So, now waiting for hyatt to take a look and then I can start coding.

Comment 9

[Blake Ross]

<hyatt> jan owns outliner now.
<hyatt> he can do whatever he wants.
<hyatt> :)

Comment 10

[Jan Varga [:janv]]

Attachment: screenshot, showing basic support for progress meter

Comment 11

[Jan Varga [:janv]]

Attachment: new screenshot showing full support for progress meter

TODO:
- add new CSS style rules for Modern theme
- add new stub method GetProgressState() to other outliner views
- implement GetProgressState() for rdfliner
- investigate why undetermined progress meter hogs CPU

Comment 12

[timeless]

can you make the progress bars line up with the range that text gets for a 
collumn?

Comment 13

[Jan Varga [:janv]]

sure

Comment 14

[Blake Ross]

Jan: awesome!

Comment 15

[Blake Ross]

how's this coming?  would be better, but not critical, to give se test builds
with outliner instead of tree.

Comment 16

[Jan Varga [:janv]]

I need to fix TODO list mentioned above.
I was flooded by other bug reports and other stuff.
Once I clean it up I'll post a patch.

Comment 17

[Jan Varga [:janv]]

Attachment: patch v1.0

Comment 18

[Jan Varga [:janv]]

Attachment: outliner-content.xul

Comment 19

[Jan Varga [:janv]]

Attachment: outliner-rdf.xul

Comment 20

[Jan Varga [:janv]]

Attachment: downloads.rdf

Comment 21

[Jan Varga [:janv]]

I figured out that undetermined progress meter hogs CPU only when using Classic
skin.
Mac progress meter also needs some loving.

Comment 22

[Jan Varga [:janv]]

This is really bad:

outlinerchildren:-moz-outliner-progressmeter(progressUndetermined) {
  background: url("chrome://global/skin/progressmeter/progressmeter-busy.gif");
}

since it invalidates all outliner very often.
Need a better way for undetermined progressmeter.

Comment 23

[Jan Varga [:janv]]

Ok, I figured out that perf issue with undetermined progressmeter.

But there is still a general perf issue with animated images in outliner.
If an undetermined progressmeter (animated image) goes to normal or none
mode, cell gets invalidated all over again. In other words, if a cell has
animated image once, it is still considered as animated image even though it has
no animated image anymore.

Comment 24

[Blake Ross]

Jan, can you fix it so that I can switch between progressmeter and text by
setting numeric and non-numeric values, respectively? Maybe not the best
solution in the end, but that's the only thing preventing me from converting
download manager to outliner (which is always blocking Joe's work).

Comment 25

[Jan Varga [:janv]]

Attachment: patch ready for review

This patch should be ready for a review.
I fixed undetermined progressmeters by using |list-style-image| instead of
|background| and using DrawTile instead of DrawImage for image painting.
It is also possible to use an image to paint determined progress, like we do on
Mac

Once this lands, I'll fill a separate bug for the issue with animated images.

Comment 26

[Jan Varga [:janv]]

Attachment: real patch

Comment 27

[Brian Ryner (not reading)]

Comment on attachment 76203 [details] [diff] [review]
real patch

r=bryner, looks good

Comment 28

[David Hyatt]

Comment on attachment 76203 [details] [diff] [review]
real patch

sr=hyatt

Comment 29

[Asa Dotzler [:asa]]

Comment on attachment 76203 [details] [diff] [review]
real patch

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 30

[Joe Hewitt (gone)]

fixed

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Bug 136860 is a regression of this.
Just one "getImageSrc: function(row, colID){}"
is missing in content/global/config.js

Comment 32

[Jan Varga [:janv]]

yeah, I forgot to add it there.