Implement a content model view for <outliner>

RESOLVED FIXED in mozilla0.9.7

Status

()

defect
RESOLVED FIXED
18 years ago
11 years ago

People

(Reporter: 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
bryner
: review+
Details | Diff | Splinter Review
2.72 KB, application/octet-stream
Details
Reporter

Description

18 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

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

Comment 1

18 years ago
This is really great.
Blocks: 59108

Updated

18 years ago
No longer blocks: 59108

Comment 2

18 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

18 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

18 years ago
Posted file outliner.xul (obsolete) —
Assignee

Comment 6

18 years ago
Posted file outliner.css (obsolete) —
Assignee

Comment 7

18 years ago
Assignee

Comment 8

18 years ago
Assignee

Comment 9

18 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

18 years ago
Posted file outliner.xul (obsolete) —
Assignee

Comment 11

18 years ago
Posted file outliner.css (obsolete) —
Assignee

Comment 12

18 years ago
Posted patch content.diffSplinter Review
Assignee

Comment 13

18 years ago
Posted patch layout.diff (obsolete) — Splinter Review
Assignee

Comment 14

18 years ago
hyatt, could you take a look at the last patches ?
Assignee

Comment 15

18 years ago
Posted patch newer patch for layout (obsolete) — Splinter Review
Assignee

Comment 16

18 years ago
Posted patch newer patch for layout (obsolete) — Splinter Review
Assignee

Comment 17

18 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

18 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

18 years ago
Posted patch almost there (layout.diff) (obsolete) — Splinter Review
Assignee

Comment 20

18 years ago
Posted file almost there (testcase.zip) (obsolete) —
Reporter

Comment 21

18 years ago
Heh, go go go! :)
Assignee

Comment 22

18 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

18 years ago
Attachment #52972 - Attachment is obsolete: true
Assignee

Updated

18 years ago
Attachment #52974 - Attachment is obsolete: true
Assignee

Updated

18 years ago
Attachment #52979 - Attachment is obsolete: true
Assignee

Updated

18 years ago
Attachment #52980 - Attachment is obsolete: true
Assignee

Updated

18 years ago
Attachment #53460 - Attachment is obsolete: true
Assignee

Updated

18 years ago
Attachment #53461 - Attachment is obsolete: true
Assignee

Updated

18 years ago
Attachment #53463 - Attachment is obsolete: true
Assignee

Updated

18 years ago
Attachment #53545 - Attachment is obsolete: true
Attachment #53546 - Attachment is obsolete: true
Assignee

Comment 23

18 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

18 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

18 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

18 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

18 years ago
Posted file testcase.zip (obsolete) —
Assignee

Comment 28

18 years ago
Posted patch patch for layout (obsolete) — Splinter Review
Assignee

Updated

18 years ago
Attachment #53861 - Attachment is obsolete: true
Assignee

Updated

18 years ago
Attachment #53862 - Attachment is obsolete: true
Assignee

Comment 29

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

Updated

18 years ago
Status: NEW → ASSIGNED
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Assignee

Updated

18 years ago
Attachment #56309 - Attachment is obsolete: true
Assignee

Comment 30

18 years ago
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

18 years ago
taking
Assignee: hewitt → varga
Status: ASSIGNED → NEW
Assignee

Updated

18 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

18 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

18 years ago
Blocks: 109478
Assignee

Comment 34

18 years ago
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

18 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

18 years ago
Posted patch real patch (obsolete) — Splinter Review
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

18 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

18 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

18 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

18 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

18 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

18 years ago
Attachment #57494 - Attachment is obsolete: true
Assignee

Comment 50

18 years ago
Posted patch better patchSplinter Review
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

18 years ago
sr=hyatt

Great work!
Assignee

Comment 53

18 years ago
Fix checked in.
Assignee

Comment 54

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

Comment 55

18 years ago
Posted file latest testcase.zip
backing up my latest testcase (sorry for spam)
Attachment #56306 - Attachment is obsolete: true

Updated

11 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.