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.
This is really great.
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.
You can use it without RDF. It doesn't rely on RDF at all.
hyatt: How would this content model idea work?
I've attached a testcase and unfinished patches. There's still too much to do, but I want to provide planned design.
hyatt, could you take a look at the last patches ?
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.
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.
Heh, go go go! :)
hyatt, you ought to take a look It works really cool! Synchronization after inserting of item or separator is still not implemented.
Well, the synchronization is complete. I'll try to add some comments and then I'll post patch for first review.
Testcase looks good but I didn't see any use of .parent or .childNodes.item() Will these be supported? Thanks, looking forward to landing.
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
--> 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.
I think, basic implementation is done. It needs more error checking, cleanup, comments and testing.
Created attachment 57090 [details] [diff] [review] 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
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?
good point, I've replaced it with |NS_NewOutlinerContentView(getter_AddRefs(contentView));| but, should we remove it from compoment manager completely ?
Created attachment 57356 [details] [diff] [review] one which actually doesn't crash
I don't see any need to support creating this through the component manager. Can you summarize the change between your last two patches?
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.
Created attachment 57494 [details] [diff] [review] 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.
Is there a missing part of the patch that adds some new xul atoms?
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.
bryner, see attached content.diff patch and rename GetItemAtIndex() to getItemAtIndex() in the testcase. That should help.
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?
sigh, put it rather into chrome:// see bug 80926
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.
Actually, that's not quite accurate. "Append children" doesn't seem to always work, but I can't figure out what causes it.
"append children" works when you are appending to item with the attribute "container" set to "true".
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.
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.
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.
Created attachment 58215 [details] [diff] [review] patch which incorporates hyatt's comments
Created attachment 58357 [details] [diff] [review] better patch better caching of row properties, but without caching of cell properties
Comment on attachment 58357 [details] [diff] [review] better patch r=bryner
sr=hyatt Great work!
Fix checked in.
We have another bugs filed for (2) (3) (4) Marking as fixed.
Created attachment 61056 [details] latest testcase.zip backing up my latest testcase (sorry for spam)