If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Implement a content model view for <outliner>

RESOLVED FIXED in mozilla0.9.7

Status

()

Core
XUL
RESOLVED FIXED
16 years ago
9 years ago

People

(Reporter: David Hyatt, Assigned: janv)

Tracking

Trunk
mozilla0.9.7
x86
Windows 2000
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [xul1.0-widgets-tree])

Attachments

(3 attachments, 17 obsolete attachments)

711 bytes, patch
Details | Diff | Splinter Review
61.70 KB, patch
Brian Ryner (not reading)
: review+
Details | Diff | Splinter Review
2.72 KB, application/octet-stream
Details
(Reporter)

Description

16 years ago
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.
(Reporter)

Updated

16 years ago
Blocks: 70753
Status: NEW → ASSIGNED
Whiteboard: [xul1.0-widgets-tree]
Target Milestone: --- → mozilla0.9.6
(Assignee)

Comment 1

16 years ago
This is really great.
Blocks: 99715
Blocks: 59108

Updated

16 years ago
No longer blocks: 59108

Comment 2

16 years ago
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.
(Reporter)

Comment 3

16 years ago
You can use it without RDF.  It doesn't rely on RDF at all.
hyatt: How would this content model idea work?
(Assignee)

Comment 5

16 years ago
Created attachment 52972 [details]
outliner.xul
(Assignee)

Comment 6

16 years ago
Created attachment 52974 [details]
outliner.css
(Assignee)

Comment 7

16 years ago
Created attachment 52979 [details] [diff] [review]
work in progress - patch for content
(Assignee)

Comment 8

16 years ago
Created attachment 52980 [details] [diff] [review]
work in progress - patch for layout
(Assignee)

Comment 9

16 years ago
I've attached a testcase and unfinished patches.
There's still too much to do, but I want to provide planned design.
(Assignee)

Comment 10

16 years ago
Created attachment 53460 [details]
outliner.xul
(Assignee)

Comment 11

16 years ago
Created attachment 53461 [details]
outliner.css
(Assignee)

Comment 12

16 years ago
Created attachment 53462 [details] [diff] [review]
content.diff
(Assignee)

Comment 13

16 years ago
Created attachment 53463 [details] [diff] [review]
layout.diff
(Assignee)

Comment 14

16 years ago
hyatt, could you take a look at the last patches ?
(Assignee)

Comment 15

16 years ago
Created attachment 53545 [details] [diff] [review]
newer patch for layout
(Assignee)

Comment 16

16 years ago
Created attachment 53546 [details] [diff] [review]
newer patch for layout
(Assignee)

Comment 17

16 years ago
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.



(Assignee)

Comment 18

16 years ago
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.
(Assignee)

Comment 19

16 years ago
Created attachment 53861 [details] [diff] [review]
almost there (layout.diff)
(Assignee)

Comment 20

16 years ago
Created attachment 53862 [details]
almost there (testcase.zip)
(Reporter)

Comment 21

16 years ago
Heh, go go go! :)
(Assignee)

Comment 22

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

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

Updated

16 years ago
Attachment #52972 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #52974 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #52979 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #52980 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #53460 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #53461 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #53463 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #53545 - Attachment is obsolete: true
Attachment #53546 - Attachment is obsolete: true
(Assignee)

Comment 23

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

Comment 24

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

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

Comment 25

16 years ago
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
(Reporter)

Comment 26

16 years ago
--> 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.
Assignee: hyatt → hewitt
Status: ASSIGNED → NEW
(Assignee)

Comment 27

16 years ago
Created attachment 56306 [details]
testcase.zip
(Assignee)

Comment 28

16 years ago
Created attachment 56309 [details] [diff] [review]
patch for layout
(Assignee)

Updated

16 years ago
Attachment #53861 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #53862 - Attachment is obsolete: true
(Assignee)

Comment 29

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

Updated

16 years ago
Status: NEW → ASSIGNED
Target Milestone: mozilla0.9.6 → mozilla0.9.7
(Assignee)

Updated

16 years ago
Attachment #56309 - Attachment is obsolete: true
(Assignee)

Comment 30

16 years ago
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
(Assignee)

Comment 31

16 years ago
taking
Assignee: hewitt → varga
Status: ASSIGNED → NEW
(Assignee)

Updated

16 years ago
Status: NEW → ASSIGNED
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?
(Assignee)

Comment 33

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

but, should we remove it from compoment manager completely ?
(Assignee)

Updated

16 years ago
Blocks: 109478
(Assignee)

Comment 34

16 years ago
Created attachment 57356 [details] [diff] [review]
one which actually doesn't crash
Attachment #57090 - Attachment is obsolete: true
I don't see any need to support creating this through the component manager.

Can you summarize the change between your last two patches?
(Assignee)

Comment 36

16 years ago
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.
(Assignee)

Comment 37

16 years ago
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.
Attachment #57356 - Attachment is obsolete: true
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.
(Assignee)

Comment 40

16 years ago
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?

(Assignee)

Comment 42

16 years ago
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.
(Assignee)

Comment 45

16 years ago
"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.
Attachment #57494 - Flags: review+
(Reporter)

Comment 47

16 years ago
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.
(Assignee)

Comment 48

16 years ago
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.
(Assignee)

Comment 49

16 years ago
Created attachment 58215 [details] [diff] [review]
patch which incorporates hyatt's comments
Attachment #57494 - Attachment is obsolete: true
(Assignee)

Comment 50

16 years ago
Created attachment 58357 [details] [diff] [review]
better patch

better caching of row properties, but without caching of cell properties
Attachment #58215 - Attachment is obsolete: true
Comment on attachment 58357 [details] [diff] [review]
better patch

r=bryner
Attachment #58357 - Flags: review+
(Reporter)

Comment 52

16 years ago
sr=hyatt

Great work!
(Assignee)

Comment 53

16 years ago
Fix checked in.
(Assignee)

Comment 54

16 years ago
We have another bugs filed for (2) (3) (4)
Marking as fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
(Assignee)

Comment 55

16 years ago
Created attachment 61056 [details]
latest testcase.zip

backing up my latest testcase (sorry for spam)
Attachment #56306 - Attachment is obsolete: true

Updated

9 years ago
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: jrgmorrison → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.