97062 - Implement a content model view for <outliner>

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[David Hyatt]

The basic idea behind this bug is as follows.

(1) Enable <outliner> to support a content model in the form of items, rows,
cells, and children.
(2) Convert <outliner> --> <tree>, and ditch the <outliner> tag.
(3) Implement <listbox> and convert uses of <tree> that require an arbitrary
content model (e.g., mailcompose) over to <listbox>.
(4) Eliminate the old <tree> code from the build entirely.

Comment 1

[Jan Varga [:janv]]

This is really great.

Comment 2

[Peter English]

The outliner in its current form is very difficult to use.  I don't see even
very good programmers using this control because it is so difficult and rely too
much on RDF. 

Should make it behave like the tree control.

Comment 3

[David Hyatt]

You can use it without RDF.  It doesn't rely on RDF at all.

Comment 4

[Hixie (not reading bugmail)]

hyatt: How would this content model idea work?

Comment 5

[Jan Varga [:janv]]

Attachment: outliner.xul

Comment 6

[Jan Varga [:janv]]

Attachment: outliner.css

Comment 7

[Jan Varga [:janv]]

Attachment: work in progress - patch for content

Comment 8

[Jan Varga [:janv]]

Attachment: work in progress - patch for layout

Comment 9

[Jan Varga [:janv]]

I've attached a testcase and unfinished patches.
There's still too much to do, but I want to provide planned design.

Comment 10

[Jan Varga [:janv]]

Attachment: outliner.xul

Comment 11

[Jan Varga [:janv]]

Attachment: outliner.css

Comment 12

[Jan Varga [:janv]]

Attachment: content.diff

Comment 13

[Jan Varga [:janv]]

Attachment: layout.diff

Comment 14

[Jan Varga [:janv]]

hyatt, could you take a look at the last patches ?

Comment 15

[Jan Varga [:janv]]

Attachment: newer patch for layout

Comment 16

[Jan Varga [:janv]]

Attachment: newer patch for layout

Comment 17

[Jan Varga [:janv]]

changes:

new methods for nsIOutlinerContentView
- nsIDOMElement GetItemAtIndex(in long index);
- long GetIndexOfItem(in nsIDOMElement item);

mContainer, mOpen, mEmpty, mSeparator (4 bytes) packed to mFlags (1 byte)
new attribute mSubrows


Each "Row" still allocates 20 bytes, so no real savings here for now, but it
might be worth in future.

Comment 18

[Jan Varga [:janv]]

I've dropped the attribute mSubrows, it's useless (now 16 bytes for each row)

I have almost working synchronization with content model.
Will attach a new patch after some cleanup and refactoring.

Comment 19

[Jan Varga [:janv]]

Attachment: almost there (layout.diff)

Comment 20

[Jan Varga [:janv]]

Attachment: almost there (testcase.zip)

Comment 21

[David Hyatt]

Heh, go go go! :)

Comment 22

[Jan Varga [:janv]]

hyatt, you ought to take a look
It works really cool!

Synchronization after inserting of item or separator is still not implemented.

Comment 23

[Jan Varga [:janv]]

Well, the synchronization is complete.
I'll try to add some comments and then I'll post patch for first review.

Comment 24

[Andy Edmonds]

Testcase looks good but I didn't see any use of .parent or .childNodes.item()

Will these be supported?  Thanks, looking forward to landing.

Comment 25

[Jan Varga [:janv]]

andy, I'm not sure what you mean, but you can use something like this:

1. to get parent index of 'row'
var outliner = document.getElementById("outliner");
var view = outliner.outlinerBoxObject.view;
dump(view.getParentIndex(row)+"\n");


2. to get content item associated with 'row'
var contentView = view.QueryInterface(Components.interfaces.nsIOutlinerContentView);
var item = contentView.GetItemAtIndex(row);

I'll attach a new patch ASAP

Comment 26

[David Hyatt]

--> hewitt, or bryner if you want to help out too.  Let's all tag team this and
get it working well enough to go into the tree.

Comment 27

[Jan Varga [:janv]]

Attachment: testcase.zip

Comment 28

[Jan Varga [:janv]]

Attachment: patch for layout

Comment 29

[Jan Varga [:janv]]

I think, basic implementation is done.
It needs more error checking, cleanup, comments and testing.

Comment 30

[Jan Varga [:janv]]

Attachment: a outliner content view implementation

Fix ready for r/sr action.

Changes:
1. Saved 4 bytes for row (now 16 bytes for each row), mLevel no longer exists.
2. Fixed some isues with parent indexes.
3. Probably more efficient HasNextSibling() method.
4. Added some comments.
5. A lot of cleanup

bryner, hyatt, waterson ... please take a look

thanks

Comment 31

[Jan Varga [:janv]]

taking

Comment 32

[Brian Ryner (not reading)]

Patch looks pretty good, I'll have to apply it and test, but I am wondering
about one thing.  Why are you going through the component manager to create a
nsOutlinerContentView when this is all inside the layout dll?

Comment 33

[Jan Varga [:janv]]

good point, I've replaced it with
|NS_NewOutlinerContentView(getter_AddRefs(contentView));|

but, should we remove it from compoment manager completely ?

Comment 34

[Jan Varga [:janv]]

Attachment: one which actually doesn't crash

Comment 35

[Brian Ryner (not reading)]

I don't see any need to support creating this through the component manager.

Can you summarize the change between your last two patches?

Comment 36

[Jan Varga [:janv]]

I've added weak reference to document - mDocument.
We need this reference because when content view goes away first,
mRoot->GetDocument() returns null. So it was impossible to 
remove content view from document's observers.
I was inspired by the nsContentList.
There is also a comment at the beginning of nsOutlinerContentView.cpp
which explains this.

Comment 37

[Jan Varga [:janv]]

Attachment: real patch

changes:

- GetItemAtIndex -> getItemAtIndex (in IDL file)
- GetIndexOfItem -> getIndexOfItem (in IDL file)
- removed component manager stuff which bryner pointed out
- fixed slightly misused RowCountChanged() method
  (why I was using everywhere index - 1, insteed of just index ?)

I converted prefs category tree to use outliner, it works just fine.
No problems till now.

Comment 38

[Brian Ryner (not reading)]

Is there a missing part of the patch that adds some new xul atoms?

Comment 39

[Brian Ryner (not reading)]

some of the items in the testcase appear to not be working, either that or I
couldn't figure out how to use them.  The outliner appears to not respond to
clicks.  It did build up the rows just fine though.

Comment 40

[Jan Varga [:janv]]

bryner, see attached content.diff patch and rename GetItemAtIndex() to
getItemAtIndex() in the testcase. That should help.

Comment 41

[Brian Ryner (not reading)]

Jan, I'm still seeing the same problem in the testcase.  Clicking on rows in the
outliner does not cause them to be selected.  The only buttons which seem to
have any effect are "remove col2 properties" and "set col2 properties".  I saved
the testcase.zip in my home directory, unzipped it, and loaded
file:///u/bryner/testcase/outliner.xul.  Am I missing something?

Comment 42

[Jan Varga [:janv]]

sigh, put it rather into chrome://
see bug 80926

Comment 43

[Brian Ryner (not reading)]

Thanks for the tip.  It now seems to work... except for one problem I noticed
where "Append children" only works if the row you are appending to has a next
sibling.

Comment 44

[Brian Ryner (not reading)]

Actually, that's not quite accurate.  "Append children" doesn't seem to always
work, but I can't figure out what causes it.

Comment 45

[Jan Varga [:janv]]

"append children" works when you are appending to item with the attribute
"container" set to "true".

Comment 46

[Brian Ryner (not reading)]

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

r=bryner

We should get this in and try it out on some real-world trees.

Comment 47

[David Hyatt]

Ok, comments.

(1) I think the logic for hooking up the content view could be improved.  IMO a
content model view should *always* be hooked up, unless there is a
XULOutlinerBuilder view.  I don't think you should be checking for children,
since you should just get a content model view even if you haven't put any kids
into the outliner yet.  So change the logic to check for the outlinerbuilder
first, and if you don't have it, just automatically hook up a content model view.

(2) GetRowProperties needs to cache tokenized values (just like the
XULOutlinerBuilder does).  Remember, this is called every time you paint!  You
don't want to be retokenizing every time.  The same goes for GetCellProperties.

(3) You need to patch nsCSSFrameConstructor's
ContentInserted/ContentAppended/ContentRemoved methods to ignore outliner tags.
 There's going to be a lot of thrashing around otherwise in the frame
constructor code as you open and close containers otherwise.

Other than that, looks great!  This is a fantastic job, Jan!  I think one more
patch will probably do it.

Comment 48

[Jan Varga [:janv]]

okey, I did (1) and (3)

but I don't have exact idea how to implement (2)
Just keep own nsISupportsArray of properties for each row ?
And how to cache cell properties ?
I'm not sure if nsXULOutlinerBuilder is doing something similar.

Comment 49

[Jan Varga [:janv]]

Attachment: patch which incorporates hyatt's comments

Comment 50

[Jan Varga [:janv]]

Attachment: better patch

better caching of row properties, but without caching of cell properties

Comment 51

[Brian Ryner (not reading)]

Comment on attachment 58357 [details] [diff] [review]
better patch

r=bryner

Comment 52

[David Hyatt]

sr=hyatt

Great work!

Comment 53

[Jan Varga [:janv]]

Fix checked in.

Comment 54

[Jan Varga [:janv]]

We have another bugs filed for (2) (3) (4)
Marking as fixed.

Comment 55

[Jan Varga [:janv]]

Attachment: latest testcase.zip

backing up my latest testcase (sorry for spam)