Open Bug 441414 Opened 16 years ago Updated 2 years ago

Treerows need a way to hold richer content

Categories

(Core :: XUL, enhancement)

enhancement

Tracking

()

People

(Reporter: paul, Unassigned)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [bounty])

Attachments

(14 files, 22 obsolete files)

21.54 KB, image/png
Details
112.46 KB, image/png
Details
162.92 KB, image/png
Details
15.88 KB, image/png
Details
12.36 KB, image/png
Details
28.98 KB, patch
enndeakin
: review-
Details | Diff | Splinter Review
30.02 KB, patch
Details | Diff | Splinter Review
5.71 KB, application/zip
Details
8.09 KB, application/zip
Details
1.50 KB, patch
seth
: review+
neil
: review+
Details | Diff | Splinter Review
4.90 KB, patch
seth
: review+
dbaron
: superreview-
evilpie
: feedback+
Details | Diff | Splinter Review
2.62 KB, patch
seth
: review+
dbaron
: superreview-
Details | Diff | Splinter Review
4.24 KB, patch
Details | Diff | Splinter Review
30.64 KB, patch
dbaron
: superreview-
WeirdAl
: feedback-
Details | Diff | Splinter Review
The current treerow layout is a horizontally drawn set of cells (width based on the column width, and height based on the row height). Each cell could hold a text (cropped if needed, but using just one line) which could be styled as a all (no way to make one piece of the text green and another piece red). Here, I propose a way to improve the current tree mechanism in order to make treerows richer without breaking the current API. The following does not mean we have to rewrite every nsITreeView implementation. It would be especially useful for TB and its message list's pane (see bug 213945). Any comments about the relevancy of such improvement and about the following mechanism are welcome :) Proposed new layout: ------------ Each row use the same height (needed for performance issues, like current behaviour) (CSS defined); The Y offset and the height of each cell are independent (CSS defined), not based on the row height. The width and the X offset of each cell is the same as the related column (like current behavior). The text embedded in each cells is reachable as a set of piece of text ("items", reachable from the new treeview API). Each piece of text is independently stylable (through properties set from on the new treeview API). The text is drawn using all available vertical space. A rect embedding text ("description") is drawn in the top of the cells. This block works like cells, but the X offset and the width is not restricted (CSS defined). Overlap with cells is assumed by the CSS rules. CSS rules: If the rows and the cells use this new mechanism: Rows and cells have a new property: "multiline"; The new block ("description") is stylable from a new pseudo-element: -moz-tree-description, and description items from -moz-tree-description-item; The cell items are stylable from a new pseudo-element: -moz-tree-cell-item; Each item and description could have a set of properties set from the new treeview API; nsITreeView2: interface nsITreeView2: nsITreeView { AString getCellItemText(in long row, in nsITreeColumn col, in long idxItem); long getCellItemCount(in long row, in nsITreeColumn col); void getCellItemProperties(in long row, in nsITreeView col, in long idxItem, in nsISupportsArray properties); AString getDescriptionItemText(in long row, in long idxItem); long getDescriptionItemCount(in long row); void getDescriptionItemProperties(in long row, in long idxItem, in nsISupportsArray properties); boolean isMultiline(); }; The "isMultiline" is useful if you want the TreeBodyFrame to use the multiline view or the classic view. This could be useful if you want to let the user choose the desired view (you don't have to implement two treeview). Know problems: The main issue is about the description box coordinates. If you want to display the description below the cells, you would set a top margin to avoid overlap. But you could want to make the last cell bigger (a cell using all the row height). Then, you must define a right margin in the description. But if your columns could be resized or could be moved, then, an overlap is still possible. What should be the correct description behavior in a hierarchic tree ? Should it follow the indentation ? Could be optionnal. P.S: used termes (description, items, multine, nsITreeView2) may be not very relevant. Feel free to propose alternatives :)
Attached file a XulRunner app using nsITreeView2 (obsolete) —
Attached image what it looks like (obsolete) —
More screenshots here: http://www.mozbox.org/pub/tb/multilineView/ Mainly TB related.
Blocks: 213945
Blocks: 364090
It'd be really good to see something like this in 1.9.1 for Tb3. bz (out of a hat): any chance you could give this patch some quick feedback, so that Paul can know what's needed to make it review-ready?
Flags: wanted1.9.1?
It'd make sense to have someone sorta familiar with trees look at this instead of me...
I didn't look at the code in any detail. Some comments: > interface nsITreeView2: nsITreeView You should either just add the new methods to nsITreeView or use a more descriptive name like nsIMultilineTreeView. 'treeitem' is already used as a tagname, so calling the text pieces 'items' is confusing. Instead, I would suggest 'textpart' as in '-moz-tree-cell-text-part' or getCellTextPart. > What should be the correct description behavior in a hierarchic tree ? Each text part should just be drawn one underneath each other. I'm not clear what 'description' is and how is differs from 'text parts'. > + nsCOMPtr<nsITreeView2> mView2; Don't store this separately. Just get it again when needed. What does 'multiline' get you? Multiple parts of text or one piece of text on multiple lines? Is getItemText(0, 0) supposed to be equivalent to getCellText(0)?
(In reply to comment #6) > I didn't look at the code in any detail. Some comments: > > > interface nsITreeView2: nsITreeView > > You should either just add the new methods to nsITreeView or use a more > descriptive name like nsIMultilineTreeView. Changing nsITreeView requires an update for each nsITreeView implementation. nsIMultilineTreeVew sounds good. > 'treeitem' is already used as a tagname, so calling the text pieces 'items' is > confusing. Instead, I would suggest 'textpart' as in '-moz-tree-cell-text-part' > or getCellTextPart. Yes, it sounds more appropriate. > > What should be the correct description behavior in a hierarchic tree ? > > Each text part should just be drawn one underneath each other. You mean descriptions boxes should not be impacted by the hierarchic behavior ? No left margin ? > I'm not clear what 'description' is and how is differs from 'text parts'. The description is a box containing a set of "textpart" (as a cell), but it's not anchored to any column. It's drawn directly in the row (the blue part in attachment 326398 [details]). > > + nsCOMPtr<nsITreeView2> mView2; > > Don't store this separately. Just get it again when needed. > > What does 'multiline' get you? Multiple parts of text or one piece of text on > multiple lines? Multiple parts of text on multiple lines: a cell or a description box: ___________________ |part1part2part3pa| |rt4part5part6 | |_________________| > > Is getItemText(0, 0) supposed to be equivalent to getCellText(0)? > No. var foobar = getCellText(0); is equivalent to: var count = getCellItemCount(0); var foobar = ""; for (var i = 0; i < count; i++) { foobar += getCellItemText(0, i); }
I'm still a bit confused by item parts and descriptions. There are several 'blue parts' in that image. Also, the description api you mention in your first comment doesn't exist in the patch you provided. Can you provide a example of what would be displayed in the following: getCellText(x, y) returns "Cell Text" getCellTextPart(x, y, 0) returns "Apples" getCellTextPart(x, y, 1) returns "Oranges" (and assuming you meant to implement getDescriptionText as well): getDescriptionText(x, 0) returns "Lion" getDescriptionText(x, 1) returns "Tiger" All getXXXProperties return nothing of interest, and no stylesheet is used.
(In reply to comment #8) > I'm still a bit confused by item parts and descriptions. There are several > 'blue parts' in that image. Description boxes are boxes containing text like "Path: .......". One description box per row (only 3 are blue). > Also, the description api you mention in your first > comment doesn't exist in the patch you provided. Yes, I'm sorry, the patch doesn't really match this proposed API. In this patch, multipart concept was just about description, not about cells, so getItemText (in the patch) is the equivalent of getDescriptionText (in this proposed API) ... a bit confusing. > Can you provide a example of what would be displayed in the following: > > getCellText(x, y) returns "Cell Text" If the component implements nsIMultilineTreeView, getCellText isn't used. > getCellTextPart(x, y, 0) returns "Apples" > getCellTextPart(x, y, 1) returns "Oranges" > (and assuming you meant to implement getDescriptionText as well): > getDescriptionText(x, 0) returns "Lion" > getDescriptionText(x, 1) returns "Tiger" > All getXXXProperties return nothing of interest, and no stylesheet is used. > So the text in the cell x/y will be: "ApplesOranges" (which would be cropped and displayed using several lines if needed). The text in the description box will be: "LionTiger" (which would be cropped and displayed using several lines if needed). In short: If a treeview implements nsITreeView and not nsIMultilineTreeView, it's drawn as the old way. If it implements nsIMultilineTreeView, getCellText isn't used, but getCellTextPart are. In addition, an other box is drawn, named "description", using the getDescriptiontext methods, and drawn relatively to the row. I hope I'm more understandable.
No, as I don't understand what a description box is and how it relates to the cell text parts and the cell value, I want an actual screenshot or a diagram showing exactly what would be displayed in the example I gave. getCellText needs to return a value for accessibility purposes.
(In reply to comment #9) > Description boxes are boxes containing text like "Path: .......". One > description box per row (only 3 are blue). There are seven boxes in the image that are blue, all of which contain the text 'Path:'
Attached image Neil example
Blue boxes are description boxes. Red boxes are cells.
Ah, OK, now I understand. The code doesn't implement it this way I think, which is why I couldn't figure this out. So this looks pretty good then. The text parts specify pieces of text to use instead of getCellText which specifies only one piece. In that case, it makes sense that implementors make getCellText return the text appended together, or some variation in case only certain parts of the text are useful. The description applies to the entire row and is displayed underneath the column text. I think it should appear flush with the cell text of the primary column. Both text parts and the description wrap right? Do text parts wrap only at part boundaries or in between a string when reaching the end of the line?
(In reply to comment #14) > Ah, OK, now I understand. The code doesn't implement it this way I think, which > is why I couldn't figure this out. So this looks pretty good then. > > The text parts specify pieces of text to use instead of getCellText which > specifies only one piece. In that case, it makes sense that implementors make > getCellText return the text appended together, or some variation in case only > certain parts of the text are useful. Yes. > The description applies to the entire row and is displayed underneath the > column text. Cells could be displayed underneath the description box. It depends on the CSS rules. In the screenshot, description boxes have a "margin-top: 18px;" rule, and cells have a "height: 18px" rule. But such behavior could be wrong. Coords are calculated from fixed rules (CSS), so if a column is moved, that could create an overlap. Example: In the last screenshot (attachment 327116 [details]), the last cell is bigger. No overlap because the description box have a right margin (defined in the css file). But, if the user resize the XXX column, the content of the cell could overlap the content of the description. So dynamically calculate the description coords and size (in order to make the box use the maximum space available) could be the solution... I don't know. > I think it should appear flush with the cell text of the primary > column. Ok. > Both text parts and the description wrap right? Yes. > Do text parts wrap only at part boundaries or in between a string when reaching the end of the line? Between a string, example: 4 text parts: "part1", "bigpart2", "part3", "part4" ___________ |part1bigpa| |rt2part...| |__________|
(In reply to comment #15) > Cells could be displayed underneath the description box. It depends on the CSS > rules. In the screenshot, description boxes have a "margin-top: 18px;" rule, > and cells have a "height: 18px" rule. Where are these rules specified? I'd like a screenshot of what it looks like where the developer has *no stylesheets defined*. All of the functionality should work as is and be displayed in an appropriate manner without the developer having to use any stylesheets.
Attached patch updated patch using proposed API (obsolete) — Splinter Review
This patch is based on the proposed API. Graphical elements works without any user defined style.
Assignee: nobody → paul.rouget
Attachment #326396 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #326397 - Attachment is obsolete: true
Attached image what it looks like
Attachment #326398 - Attachment is obsolete: true
What about the keyboard navigation model of this new widget? Does it match standard tree views, or is the behavior different when it's in multi-line mode?
The keyboard navigation model is not affected.
Attachment #328912 - Attachment is obsolete: true
In order to make the patch review-ready, I need: * some feedback about the code (which is currently pretty rough) * a validation of the current behavior * a validation of the current API/used names/CSS rules * some information about accessibility issues (is there any stuff I should care about ?) * someone who can test the patch on Windows and MacOS ? * any advise you could give me :) ... and I must write some tests.
Yet, Thunderbird team does not clearly define their needs about the new tree layout. For instance, we're talking about rows with different sizes and a set of "description" like boxes which could host content like image/singlepart text/multipart text. While we are not ok about what it's needed, I don't update my code.
I think the ill-defined needs referred to are those of the 'experimental message view', which is going to go with a different approach, at least for prototyping. So let's take that off the table. These changes are still desired for bug 213945 (allowing a better vertical view configuration by having multi-line tree items through the use of descriptions) and bug 364090 (by allowing the preview to be styled differently and also be on its own line). So I would say those are the use-cases to target, and we would still very much like this excellent functionality available in Thunderbird.
Component: XP Toolkit/Widgets: Trees → XUL
QA Contact: xptoolkit.trees → xptoolkit.widgets
Paul, did you want someone to review this?
(In reply to comment #29) > Paul, did you want someone to review this? Yes, for a feedback. Who should I ask to ?
Comment on attachment 329007 [details] [diff] [review] updated patch: description box supports "display: none". Should work on Windows and Mac (not tested) Neil's a good person to ask for review here, I think. Doing so. :-)
Attachment #329007 - Flags: review?(enndeakin)
Comment on attachment 329007 [details] [diff] [review] updated patch: description box supports "display: none". Should work on Windows and Mac (not tested) +[scriptable, uuid(3c2a2d26-08d6-415b-a635-24f15983bde6)] +interface nsIMultilineTreeView: nsITreeView ... + long getDescriptionTextPartCount(in long row); + AString getDescriptionTextPart(in long row, in long partIdx); + void getDescriptionTextPartProperties(in long row, in long partIdx, in nsISupportsArray properties); I wonder if we should use nsIArray here. Not a big deal for now. Add documentation comments for this interface and methods. + boolean isMultiline(); +}; + + Remove extra blank lines // We don't want to consider any of the decorations that may be present // on the current row, so we have to deflate the rect by the border and // padding and offset its left and top coordinates appropriately. + AdjustForBorderPadding(rowContext, cellRect); + NS_NAMED_LITERAL_CSTRING(cell, "cell"); nsStyleContext* cellContext = GetPseudoStyleContext(nsCSSAnonBoxes::moztreecell); - - NS_NAMED_LITERAL_CSTRING(cell, "cell"); This change doesn't seem necessary. if (currCol->IsCycler() || cell.Equals(aElement)) { // If the current Column is a Cycler, then the Rect is just the cell - the margins. // Similarly, if we're just being asked for the cell rect, provide it. theRect = cellRect; nsMargin cellMargin; cellContext->GetStyleMargin()->GetMargin(cellMargin); theRect.Deflate(cellMargin); @@ -1933,16 +1942,21 @@ nsTreeBodyFrame::PrefillPropertyArray(PR if (mFocused) mScratchArray->AppendElement(nsGkAtoms::focus); // sort PRBool sorted = PR_FALSE; mView->IsSorted(&sorted); if (sorted) mScratchArray->AppendElement(nsGkAtoms::sorted); + + // multiline + if (mIsMultiline) + mScratchArray->AppendElement(nsGkAtoms::multiline); + remove extra blank line +PRInt32 nsTreeBodyFrame::GetCellHeight(PRInt32 aRow, nsITreeColumn* aColumn) { ... + if (cellHeight < cellMinHeight) + cellHeight = cellMinHeight; cellHeight = PR_MIN(cellHeight, cellMinHeight); + nscoord height; + // As good a default as any. + if (mIsMultiline) { + height = 36; + } else { + height = 18; + } + return nsPresContext::CSSPixelsToAppUnits(height); +} + Why not: return nsPresContext::CSSPixelsToAppUnits(mIsMultiline ? 36 : 18); Also, remove the extra blank line. + PRInt32 xOffSet = 0; + nsTreeColumn* primaryCol = mColumns->GetPrimaryColumn(); + if (primaryCol) { + PRInt32 cellX, cellY, cellWidth, cellHeight; + GetCoordsForCellItem(aRowIndex, primaryCol, NS_LITERAL_CSTRING("image"), + &cellX, &cellY, &cellWidth, &cellHeight); + xOffSet = nsPresContext::CSSPixelsToAppUnits(cellX); Hmmm. This seems an unoptimal way of calculating just the x coordinate. Not sure if this could be improved. // Now paint our element, but only if we aren't a cycler column. // XXX until we have the ability to load images, allow the view to // insert text into cycler columns... + + Remove extra blank lines } + + +void +nsTreeBodyFrame::PaintDescription(PRInt32 aRowIndex, Remove extra blank line + const nsRect& aDescRect, + nsPresContext* aPresContext, + nsIRenderingContext& aRenderingContext, + const nsRect& aDirtyRect, + nsPoint aPt) aPt is never used, so remove it. + // Adjust the rect for its border and padding. + nsMargin borderPadding(0, 0, 0, 0); + GetBorderPadding(descContext, borderPadding); + + AdjustForBorderPadding(descContext, descRect); borderPadding isn't used. I think you should remove those two lines. + nscoord currX = descRect.x; + nscoord remainingWidth = descRect.width; + + nsRect elementRect(currX, descRect.y, remainingWidth, descRect.height); elementRect will end up being equal to descRect. Just use descRect directly. +} + + +void +nsTreeBodyFrame::PaintMultilineText(nsAutoString& aText, Remove extra blank line. + // Obtain the margins for the text and then deflate our rect by that + // amount. The text is assumed to be contained within the deflated rect. + nsRect textRect(aTextRect); The comment doesn't apply here. + + // ------ ? + tmpText.Left(textToDraw, i); + textToDraw.Append(kEllipsis); + tmpText.Right(tmpText, tmpText.Length() - i); No need to adjust tmpText right? It's the last line, so we should empty it. + } + } else { + nscoord cwidth; + nscoord twidth = 0; + int length = tmpText.Length(); + int i; + for (i = 0; i < length; ++i) { + PRUnichar ch = tmpText[i]; + // XXX this is horrible and doesn't handle clusters + aRenderingContext.GetWidth(ch,cwidth); + if (twidth + cwidth > width) + break; + twidth += cwidth; + } + tmpText.Left(textToDraw, i); + tmpText.Right(tmpText, tmpText.Length() - i); Just 'length' can be used instead of 'tmpText.Length()' + + void nsTreeBodyFrame::PaintTwisty(PRInt32 aRowIndex, Remove extra blank lines. There are also a number of places where you've included extra blank lines such as this in the headers and css files. Remove those. + PRBool mIsMultiline; This should be intialized in the constructor as well just to be safe. Is there no way to share more code between PaintMultilineText and PaintText or AdjustForCellText? Maybe some extra functions perhaps? Also, I notice that text doesn't word-wrap. That doesn't sound particularly as useful. Maybe ask roc if there is a convenient text drawing api available that takes a string(s) and wraps and paints them into a bounding box. In the example you provided, on Mac, the description text doesn't repaint properly when the window is resized smaller and then larger again. The actual feature looks great though. For accessibility, we can assume that getCellText() should return all of the text concatenated.
Attachment #329007 - Flags: review?(enndeakin) → review-
> Maybe ask roc if there is a convenient text drawing api available that > takes a string(s) and wraps and paints them into a bounding box. There isn't. Watch out for bidi! nsCanvasRenderingContext2D has some reasonably smart text drawing you might be able to get ideas from.
(In reply to comment #32) > The actual feature looks great though. For accessibility, we can assume that > getCellText() should return all of the text concatenated. That's the way for unaccessible content. But since we need to expose style changes to AT as well then we should depend on new interface. Once we get this patch landed I'll put the patch for a11y issue.
Flags: wanted1.9.1? → wanted1.9.1+
This patch seems to have stalled a bit - we would love to be able to use this in TB 3.0...
Blocks: 414038
No longer blocks: 414038
I'm looking for some help about word wrapping and cropping mechanism : I have a set of text part. I have the largest box which will host the text. I need to compute the text bounding box for a given text. There are two context where I need to know the bounding box : * to know which item is at a given coordinate (function GetItemWithinCellAt) * where to draw the text : needed *before* drawing the text since we first have to know the height of the bounding box for the center vertical align. So I can't wrap and crop the text only during the draw process. Two choice : * wrapping and cropping two times (compute the bounding box and draw the text) -> perf issues * wrapping and cropping one time, but we have to "save" the result of a such computation -> dynamic allocation -> perf issues A solution could be : * no vertical align : start at the top of the cell/description box (so crop and wrap during the draw process) * For GetItemWithingCellAt, base the result on the cell box and not on the text bounding box (no wrap/crop process needed). Rough result. Comments ?
I don't quite understand. All these text parts are on the same line?
No. According to the size of the complete text and the size of the cell or description box, these parts would be drawn on several lines. Please see attachment 327116 [details] : The description box of the 4th row (the blue box under the "syncCVS-1.8.sh" cell) is wrapped and cropped. There are 3 part (a green one, a red one and a grey one).
What I mean is, the different text items always place horizontally (like a XUL hbox), not stacking vertically (like a XUL vbox)?
What we actually want here is to have a bounding box (which we know the size of beforehand, right?) and render a set of strings into it, with wrapping.
That's hugely painful if you want to handle mixed-direction text.
(In reply to comment #41) > That's hugely painful if you want to handle mixed-direction text. Why? The implementation is complex? What complexity arises that doesn't for say, a normal block with some inline text strings in it?
None. A normal block with inline text strings is really complicated, if you want to handle mixed-direction bidi text, and especially if you want text runs to be given different styles.
You might want to look at nsCanvasRenderingContext2D for some clues about displaying mixed-direction text; it also handles measurement, although it doesn't actually handle line-breaking.
Neil, we know the size of the cell (or description) box, but we don't know the text bounding box : After deflating (margin border and padding), we have a box where the text will be paint. This is not the final text bounding box. Beforehand, we don't know the number of lines needed for drawing the text and the width of the largest line which will be paint. In order to know the number of lines and the width of the largest line, we need to collect all text parts and then apply the word-wrap/crop mechanism. Once we know that, we know the height and the width of the bounding box. Then, we can draw the text at the correct x/y position (according to the align properties). All those last operations will be needed again for drawing the text. So I'm looking for a way to make such mechanism more efficient. See my current solution in comment 36.
If you gather all the strings, do the bidi splitting, then generate a list of textruns, you've done most of the work. Measuring and painting a textrun after you've created it is really cheap.
This bug would also finally make Thunderbird's "Vertical View" usable!? +------------------------------------------------------------------------------+ | All Folders | Long Subject #1 of an e-mail that is so | This is the body of | | Account1 | long that it takes up two lines (bold!) | the selected e-mail.| | - Inbox | From: Name1@domain1.com Date: dd.mm.yyyy | Bla bla bla. Bla bla| | - Mail |------------------------------------------| bla bla bla. | | - IT | Long Subject #2 of an e-mail that is so | Bla bla bla. Bla bla| | - Politic| long that it takes up two lines (bold!) | Bla bla bla. Bla | | - Persona| From: Name2@domain2.com Date: dd.mm.yyyy | Bla bla bla. Bla bla| | - Work |------------------------------------------| Bla bla bla. Bla bla| | | Long Subject #3 of an e-mail that is so | Bla bla bla. | | | long that it takes up two lines (bold!) | | | | From: Name3@domain3.com Date: dd.mm.yyyy | | +------------------------------------------------------------------------------+
Sorry about the broken ASCII art (was too wide by one column). :-[ +----------------------------------------------------------------------------+ | | Long Subject #1 of an e-mail that is so | This is the body of| | AccountName | long that it takes up two lines (bold!) | the selected e-mail| | -*Inbox* | From: Name1@domain1.com Date: dd.mm.yyyy| Bla bla bla. Bla | | - Mail |-----------------------------------------| bla bla bla. | | - IT | *Long Subject #2 of an e-mail that is* | Bla bla bla. Bla | | - Politics| *long. It takes up two lines (bold!)* | Bla bla bla. Bla | | - Personal| *From: Name@domain.com Date: dd.mm.yy* | Bla bla bla. Bla | | - Work |-----------------------------------------| Bla bla bla. Bla | | | Long Subject #3 of an e-mail that is so | Bla bla. | | | long that it takes up two lines (bold!) | | | | From: Name3@domain3.com Date: dd.mm.yyyy| | +----------------------------------------------------------------------------+
(In reply to comment #32) > (From update of attachment 329007 [details] [diff] [review]) > > +[scriptable, uuid(3c2a2d26-08d6-415b-a635-24f15983bde6)] > +interface nsIMultilineTreeView: nsITreeView > ... > + long getDescriptionTextPartCount(in long row); > + AString getDescriptionTextPart(in long row, in long partIdx); > + void getDescriptionTextPartProperties(in long row, in long partIdx, in > nsISupportsArray properties); > > I wonder if we should use nsIArray here. Not a big deal for now. Add > documentation comments for this interface and methods. Currently, all methods in nsITreeView use nsISupportsArray. So is it better to use nsISupportsArray and use another bug to use nsISupportsArray in nsITreeView/nsIMultilineTreeView ? > > + PRInt32 xOffSet = 0; > + nsTreeColumn* primaryCol = mColumns->GetPrimaryColumn(); > + if (primaryCol) { > + PRInt32 cellX, cellY, cellWidth, cellHeight; > + GetCoordsForCellItem(aRowIndex, primaryCol, > NS_LITERAL_CSTRING("image"), > + &cellX, &cellY, &cellWidth, &cellHeight); > + xOffSet = nsPresContext::CSSPixelsToAppUnits(cellX); > > Hmmm. This seems an unoptimal way of calculating just the x coordinate. Not > sure if this could be improved. I'm not sure we really want to use x coordinate as offset. For example, presentation would be very strang if primaryCol is second column of the tree. What about just computing padding, margin, indentation and moztreetwisty for primary row ? I'll attach a patch that uses this solution, and fixes most of the problem you point in your comment
currently attached example (xulrunner app) works only with very first version of the patch. As nsIMultiTreeView api has changed in paul's second patch, here is an update xulrunner app.
Attachment #328913 - Attachment is obsolete: true
I tried to work on the problem reported in comment 36 but did not have a lot of success. In nsTreeBodyFrame::PaintDescription, once I have the description text, I create a textRun with following code: const nsStyleFont* fontData = textContext->GetStyleFont(); const nsFont& font = fontData->mFont; nsCAutoString langGroup; nsIAtom *langGroupAtom = aPresContext->GetLangGroup(); if (langGroupAtom) { const char* lg; langGroupAtom->GetUTF8String(&lg); langGroup.Assign(lg); } gfxFontStyle fontStyle(font.style, font.weight, font.stretch, font.size, langGroup, font.sizeAdjust, font.systemFont, font.familyNameQuirks, PR_FALSE); nsRefPtr<gfxFontGroup> fontGroup = gfxPlatform::GetPlatform()->CreateFontGroup(font.name, &fontStyle, PresContext()->GetUserFontSet()); PRUint32 flags = gfxTextRunFactory::TEXT_NEED_BOUNDING_BOX | nsLayoutUtils::GetTextRunFlagsForStyle(textContext, GetStyleText(), fontData); nsRefPtr<gfxContext> ctx = aRenderingContext.ThebesContext(); gfxTextRunFactory::Parameters params = { ctx, nsnull, nsnull, nsnull, 0, nsIDeviceContext::AppUnitsPerCSSPixel() }; gfxTextRun* textRun = gfxTextRunCache::MakeTextRun(text.get(), text.Length(), fontGroup, &params, flags); Then, instead of calling PaintMultilineText, I try to display it with gfxRect dirtyGfxRect ( gfxFloat(aDirtyRect.x) / aPresContext->DevPixelsToAppUnits(1), gfxFloat(aDirtyRect.y) / aPresContext->DevPixelsToAppUnits(1), gfxFloat(aDirtyRect.width) / aPresContext->DevPixelsToAppUnits(1), gfxFloat(aDirtyRect.height) / aPresContext->DevPixelsToAppUnits(1) ); gfxPoint startPoint(NSAppUnitsToFloatPixels(aDirtyRect.x, PresContext()->AppUnitsPerDevPixel()), NSAppUnitsToFloatPixels(aDirtyRect.y, PresContext()->AppUnitsPerDevPixel())); textRun->Draw(ctx.get(), startPoint, 0, textRun->GetLength(), &dirtyGfxRect, nsnull, nsnull); but it does not work. Do you have any clue to make this work ?
Blocks: 468835
ctx, nsnull, nsnull, nsnull, 0, nsIDeviceContext::AppUnitsPerCSSPixel() That should be AppUnitsPerDevPixel, but that won't fix your problem... I think your problem is that the startPoint should be the baseline of the text --- i.e. the bottom of the letter, but you're setting it to the top-left of the dirty rect, so only the descenders (e.g. the tail of 'g') will be visible in the dirtyrect...
So, I know I'm probably going to regret asking this, but other than the stuff in comment 36, what remains to be cone here? I can probably work on this (with some guidance), but I'm not sure if this is "mostly done" or "barely started".
The main issue is supporting wrapped text. This work has been described above as "hugely painful" and "really complicated".
nsCanvasRenderingContext2D was mentioned above as something that does what needs to be done here. Is this function what I should be looking at? http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/nsCanvasRenderingContext2D.cpp#2606 I'm assuming (perhaps naively) here that while this is painful to do, it's already been done elsewhere (right?), so I can just take advantage of other people's pain in getting this working.
(In reply to Jim Porter (:squib) from comment #56) > nsCanvasRenderingContext2D was mentioned above as something that does what > needs to be done here. Is this function what I should be looking at? > http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/ > nsCanvasRenderingContext2D.cpp#2606 > > I'm assuming (perhaps naively) here that while this is painful to do, it's > already been done elsewhere (right?), so I can just take advantage of other > people's pain in getting this working. Hi Jim, Any luck on this?
Assignee: paul → nobody
In an attempt to give a little incentive to anyone capable of solving this, I added this bug to FossFactory.org and donated $100 to it.
Oh and a link to the project of course: http://www.fossfactory.org/project/p294
Good idea, Jakob. I added $50 (actually $48.25 after deducing PayPal theft) to the sponsorship.
I put another $5, making this bug worth $165.
i put some 9,62€ in, perhaps we could get this long awaited message view in Thunderbird with the help of some money ;-)
The lack of 2-line headers in vertical view is what finally drove me to the paid Postbox version of Thunderbird (which has a decent implementation of this feature, as well as a polished version of the Conversations plugin). Perhaps I should have spent the money at FossFactory to get it into Thunderbird proper?
Added $50 to the Fossfactory fund. Come on you guys, join in if you haven't already. Even $5 or $10 will help.
Looks like the reward is already at US$372. But looks like a hard problem.
Status: ASSIGNED → NEW
Whiteboard: [bounty]
Version: unspecified → Trunk
Will a richlist do the trick, instead of coding a new structure?
(In reply to Alfred Kayser from comment #66) > Will a richlist do the trick, instead of coding a new structure? That all depends on how much you want to support. Even just pure HTML could "do the trick" if you limit the functionality in some ways. However, supporting customization is the hard part.
Also we have to keep in mind that: - what makes Thunderbird good is the fact that scrolling the message list is really fast and efficient, - and that's because we're implementing a custom tree view from C++. Moving away from a XUL tree would probably require significant rework of our code (I think).
I don't mean to be a jerk, but much of the discussion in this bug seems to be misplaced. From what I can see, there are two things at play here: - This is a bug with a low-level focus: adding to nsITreeView's bag of tricks - There are people with a higher-level interest: the mail thread pane should offer a view like outlook, gmail, et cetera. We have bug 213945 and bug 364090 for this (one of which may need to be duped against the other). Whether the second thing happens as a direct result of the first thing isn't really the focus of those with the second interest. It's possible that someone will manage do the second thing using a rich list, HTML, et cetera. But that's the concern of bug 213945 or bug 364090, not this one.
PostBox has made enhancements to TreeView that enable the view that folks want for Thunderbird bug 213945. That code is available due to MPL. See https://bugzilla.mozilla.org/show_bug.cgi?id=213945#c107.
How did PostBox enhancements to TreeView?
(In reply to Yonggang Luo from comment #73) > How did PostBox enhancements to TreeView? Apparently, they used the patch attached to this bug.
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #74) > (In reply to Yonggang Luo from comment #73) > > How did PostBox enhancements to TreeView? > > Apparently, they used the patch attached to this bug. How close would you say the patch here is to being completed? If it's nearly done, I could probably work on it.
I am confused, if I use the patch, did this support for vbox/hbox in treecell?
Does anybody know which version of Mozilla that PostBox are based on? So I can do easier merge.
The patch in the bug adds support for multiline text. Getting text to wrap properly is difficult and is not done by this patch. Also, this patch is very old.
I know, but I am working on this issue now, based on mozilla 31, so I need all kinds of suggestion to apply this patch to getting treecell to support multiline text, or even more, support for vbox/hbox in trecell, but I need help, at the first stage, I will getting mozilla 31 to support for multiline, that means apply this patch to mozilla 31.
(In reply to Yonggang Luo from comment #77) > Does anybody know which version of Mozilla that PostBox are based on? I think Gecko 7.0.1 (yes, anchient, thunderbird 7 something)
(In reply to Yonggang Luo from comment #79) > I know, but I am working on this issue now, based on mozilla 31, so I need > all kinds of suggestion to apply this patch to getting treecell to support > multiline text, or even more, support for vbox/hbox in trecell, but I need > help, at the first stage What kind of help do you need? You might want to start with https://developer.mozilla.org/en-US/docs/Introduction and https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide - for general help (not directly related to the multiline implementation), IRC or mailing lists are better than this bug. Try to rebase attachment 428204 [details] [diff] [review] first (on top of mozilla central, not Firefox 31).
(In reply to Magnus Melin from comment #80) > (In reply to Yonggang Luo from comment #77) > > Does anybody know which version of Mozilla that PostBox are based on? > > I think Gecko 7.0.1 (yes, anchient, thunderbird 7 something) Thanks a lot, I am rebasing it now.
(In reply to Yonggang Luo from comment #82) > (In reply to Magnus Melin from comment #80) > > (In reply to Yonggang Luo from comment #77) > > > Does anybody know which version of Mozilla that PostBox are based on? > > > > I think Gecko 7.0.1 (yes, anchient, thunderbird 7 something) > Thanks a lot, I am rebasing it now. I think it's more recent than version 7. Matt has some insight into their use of Thunderbird code. Their code is at http://www.postbox-inc.com/coveredcode
Flags: needinfo?(unicorn.consulting)
(In reply to Wayne Mery (:wsmwk) from comment #83) > (In reply to Yonggang Luo from comment #82) > > (In reply to Magnus Melin from comment #80) > > > (In reply to Yonggang Luo from comment #77) > > > > Does anybody know which version of Mozilla that PostBox are based on? > > > > > > I think Gecko 7.0.1 (yes, anchient, thunderbird 7 something) > > Thanks a lot, I am rebasing it now. > > I think it's more recent than version 7. Matt has some insight into their > use of Thunderbird code. > > Their code is at http://www.postbox-inc.com/coveredcode I downloaded the code, though he hide the thunderbird version number, I found that his useragent is 7.0.1, so I think it is based on thunderbird 7.0.1
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #81) > (In reply to Yonggang Luo from comment #79) > > I know, but I am working on this issue now, based on mozilla 31, so I need > > all kinds of suggestion to apply this patch to getting treecell to support > > multiline text, or even more, support for vbox/hbox in trecell, but I need > > help, at the first stage > > What kind of help do you need? You might want to start with > https://developer.mozilla.org/en-US/docs/Introduction and > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide - for > general help (not directly related to the multiline implementation), IRC or > mailing lists are better than this bug. > > Try to rebase attachment 428204 [details] [diff] [review] first (on top of > mozilla central, not Firefox 31). I am looking for incremental building instructions (for testing) and something help me to running tests, I need to verify my code, I will try to base on Firefox 31 first because it's stable, and then I will rebase it on mozilla central.
Flags: needinfo?(unicorn.consulting)
It's seems the multiline already been committed in nsGkAtomList.h How to deal with this issue?
I removed the orignal "multiline" defines, don't know if that is a good idea. content/base/src/nsGkAtomList.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/content/base/src/nsGkAtomList.h b/content/base/src/nsGkAtomList.h index a6cf9fb..5bc3512 100644 --- a/content/base/src/nsGkAtomList.h +++ b/content/base/src/nsGkAtomList.h @@ -2191,7 +2191,7 @@ GK_ATOM(marginTop, "margin-top") GK_ATOM(menuitemcheckbox, "menuitemcheckbox") GK_ATOM(menuitemradio, "menuitemradio") GK_ATOM(mixed, "mixed") -GK_ATOM(multiline, "multiline") +//GK_ATOM(multiline, "multiline") GK_ATOM(password, "password") GK_ATOM(posinset, "posinset") GK_ATOM(presentation, "presentation")
(In reply to Yonggang Luo from comment #86) > It's seems the multiline already been committed in nsGkAtomList.h > How to deal with this issue? It's changed in bug 481395 nsAccessibilityAtoms->nsGkAtoms
(In reply to xunxun from comment #88) > (In reply to Yonggang Luo from comment #86) > > It's seems the multiline already been committed in nsGkAtomList.h > > How to deal with this issue? > > It's changed in bug 481395 > > nsAccessibilityAtoms->nsGkAtoms Thanks for your quick response, and I am not using nsAccessibilityAtoms, just nsGkAtom, but the problem is I am using a existed nsGkAtom multiline that conflict with Paul Rouget's patch I don't know if I can still use this GkAtom
What's the replacement in mozilla 31 for nsISupportsArray It's already removed in xul/tree, now it's use AtomArray instead of nsISupportsArray*
I am trying to use GK_ATOM(multiline, "multiline") defined in nsGkAtomList.h, but it under the macro #ifdef ACCESSIBILITY, may I move it to the global place?
Attached patch multiline-tree.patch (obsolete) — Splinter Review
This is the updated multiline patch that support for mozilla 31esr, should be easy to cherry-pick to master. Need reviews
Attachment #8472801 - Flags: approval-mozilla-esr31?
There's no way this will land on 31 ESR. That's a stable branch primarily for security fixes; they are definitely not taking new features.
(In reply to Alex Vincent [:WeirdAl] from comment #93) > There's no way this will land on 31 ESR. That's a stable branch primarily > for security fixes; they are definitely not taking new features. I am not intent to land on 31 ESR. but base 31 ESR to implement this feature, can you do a review for me, after that, I will cherry-pick it to master
Comment on attachment 8472801 [details] [diff] [review] multiline-tree.patch Yonggang, if you want someone to review your patch, please use review and select a developer. approval-mozilla-esr31 is to get a patch cherry-picked from m-c to esr31 You should develop directly on mozilla-central: * 31 code can be very different from the one in m-c (34) * it will be harder to the reviewer * it is not the correct way to write patch
Attachment #8472801 - Flags: approval-mozilla-esr31?
(In reply to Sylvestre Ledru [:sylvestre] from comment #95) > Comment on attachment 8472801 [details] [diff] [review] > multiline-tree.patch > > Yonggang, if you want someone to review your patch, please use review and > select a developer. approval-mozilla-esr31 is to get a patch cherry-picked > from m-c to esr31 > > You should develop directly on mozilla-central: > * 31 code can be very different from the one in m-c (34) > * it will be harder to the reviewer > * it is not the correct way to write patch I know what's your concern, I want to apply this patch to my own application that use XUL runner, so I need some stable version of mozilla, at the current time mozilla 31 is a relative new and stable version, so I won't miss the opportunity to merge this patch to mainline, at the current time, I need someone to review my patch, and teach me how to make it product quality, such as writing testcase for it and minor fixes. After the functional and tests are ready, then I will submit the last patch that rebased on mainline, Thanks for your detail explain. I want to specify ` Paul Rouget` as the reviewer, because he is the original patch creator, but I don't know how.
Yonggang, if you need help understanding the (sometimes scary !) bugzilla, there's a nice video at http://blog.johnath.com/2010/02/04/bugzilla-for-humans/ Cheers, ~ jonathan
(In reply to Yonggang Luo from comment #96) > I want to specify ` Paul Rouget` as the reviewer, because he is the > original patch creator, but I don't know how. I can't review this patch (I'm not a peer of this module). But first, you need to rebase the patch on top of central (not 31), and make sure text properly wraps (iirc, that's why this patch never got finished).
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #98) > (In reply to Yonggang Luo from comment #96) > > I want to specify ` Paul Rouget` as the reviewer, because he is the > > original patch creator, but I don't know how. > > I can't review this patch (I'm not a peer of this module). > > But first, you need to rebase the patch on top of central (not 31), and make > sure text properly wraps (iirc, that's why this patch never got finished). Text properly wraps? I would like you explain it a bit more. Rebase on top of central is OK,
About RichTree, I have two question, 1. How to setting different row with different height? I means by using callback like getRowProperties, we turn out have something like getRowHeight() 2. How to turn a cell draw to be a canvas, so it can be draw outside, for this, for example drawCell(view, row,column, mouseStatus, keybordStatus) The view means TreeView, mouseStatus for the customize draw to respond mouse event such as onClick, onHover, onFocus, onUnfocus for the specific cell. If we have these two hook, we can draw anything we want.
(In reply to Yonggang Luo from comment #101) > About RichTree, I have two question, > 1. How to setting different row with different height? I means by using > callback like getRowProperties, we turn out have something like > getRowHeight() For this, we can through css to specify different row height for different rows have different properties. > 2. How to turn a cell draw to be a canvas, so it can be draw outside, for > this, > for example > drawCell(view, row,column, mouseStatus, keybordStatus) > The view means TreeView, > mouseStatus for the customize draw to respond mouse event such as > onClick, onHover, onFocus, onUnfocus for the specific cell. > > If we have these two hook, we can draw anything we want. The canvas is the only thing we need to provide.
(In reply to Yonggang Luo from comment #101) > About RichTree, I have two question, > 1. How to setting different row with different height? I means by using > callback like getRowProperties, we turn out have something like > getRowHeight() This is not possible. A tree is designed to be able to hold a large amount of items. And to do so, one of the requirement is for all the items to have the same geometry.
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #103) > (In reply to Yonggang Luo from comment #101) > > About RichTree, I have two question, > > 1. How to setting different row with different height? I means by using > > callback like getRowProperties, we turn out have something like > > getRowHeight() > > This is not possible. A tree is designed to be able to hold a large amount Yes, I agreed, > of items. And to do so, one of the requirement is for all the items to have > the same geometry. Agreed too, Now my solution works fine, I write the following function to rendering the image background, and works fine for me: //AString render(in long row, in nsITreeColumn col, in long x, in long y, in long width, in long height, in nsIMutableArray props); render: function(row, col, x, y, width, height, props) { x = x / 60; y = y / 60; width = width / 60; height = height / 60; if (!cellRenderCanvas) { cellRenderCanvas = window.document.createElementNS("http://www.w3.org/1999/xhtml", "canvas"); } cellRenderCanvas.width = width; cellRenderCanvas.height = height; var ctx = cellRenderCanvas.getContext("2d"); ctx.clearRect(0, 0, cellRenderCanvas.width, cellRenderCanvas.height); var bubbleTextStyle = "bold 11px Segoe UI"; ctx.font = bubbleTextStyle; var txt = "Hello " + row.toString() + " 123 World!" + row.toString() ctx.fillText(txt,0,0); ctx.fillText(txt,0,30); return cellRenderCanvas.toDataURL("image/png"); } The only problem is that I am using cellRenderCanvas.toDataURL("image/png"); to passing the render result as a image and passed to PaintCell to PaintImage mIsRichCell = aColumn->GetType() == nsITreeColumn::TYPE_RICH || mScratchArray.Contains(nsGkAtoms::rich); if (mIsRichCell) { nsCOMPtr<nsIMutableArray> scratchArray = AtomArrayToMutableArray(mScratchArray); nsCOMPtr<nsITreeCellCanvas> cellCanvas; GetCellCanvas(getter_AddRefs(cellCanvas)); mRichCellImageSrc = EmptyString(); if (cellCanvas) { cellCanvas->Render(aRowIndex, aColumn, cellRect.x, cellRect.y, cellRect.width, cellRect.height, scratchArray, mRichCellImageSrc); } PaintImage(aRowIndex, aColumn, cellRect, aPresContext, aRenderingContext, nsCSSAnonBoxes::moztreeimage, aDirtyRect, remainingWidth, currX); aCurrX = currX; return; } So I am looking for faster way to handling this, I means no need the image wrapper. and the patch is truely simple, also, I need some suggestion about the event handling, because I want to implement buttons in canvas to respond to mouse events.
Now I am happy to announce this issue has a big progress, ``` 22035320a5ec86fa7639836cd41a88278bdc4c0d content/base/src/nsGkAtomList.h | 1 + layout/xul/tree/nsITreeBoxObject.idl | 18 ++++++++++ layout/xul/tree/nsITreeColumns.idl | 1 + layout/xul/tree/nsTreeBodyFrame.cpp | 66 ++++++++++++++++++++++++++++++++++-- layout/xul/tree/nsTreeBodyFrame.h | 5 +++ layout/xul/tree/nsTreeBoxObject.cpp | 19 +++++++++++ layout/xul/tree/nsTreeColumns.cpp | 3 +- 7 files changed, 109 insertions(+), 4 deletions(-) diff --git a/content/base/src/nsGkAtomList.h b/content/base/src/nsGkAtomList.h index 3b633f5..d8523f3 100644 --- a/content/base/src/nsGkAtomList.h +++ b/content/base/src/nsGkAtomList.h @@ -941,6 +941,7 @@ GK_ATOM(resultPrefix, "result-prefix") GK_ATOM(rev, "rev") GK_ATOM(reverse, "reverse") GK_ATOM(reversed, "reversed") +GK_ATOM(rich, "rich") GK_ATOM(richlistbox, "richlistbox") GK_ATOM(richlistitem, "richlistitem") GK_ATOM(right, "right") diff --git a/layout/xul/tree/nsITreeBoxObject.idl b/layout/xul/tree/nsITreeBoxObject.idl index 7832c38..70fc0cc 100644 --- a/layout/xul/tree/nsITreeBoxObject.idl +++ b/layout/xul/tree/nsITreeBoxObject.idl @@ -4,14 +4,26 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ #include "nsISupports.idl" +#include "nsIDOMEventListener.idl" interface nsIDOMElement; interface nsITreeView; interface nsITreeSelection; interface nsITreeColumn; interface nsITreeColumns; +interface nsIMutableArray; interface nsIScriptableRegion; +[scriptable, uuid(ACEED5F8-E57A-4CDC-B859-CF864904AB1C)] +interface nsITreeCellCanvas : nsIDOMEventListener +{ + /** + * To rendering a TreeCell as a canvas, we return the rendering result + * as a image, that can be used by PaintImage + */ + AString render(in nsITreeView view, in long row, in nsITreeColumn col, in long x, in long y, in long width, in long height, in nsIMutableArray props); +}; + [scriptable, uuid(64BA5199-C4F4-4498-BBDC-F8E4C369086C)] interface nsITreeBoxObject : nsISupports { @@ -28,6 +40,12 @@ interface nsITreeBoxObject : nsISupports attribute nsITreeView view; /** + * The canvas handler for rich cell, to rendering anything that canvs + * support, we can use this to rendering beatiful tree cell. + */ + attribute nsITreeCellCanvas cellCanvas; + + /** * Whether or not we are currently focused. */ attribute boolean focused; diff --git a/layout/xul/tree/nsITreeColumns.idl b/layout/xul/tree/nsITreeColumns.idl index 9b27ce2..cc7f159 100644 --- a/layout/xul/tree/nsITreeColumns.idl +++ b/layout/xul/tree/nsITreeColumns.idl @@ -32,6 +32,7 @@ interface nsITreeColumn : nsISupports const short TYPE_TEXT = 1; const short TYPE_CHECKBOX = 2; const short TYPE_PROGRESSMETER = 3; + const short TYPE_RICH = 4; readonly attribute short type; nsITreeColumn getNext(); diff --git a/layout/xul/tree/nsTreeBodyFrame.cpp b/layout/xul/tree/nsTreeBodyFrame.cpp index 7f33ed2..add6fad 100644 --- a/layout/xul/tree/nsTreeBodyFrame.cpp +++ b/layout/xul/tree/nsTreeBodyFrame.cpp @@ -61,6 +61,7 @@ #include "nsIScriptableRegion.h" #include <algorithm> #include "ScrollbarActivity.h" +#include <nsIMutableArray.h> #ifdef ACCESSIBILITY #include "nsAccessibilityService.h" @@ -88,6 +89,16 @@ CancelImageRequest(const nsAString& aKey, return PL_DHASH_NEXT; } +static nsCOMPtr<nsIMutableArray> AtomArrayToMutableArray(AtomArray &atomArray) +{ + nsCOMPtr<nsIMutableArray> scratchArray = do_CreateInstance(NS_ARRAY_CONTRACTID); + NS_ENSURE_TRUE(scratchArray, nullptr); + for (uint32_t i = 0; i < atomArray.Length(); ++i) { + scratchArray->AppendElement(atomArray[i], true); + } + return scratchArray; +} + // // NS_NewTreeFrame // @@ -129,7 +140,8 @@ nsTreeBodyFrame::nsTreeBodyFrame(nsIPresShell* aPresShell, nsStyleContext* aCont mVerticalOverflow(false), mHorizontalOverflow(false), mReflowCallbackPosted(false), - mCheckingOverflow(false) + mCheckingOverflow(false), + mIsRichCell(false) { mColumns = new nsTreeColumns(this); } @@ -2109,7 +2121,13 @@ nsTreeBodyFrame::GetImage(int32_t aRowIndex, nsTreeColumn* aCol, bool aUseContex *aResult = nullptr; nsAutoString imageSrc; - mView->GetImageSrc(aRowIndex, aCol, imageSrc); + if (mIsRichCell) { + imageSrc = mRichCellImageSrc; + if (!imageSrc.IsEmpty()) + aUseContext = false; + } else { + mView->GetImageSrc(aRowIndex, aCol, imageSrc); + } nsRefPtr<imgRequestProxy> styleRequest; if (!aUseContext && !imageSrc.IsEmpty()) { aAllowImageRegions = false; @@ -3268,7 +3286,11 @@ nsTreeBodyFrame::PaintCell(int32_t aRowIndex, nsRect elementRect(currX, cellRect.y, remainingWidth, cellRect.height); nsRect dirtyRect; if (dirtyRect.IntersectRect(aDirtyRect, elementRect)) { - switch (aColumn->GetType()) { + int16_t cellType = aColumn->GetType(); + if (mScratchArray.Contains(nsGkAtoms::rich)) { + cellType = nsITreeColumn::TYPE_RICH; + } + switch (cellType) { case nsITreeColumn::TYPE_TEXT: PaintText(aRowIndex, aColumn, elementRect, aPresContext, aRenderingContext, aDirtyRect, currX); break; @@ -3289,6 +3311,26 @@ nsTreeBodyFrame::PaintCell(int32_t aRowIndex, break; } break; + case nsITreeColumn::TYPE_RICH: + nsCOMPtr<nsITreeCellCanvas> cellCanvas; + GetCellCanvas(getter_AddRefs(cellCanvas)); + if (cellCanvas) { + nsCOMPtr<nsIMutableArray> scratchArray = AtomArrayToMutableArray(mScratchArray); + mRichCellImageSrc = EmptyString(); + cellCanvas->Render(mView, + aRowIndex, + aColumn, + elementRect.x, + elementRect.y, + elementRect.width, + elementRect.height, + scratchArray, + mRichCellImageSrc); + mIsRichCell = true; + PaintImage(aRowIndex, aColumn, elementRect, aPresContext, aRenderingContext, aDirtyRect, remainingWidth, currX); + mIsRichCell = false; + } + break; } } } @@ -4685,3 +4727,21 @@ nsTreeBodyFrame::OnImageIsAnimated(imgIRequest* aRequest) return NS_OK; } + +nsresult +nsTreeBodyFrame::GetCellCanvas(nsITreeCellCanvas ** aCellCanvas) +{ + *aCellCanvas = nullptr; + nsWeakFrame weakFrame(this); + NS_ENSURE_STATE(weakFrame.IsAlive()); + NS_IF_ADDREF(*aCellCanvas = mCellCanvas); + return NS_OK; +} + +nsresult +nsTreeBodyFrame::SetCellCanvas(nsITreeCellCanvas * aCellCanvas) +{ + NS_ENSURE_ARG_POINTER(aCellCanvas); + mCellCanvas = aCellCanvas; + return NS_OK; +} diff --git a/layout/xul/tree/nsTreeBodyFrame.h b/layout/xul/tree/nsTreeBodyFrame.h index fb1f8b7..4bd7b4e 100644 --- a/layout/xul/tree/nsTreeBodyFrame.h +++ b/layout/xul/tree/nsTreeBodyFrame.h @@ -119,6 +119,8 @@ public: nsresult BeginUpdateBatch(); nsresult EndUpdateBatch(); nsresult ClearStyleAndImageCaches(); + nsresult GetCellCanvas(nsITreeCellCanvas ** aTwistyImageGenerator); + nsresult SetCellCanvas(nsITreeCellCanvas * aTwistyImageGenerator); void ManageReflowCallback(const nsRect& aRect, nscoord aHorzWidth); @@ -629,6 +631,9 @@ protected: // Data Members // have pointers to us. nsTHashtable<nsPtrHashKey<nsTreeImageListener> > mCreatedListeners; + nsCOMPtr<nsITreeCellCanvas> mCellCanvas; + bool mIsRichCell; + nsAutoString mRichCellImageSrc; }; // class nsTreeBodyFrame #endif diff --git a/layout/xul/tree/nsTreeBoxObject.cpp b/layout/xul/tree/nsTreeBoxObject.cpp index 6f61cba..af2aba5 100644 --- a/layout/xul/tree/nsTreeBoxObject.cpp +++ b/layout/xul/tree/nsTreeBoxObject.cpp @@ -490,3 +490,22 @@ NS_NewTreeBoxObject(nsIBoxObject** aResult) return NS_OK; } +NS_IMETHODIMP +nsTreeBoxObject::GetCellCanvas(nsITreeCellCanvas ** aCellCanvas) +{ + nsTreeBodyFrame* body = GetTreeBody(); + if (body) + return body->GetCellCanvas(aCellCanvas); + else + *aCellCanvas = nullptr; + return NS_OK; +} + +NS_IMETHODIMP +nsTreeBoxObject::SetCellCanvas(nsITreeCellCanvas * aCellCanvas) +{ + nsTreeBodyFrame* body = GetTreeBody(); + if (body) + return body->SetCellCanvas(aCellCanvas); + return NS_OK; +} diff --git a/layout/xul/tree/nsTreeColumns.cpp b/layout/xul/tree/nsTreeColumns.cpp index 945da14..4f46fa6 100644 --- a/layout/xul/tree/nsTreeColumns.cpp +++ b/layout/xul/tree/nsTreeColumns.cpp @@ -319,11 +319,12 @@ nsTreeColumn::Invalidate() // Figure out our column type. Default type is text. mType = nsITreeColumn::TYPE_TEXT; static nsIContent::AttrValuesArray typestrings[] = - {&nsGkAtoms::checkbox, &nsGkAtoms::progressmeter, nullptr}; + {&nsGkAtoms::checkbox, &nsGkAtoms::progressmeter, &nsGkAtoms::rich, nullptr}; switch (mContent->FindAttrValueIn(kNameSpaceID_None, nsGkAtoms::type, typestrings, eCaseMatters)) { case 0: mType = nsITreeColumn::TYPE_CHECKBOX; break; case 1: mType = nsITreeColumn::TYPE_PROGRESSMETER; break; + case 2: mType = nsITreeColumn::TYPE_RICH; break; } // Fetch the crop style. ```
Attached image richTree.jpg (obsolete) —
Now it can be rendering with canvas in a proper and simple way. This is a tree demo
Attachment #8474218 - Flags: superreview?(paul)
Attachment #8474218 - Flags: review?(paul)
Attached patch richTree.patch (obsolete) — Splinter Review
This is a simple patch to add rich tree support, only add a rich atom to original tree layout
Attachment #8474219 - Flags: superreview?(paul)
Attachment #8474219 - Flags: review?(paul)
Attached file richtree.zip (obsolete) —
For testing purpose. It's have canvas rendering code in the test.js file and it support for 1000,000 entries without performance loss.
Attachment #8472801 - Attachment is obsolete: true
Attachment #8474221 - Flags: superreview?(paul)
Attachment #8474221 - Flags: review?(paul)
Comment on attachment 8474219 [details] [diff] [review] richTree.patch (In reply to Yonggang Luo from comment #107) > Created attachment 8474219 [details] [diff] [review] > richTree.patch > > This is a simple patch to add rich tree support, only add a rich atom to > original tree layout Again, I can't review this patch. I'm not a peer of this module. Also, you totally changed what the original patch was doing, and if you delegate drawing to canvas, what's the point of using a xul:tree? File a new bug if you still want to use a canvas with a xul:tree. If you want to fix this bug, improve the current patch, and address comment 45 and comment 46.
Attachment #8474219 - Flags: superreview?(paul)
Attachment #8474219 - Flags: review?(paul)
Attachment #8474218 - Flags: superreview?(paul)
Attachment #8474218 - Flags: review?(paul)
Attachment #8474221 - Flags: superreview?(paul)
Attachment #8474221 - Flags: review?(paul)
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #109) > Comment on attachment 8474219 [details] [diff] [review] > richTree.patch > > (In reply to Yonggang Luo from comment #107) > > Created attachment 8474219 [details] [diff] [review] > > richTree.patch > > > > This is a simple patch to add rich tree support, only add a rich atom to > > original tree layout > > Again, I can't review this patch. I'm not a peer of this module. So, tell me, who is the peer of this module, I will inform he/she. > > Also, you totally changed what the original patch was doing, and if you > delegate drawing to canvas, what's the point of using a xul:tree? File a new > bug if you still want to use a canvas with a xul:tree. I am not following the original patch, I am creating a new way to solve this issue, and works fine. Please beware of the bug title, `Treerows need a way to hold richer content`, that's the exactly my new patch solved this issue, hold "Richer content", canvas is contains much more content thatn 'multiline'. > > If you want to fix this bug, improve the current patch, and address comment > 45 and comment 46.
I can't comment about the use of canvas to solve this bug, but if you continue to use the approach of creating an imgIContainer from canvas data, please request review from me before checking this in.
Add mozFetchStream API in HTML Canvas interface for retrieve the canvas content as png stream synchronously.
Attachment #8474219 - Attachment is obsolete: true
Attachment #8477256 - Flags: review?(seth)
Add rich tree support by rendering a treecell as a canvas, and that canvas can be rendered in the Javascript, so in theoretically, It's will as rich as canvas can do. Text wrapping is also can be handled properly in canvas, that's won't be a problem.
Minor changes.
Attachment #8477258 - Attachment is obsolete: true
Attachment #8477440 - Flags: review?(seth)
Comment on attachment 8477256 [details] [diff] [review] 0001-Add-API-mozFetchStream-to-synchronously-fetch-the-ns.patch Review of attachment 8477256 [details] [diff] [review]: ----------------------------------------------------------------- This isn't the way to go. The method you're using the get the content of the canvas is also extremely expensive, since you're encoding it to PNG and then immediately decoding it again. You also don't need to (and shouldn't) add any methods to nsIDOMHTMLCanvasElement; stick to internal, C++-only APIs for this. If the best way to resolve this bug is to create an image from a canvas element (and I'm not reviewing that; you need another reviewer familiar with XUL) then what you need to do is: (1) Call CanvasRenderingcontext2D::GetSurfaceSnapshot to get a SourceSurface. (2) Construct a gfxSurfaceDrawable using that SourceSurface. (3) Use ImageOps::CreateFromDrawable to create an imgIContainer from the gfxSurfaceDrawable.
Attachment #8477256 - Flags: review?(seth) → review-
Comment on attachment 8477440 [details] [diff] [review] 0002-Add-rich-tree-support-draw-the-canvas-in-treeceel-in.patch Review of attachment 8477440 [details] [diff] [review]: ----------------------------------------------------------------- r-'ing this patch as well; see the previous review for why.
Attachment #8477440 - Flags: review?(seth) → review-
(In reply to Seth Fowler [:seth] from comment #115) > Comment on attachment 8477256 [details] [diff] [review] > 0001-Add-API-mozFetchStream-to-synchronously-fetch-the-ns.patch > > Review of attachment 8477256 [details] [diff] [review]: > ----------------------------------------------------------------- > > This isn't the way to go. The method you're using the get the content of the > canvas is also extremely expensive, since you're encoding it to PNG and then > immediately decoding it again. You also don't need to (and shouldn't) add > any methods to nsIDOMHTMLCanvasElement; stick to internal, C++-only APIs for > this. Thanks a lot, I have no intention add a API to nsIDOMHTMLCanvasElement, it's just because this is the only I figure out. > > If the best way to resolve this bug is to create an image from a canvas > element (and I'm not reviewing that; you need another reviewer familiar with > XUL) then what you need to do is: > > (1) Call CanvasRenderingcontext2D::GetSurfaceSnapshot to get a SourceSurface. > (2) Construct a gfxSurfaceDrawable using that SourceSurface. > (3) Use ImageOps::CreateFromDrawable to create an imgIContainer from the > gfxSurfaceDrawable. Thanks a lot, I also knows that image encoding and decoding is costing and not necessary, I will have a try about your solution.
(In reply to Yonggang Luo from comment #117) > (In reply to Seth Fowler [:seth] from comment #115) > > Comment on attachment 8477256 [details] [diff] [review] > > 0001-Add-API-mozFetchStream-to-synchronously-fetch-the-ns.patch > > > > Review of attachment 8477256 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > This isn't the way to go. The method you're using the get the content of the > > canvas is also extremely expensive, since you're encoding it to PNG and then > > immediately decoding it again. You also don't need to (and shouldn't) add > > any methods to nsIDOMHTMLCanvasElement; stick to internal, C++-only APIs for > > this. > Thanks a lot, I have no intention add a API to nsIDOMHTMLCanvasElement, it's > just because > this is the only I figure out. > > > > > If the best way to resolve this bug is to create an image from a canvas > > element (and I'm not reviewing that; you need another reviewer familiar with > > XUL) then what you need to do is: > > > > (1) Call CanvasRenderingcontext2D::GetSurfaceSnapshot to get a SourceSurface. > > (2) Construct a gfxSurfaceDrawable using that SourceSurface. > > (3) Use ImageOps::CreateFromDrawable to create an imgIContainer from the > > gfxSurfaceDrawable. There is no such a API ImageOps::CreateFromDrawable, I am getting confused. > > Thanks a lot, I also knows that image encoding and decoding is costing and > not necessary, I will have a try about your solution.
Updated version doesn't need the image encode decode.
Attachment #8477256 - Attachment is obsolete: true
Attachment #8477440 - Attachment is obsolete: true
Attachment #8477845 - Flags: review?(seth)
Comment on attachment 8477845 [details] [diff] [review] 001-Add-rich-treecell-functional-in-tree-element-by-rend.patch Review of attachment 8477845 [details] [diff] [review]: ----------------------------------------------------------------- This patch is a good start! It still has a way to go, but with some work you can get it ready to land. Here are the most important things you need to do: 1. Pull an up-to-date copy of mozilla-central, and switch to using ImageOps::CreateFromDrawable. 2. Fix the issues I point out below. You should do this before requesting review from anyone else, because without the style issues the patch will be much easier to review. 3. Find someone who's familiar with the XUL layout code to review this patch. (You can find a list of the peers at https://wiki.mozilla.org/Modules/All) 4. Once you get an r+ from them, ask me for a review again. ::: layout/xul/tree/nsITreeBoxObject.idl @@ +20,5 @@ > + /** > + * To rendering a TreeCell as a canvas, returning the canvas that > + * rendered outside. > + */ > + nsISupports render(in nsITreeView view, in long row, in nsITreeColumn col, in nsIMutableArray props); Why return an nsISupports? I'm not sure why this should return anything, in fact. If you need to get the canvas object this contains, you should probably have a separate getter or attribute that uses a canvas interface and not just nsISupports. @@ +42,5 @@ > /** > + * The canvas handler for rich cell, to rendering anything that canvs > + * support, we can use this to rendering beatiful tree cell. > + */ > + attribute nsITreeCellCanvas cellCanvas; I don't really deal with XUL at all (which is why you really need to get someone who does to review this patch), so this may be off base, but: why add this attribute? There isn't an attribute for the image case. ::: layout/xul/tree/nsTreeBodyFrame.cpp @@ +91,4 @@ > return PL_DHASH_NEXT; > } > > +static nsCOMPtr<nsIMutableArray> AtomArrayToMutableArray(AtomArray &atomArray) Nit: Mozilla style is to place the return type on a separate line. You should also start arguments' names with 'a' and group the '&' with the type. So write: static nsCOMPtr<nsIMutableArray> AtomArrayToMutableArray(AtomArray& aAtomArray) For more information on Mozilla's recommended coding style, please read this page: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style @@ +2320,4 @@ > nsSize > nsTreeBodyFrame::GetImageDestSize(nsStyleContext* aStyleContext, > bool useImageRegion, > + nsIntSize* sz) Nit: Mozilla style is to start an argument's name with 'a', and spelling things out is preferred. Use |nsIntSize* aSize|. @@ +2361,4 @@ > // Use the width of the image region. > imageSize.width = myList->mImageRegion.width; > } > + else if (sz) { Nit: place the |else if| on the same line as the |}|. There are some other cases in this patch, too. @@ +2412,4 @@ > nsRect > nsTreeBodyFrame::GetImageSourceRect(nsStyleContext* aStyleContext, > bool useImageRegion, > + nsIntSize* sz) aSize here too. @@ +3263,5 @@ > nsRect iconRect(currX, cellRect.y, remainingWidth, cellRect.height); > nsRect dirtyRect; > if (dirtyRect.IntersectRect(aDirtyRect, iconRect)) > PaintImage(aRowIndex, aColumn, iconRect, aPresContext, aRenderingContext, aDirtyRect, > + remainingWidth, currX, false); To the reader, when you have a boolean like "false", it's not obvious what it does. You don't always have to do it, but I personally prefer to make it clear what the boolean means like this: remainingWidth, currX, /* aIsCanvas = */ false); Consider doing this for both calls to PaintImage. @@ +3410,5 @@ > nsRenderingContext& aRenderingContext, > const nsRect& aDirtyRect, > nscoord& aRemainingWidth, > + nscoord& aCurrX, > + bool isCanvas) Nit: please use |aIsCanvas|. @@ +3422,5 @@ > + if (isCanvas) { > + imageContext = GetPseudoStyleContext(nsCSSAnonBoxes::moztreecanvas); > + } else { > + imageContext = GetPseudoStyleContext(nsCSSAnonBoxes::moztreeimage); > + } This whole chunk of code can be more concisely written as: nsStyleContext* imageContext = GetPseudoStyleContext(isCanvas ? nsCSSAnonBoxes::moztreecanvas : nsCSSAnonBoxes::moztreeimage); @@ +3439,5 @@ > bool useImageRegion = true; > nsCOMPtr<imgIContainer> image; > + RefPtr<mozilla::gfx::SourceSurface> surface; > + nsIntSize contentSize(0, 0); > + nsIntSize *contentSizePtr = NULL; Nit: it's Mozilla style to group the '*' with the type, and to use 'nullptr' instead of 'NULL', so write |nsIntSize* contentSizePtr = nullptr;|. However, you don't need contentSizePtr at all, because |&contentSize| will work just as well in any context. @@ +3447,5 @@ > + if (image) { > + int32_t width; > + int32_t height; > + image->GetWidth(&width); > + image->GetHeight(&height); You can just write |image->GetWidth(&contentSize.width);| and similar for height. You don't need the intermediate variables. @@ +3462,5 @@ > + cellCanvas->Render(mView, > + aRowIndex, > + aColumn, > + scratchArray, > + getter_AddRefs(canvasCtx)); Nit: You should put multiple arguments on a line so that this call to Render takes up fewer lines. @@ +3467,5 @@ > + } > + > + nsICanvasRenderingContextInternal *ctxInternal = NULL; > + if (canvasCtx) { > + canvasCtx->QueryInterface(NS_GET_IID(nsICanvasRenderingContextInternal), (void**)&ctxInternal); You should never call QueryInterface manually; always use helper functions like do_QueryInterface. Also, you should store the result in an nsCOMPtr or you'll leak memory. You can write the above as: nsCOMPtr<nsICanvasRenderingContextInternal> ctxInternal = do_QueryInterface(canvasCtx); You don't even need to check for null. For more information, see here: https://developer.mozilla.org/en-US/docs/Using_nsCOMPtr/Reference_Manual @@ +3583,5 @@ > + imgIContainer::FLAG_NONE); > + } else { > + nsRefPtr<gfxDrawable> drawable = > + new gfxSurfaceDrawable(surface, ThebesIntSize(surface->GetSize())); > + nsLayoutUtils::DrawPixelSnapped(&aRenderingContext, You should pull, because your code is out of date. nsLayoutUtils::DrawPixelSnapped no longer exists. That also explains why you don't see ImageOps::CreateFromDrawable, which *does* exist and is what you should use instead. Once you're using ImageOps::CreateFromDrawable, you won't need two separate code paths either, because you'll have an imgIContainer in either case. @@ +4753,5 @@ > return NS_OK; > } > + > +nsresult > +nsTreeBodyFrame::GetCellCanvas(nsITreeCellCanvas ** aCellCanvas) Nit: Group the "**" with the type. Similar for SetCellCanvas below. @@ +4757,5 @@ > +nsTreeBodyFrame::GetCellCanvas(nsITreeCellCanvas ** aCellCanvas) > +{ > + *aCellCanvas = nullptr; > + nsWeakFrame weakFrame(this); > + NS_ENSURE_STATE(weakFrame.IsAlive()); Why are you creating an nsWeakFrame here at all? @@ +4758,5 @@ > +{ > + *aCellCanvas = nullptr; > + nsWeakFrame weakFrame(this); > + NS_ENSURE_STATE(weakFrame.IsAlive()); > + NS_IF_ADDREF(*aCellCanvas = mCellCanvas); Don't use NS_IF_ADDREF. The proper pattern here is: nsCOMPtr<nsITreeCellCanvas> canvas = mCellCanvas; canvas.forget(aCellCanvas); ::: layout/xul/tree/nsTreeBoxObject.cpp @@ +493,5 @@ > +NS_IMETHODIMP > +nsTreeBoxObject::GetCellCanvas(nsITreeCellCanvas ** aCellCanvas) > +{ > + nsTreeBodyFrame* body = GetTreeBody(); > + if (body) Nit: Always use braces around 'if' and 'else' bodies, even if they're only one line.
Attachment #8477845 - Flags: review?(seth) → review-
Comment on attachment 8477845 [details] [diff] [review] 001-Add-rich-treecell-functional-in-tree-element-by-rend.patch Review of attachment 8477845 [details] [diff] [review]: ----------------------------------------------------------------- I have some serious issues as well, from the standpoint of someone who crafts trees in XUL a fair bit: ::: layout/xul/tree/nsITreeBoxObject.idl @@ +10,5 @@ > interface nsITreeView; > interface nsITreeSelection; > interface nsITreeColumn; > interface nsITreeColumns; > +interface nsIMutableArray; Please, don't use an nsIMutableArray of nsIAtom objects. Use AString instead, as in https://hg.mozilla.org/releases/mozilla-release/diff/bf88a316cf18/layout/xul/tree/nsITreeView.idl . @@ +27,1 @@ > [scriptable, uuid(64BA5199-C4F4-4498-BBDC-F8E4C369086C)] I'm pretty sure you have to rev the UUID for any interface where you add a new attribute or method. @@ +42,5 @@ > /** > + * The canvas handler for rich cell, to rendering anything that canvs > + * support, we can use this to rendering beatiful tree cell. > + */ > + attribute nsITreeCellCanvas cellCanvas; The nsITreeView interface is probably a better place to add this property, because it's the tree view that we're supposed to implement to provide the contents of the tree. The tree box object is native. Why does nsITreeCellCanvas inherit from nsIDOMEventListener? If it doesn't have to, please consider adding a renderCanvas method instead to nsITreeView. To answer Seth's question: nsITreeView has a getImageSrc() method. ::: layout/xul/tree/nsITreeColumns.idl @@ +32,4 @@ > const short TYPE_TEXT = 1; > const short TYPE_CHECKBOX = 2; > const short TYPE_PROGRESSMETER = 3; > + const short TYPE_CANVAS = 4; I don't remember whether this is enough to require a uuid bump or not. ::: layout/xul/tree/nsTreeBodyFrame.cpp @@ +99,5 @@ > + scratchArray->AppendElement(atomArray[i], false); > + } > + return scratchArray; > +} > + Again, a mutable array of nsIAtoms is less desirable. See nsTreeUtils::TokenizeProperties. ::: layout/xul/tree/nsTreeBoxObject.cpp @@ +495,5 @@ > +{ > + nsTreeBodyFrame* body = GetTreeBody(); > + if (body) > + return body->GetCellCanvas(aCellCanvas); > + else else after return is considered evil.
Attachment #8477845 - Flags: feedback-
nsITreeCellCanvas sounds like it should be an object that you could draw to, but really the canvas still needs to be created elsewhere and returned by this interface's |renderCanvas| method. nsITreeCellRenderer instead? (Side note: can you use "-U 8" for your next patch? Three lines of context makes things kind of difficult to read.) (In reply to Seth Fowler [:seth] from comment #120) > why add this attribute? There isn't an attribute for the image case. Could you be more specific about your question? It seems like you're making an implicit suggestion here, but I'm not sure what it is. (In reply to Alex Vincent [:WeirdAl] from comment #121) > @@ +42,5 @@ > > /** > > + * The canvas handler for rich cell, to rendering anything that canvs > > + * support, we can use this to rendering beatiful tree cell. > > + */ > > + attribute nsITreeCellCanvas cellCanvas; > > The nsITreeView interface is probably a better place to add this property, > because it's the tree view that we're supposed to implement to provide the > contents of the tree. The tree box object is native. > > Why does nsITreeCellCanvas inherit from nsIDOMEventListener? > > If it doesn't have to, please consider adding a renderCanvas method instead > to nsITreeView. Requiring that every nsITreeView implementation have a |renderCanvas| method would be pretty terrible. Consider the cascade of changes that this would entail for implementations that live in m-c. Now consider the changes to all the implementations that live in extensions. All of which are just going end up as this: yet another no-op method to satisfy the already cumbersome interface that is nsITreeView.
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #74) > (In reply to Yonggang Luo from comment #73) > > How did PostBox enhancements to TreeView? > > Apparently, they used the patch attached to this bug. Where are you seeing that?
(In reply to Colby Russell :crussell from comment #122) > Requiring that every nsITreeView implementation have a |renderCanvas| method > would be pretty terrible. Consider the cascade of changes that this would > entail for implementations that live in m-c. Now consider the changes to > all the implementations that live in extensions. All of which are just > going end up as this: yet another no-op method to satisfy the already > cumbersome interface that is nsITreeView. Meh, I hadn't considered that impact. However, a counterpoint: only C++ implementations would *have* to change. If the method isn't called, JS implementations don't care. (Admittedly it'd be sloppy not to, but...) Alternatively, we could define a nsITreeViewCanvas interface that inherits from nsITreeView, and then QI for that interface before calling the method.
(In reply to Alex Vincent [:WeirdAl] from comment #124) > Alternatively, we could define a nsITreeViewCanvas interface that inherits > from nsITreeView, and then QI for that interface before calling the method. That's the right approach, but one I didn't mention because of how unwieldy the design for the tree views is currently; nsITreeView needs to be split up to progressively allow more advanced features for implementations that opt-in by supporting the extended interfaces. I fear that starting off in that direction for this bug alone (and then necessarily quitting less than half way there) seems like it wouldn't actually make it that much easier to do if/when the time comes where someone decides to actually do it, and might even make it harder. The whole thing really needs to have some good thought put into it and tackled in one go. Given the way things are, setting the tree's |cellRenderer|[*] right before you set its |view| seems okay enough. Your choice as the implementor whether you want the rvals in those assignments to be the same object or different ones; the |renderCanvas| call from the patch already gives you a parameter specifying the view it was called for. * Not |cellCanvas|, please, for the reason above.
(In reply to Colby Russell :crussell from comment #125) > (In reply to Alex Vincent [:WeirdAl] from comment #124) > > Alternatively, we could define a nsITreeViewCanvas interface that inherits > > from nsITreeView, and then QI for that interface before calling the method. > > That's the right approach, but one I didn't mention because of how unwieldy > the design for the tree views is currently; nsITreeView needs to be split up > to progressively allow more advanced features for implementations that > opt-in by supporting the extended interfaces. I fear that starting off in > that direction for this bug alone (and then necessarily quitting less than > half way there) seems like it wouldn't actually make it that much easier to > do if/when the time comes where someone decides to actually do it, and might > even make it harder. The whole thing really needs to have some good thought > put into it and tackled in one go. Agreed, we may have a discuss about a better idea. I need more suggestion about how to design a better API to extend the tree cell render functional, At the current time, we have TEXT, CHECKBOX, PROGRESSMETER, and now it's renderer, I am not so sure that they have the same altitude, I treat the canvas as the same position like checkbox, progressmeter. After that, is that nsIDOMEventListener need to be extend by the renderer? I need the canvas to response to external events. > > Given the way things are, setting the tree's |cellRenderer|[*] right before > you set its |view| seems okay enough. Your choice as the implementor > whether you want the rvals in those assignments to be the same object or > different ones; the |renderCanvas| call from the patch already gives you a > parameter specifying the view it was called for. > > * Not |cellCanvas|, please, for the reason above. I totally agreed.
(In reply to Alex Vincent [:WeirdAl] from comment #121) > Comment on attachment 8477845 [details] [diff] [review] > 001-Add-rich-treecell-functional-in-tree-element-by-rend.patch > > Review of attachment 8477845 [details] [diff] [review]: > ----------------------------------------------------------------- > > I have some serious issues as well, from the standpoint of someone who > crafts trees in XUL a fair bit: > > ::: layout/xul/tree/nsITreeBoxObject.idl > @@ +10,5 @@ > > interface nsITreeView; > > interface nsITreeSelection; > > interface nsITreeColumn; > > interface nsITreeColumns; > > +interface nsIMutableArray; > > Please, don't use an nsIMutableArray of nsIAtom objects. Use AString > instead, as in > https://hg.mozilla.org/releases/mozilla-release/diff/bf88a316cf18/layout/xul/ > tree/nsITreeView.idl . > > @@ +27,1 @@ > > [scriptable, uuid(64BA5199-C4F4-4498-BBDC-F8E4C369086C)] > > I'm pretty sure you have to rev the UUID for any interface where you add a > new attribute or method. > > @@ +42,5 @@ > > /** > > + * The canvas handler for rich cell, to rendering anything that canvs > > + * support, we can use this to rendering beatiful tree cell. > > + */ > > + attribute nsITreeCellCanvas cellCanvas; > > The nsITreeView interface is probably a better place to add this property, > because it's the tree view that we're supposed to implement to provide the > contents of the tree. The tree box object is native. > > Why does nsITreeCellCanvas inherit from nsIDOMEventListener? I need the events for canvas, so canvas can draw the things sense to external events. > > If it doesn't have to, please consider adding a renderCanvas method instead > to nsITreeView. From my points of view, nsITreeView is a `dataProvider`, and nsITreeCellCanvas is a `dataConsumer`, The dataProvider are often to be stable, but the dataConsumer is vary time to time, so it's better not to be placed at the same place. In other works. renderCanvas is like CSS, we can modify it frequently according to different environment. > > To answer Seth's question: nsITreeView has a getImageSrc() method. Yeah, you are right about this, we have getImageSrc, the problem with getImageSrc is this method is returned the URL, that's the compressed data, that means we need to decompress latter, and that was time consuming, Seth's now provider a way to rendering the canvas as the TreeCell, so that we doesn't need to compress/decompress the image that consuming time. > > ::: layout/xul/tree/nsITreeColumns.idl > @@ +32,4 @@ > > const short TYPE_TEXT = 1; > > const short TYPE_CHECKBOX = 2; > > const short TYPE_PROGRESSMETER = 3; > > + const short TYPE_CANVAS = 4; > > I don't remember whether this is enough to require a uuid bump or not. Yeah, you are that, that's need a bump. > > ::: layout/xul/tree/nsTreeBodyFrame.cpp > @@ +99,5 @@ > > + scratchArray->AppendElement(atomArray[i], false); > > + } > > + return scratchArray; > > +} > > + > > Again, a mutable array of nsIAtoms is less desirable. See > nsTreeUtils::TokenizeProperties. > Thanks, hope this API would simplify my logic. > ::: layout/xul/tree/nsTreeBoxObject.cpp > @@ +495,5 @@ > > +{ > > + nsTreeBodyFrame* body = GetTreeBody(); > > + if (body) > > + return body->GetCellCanvas(aCellCanvas); > > + else > > else after return is considered evil. True: thanks.
(In reply to Seth Fowler [:seth] from comment #120) > Comment on attachment 8477845 [details] [diff] [review] > 001-Add-rich-treecell-functional-in-tree-element-by-rend.patch > > Review of attachment 8477845 [details] [diff] [review]: > ----------------------------------------------------------------- > > This patch is a good start! It still has a way to go, but with some work you > can get it ready to land. Here are the most important things you need to do: > > 1. Pull an up-to-date copy of mozilla-central, and switch to using > ImageOps::CreateFromDrawable. Thanks a lot, that's a totally new API, that's good:) Maybe we need more direct API latter, so we can directly return a ImageContainer in the javascript code from canvas. Anyway that's fantastic, I am using thunderbird 31 because my product was based on a stable version of mozilla 31. now It's stable for my product, and I have the chance to getting the code up-to-date. > 2. Fix the issues I point out below. You should do this before requesting > review from anyone else, because without the style issues the patch will be > much easier to review. > 3. Find someone who's familiar with the XUL layout code to review this > patch. (You can find a list of the peers at > https://wiki.mozilla.org/Modules/All) > 4. Once you get an r+ from them, ask me for a review again. > > ::: layout/xul/tree/nsITreeBoxObject.idl > @@ +20,5 @@ > > + /** > > + * To rendering a TreeCell as a canvas, returning the canvas that > > + * rendered outside. > > + */ > > + nsISupports render(in nsITreeView view, in long row, in nsITreeColumn col, in nsIMutableArray props); > > Why return an nsISupports? I'm not sure why this should return anything, in > fact. If you need to get the canvas object this contains, you should > probably have a separate getter or attribute that uses a canvas interface > and not just nsISupports. > Indeed, I'd like to return ImageContainer directly, but we lack that API in Javascript, return the canvas is also a good idea, but that means more C++ code-wrapping. > @@ +42,5 @@ > > /** > > + * The canvas handler for rich cell, to rendering anything that canvs > > + * support, we can use this to rendering beatiful tree cell. > > + */ > > + attribute nsITreeCellCanvas cellCanvas; > > I don't really deal with XUL at all (which is why you really need to get > someone who does to review this patch), so this may be off base, but: why > add this attribute? There isn't an attribute for the image case. Yeap, we need a better design about this. > > ::: layout/xul/tree/nsTreeBodyFrame.cpp > @@ +91,4 @@ > > return PL_DHASH_NEXT; > > } > > > > +static nsCOMPtr<nsIMutableArray> AtomArrayToMutableArray(AtomArray &atomArray) > > Nit: Mozilla style is to place the return type on a separate line. You > should also start arguments' names with 'a' and group the '&' with the type. Gocha > So write: > > static nsCOMPtr<nsIMutableArray> > AtomArrayToMutableArray(AtomArray& aAtomArray) > > For more information on Mozilla's recommended coding style, please read this > page: > > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style > > @@ +2320,4 @@ > > nsSize > > nsTreeBodyFrame::GetImageDestSize(nsStyleContext* aStyleContext, > > bool useImageRegion, > > + nsIntSize* sz) > > Nit: Mozilla style is to start an argument's name with 'a', and spelling > things out is preferred. Use |nsIntSize* aSize|. > ditto > @@ +2361,4 @@ > > // Use the width of the image region. > > imageSize.width = myList->mImageRegion.width; > > } > > + else if (sz) { > > Nit: place the |else if| on the same line as the |}|. There are some other > cases in this patch, too. > ditto > @@ +2412,4 @@ > > nsRect > > nsTreeBodyFrame::GetImageSourceRect(nsStyleContext* aStyleContext, > > bool useImageRegion, > > + nsIntSize* sz) > > aSize here too. > ditto > @@ +3263,5 @@ > > nsRect iconRect(currX, cellRect.y, remainingWidth, cellRect.height); > > nsRect dirtyRect; > > if (dirtyRect.IntersectRect(aDirtyRect, iconRect)) > > PaintImage(aRowIndex, aColumn, iconRect, aPresContext, aRenderingContext, aDirtyRect, > > + remainingWidth, currX, false); > > To the reader, when you have a boolean like "false", it's not obvious what > it does. You don't always have to do it, but I personally prefer to make it > clear what the boolean means like this: > > remainingWidth, currX, /* aIsCanvas = */ false); > ditto, maybe the better solution is doesn't pass a bool, but pass the image directly. > Consider doing this for both calls to PaintImage. > > @@ +3410,5 @@ > > nsRenderingContext& aRenderingContext, > > const nsRect& aDirtyRect, > > nscoord& aRemainingWidth, > > + nscoord& aCurrX, > > + bool isCanvas) > > Nit: please use |aIsCanvas|. > ditto > @@ +3422,5 @@ > > + if (isCanvas) { > > + imageContext = GetPseudoStyleContext(nsCSSAnonBoxes::moztreecanvas); > > + } else { > > + imageContext = GetPseudoStyleContext(nsCSSAnonBoxes::moztreeimage); > > + } > > This whole chunk of code can be more concisely written as: > > nsStyleContext* imageContext = > GetPseudoStyleContext(isCanvas ? nsCSSAnonBoxes::moztreecanvas : > nsCSSAnonBoxes::moztreeimage); > True, > @@ +3439,5 @@ > > bool useImageRegion = true; > > nsCOMPtr<imgIContainer> image; > > + RefPtr<mozilla::gfx::SourceSurface> surface; > > + nsIntSize contentSize(0, 0); > > + nsIntSize *contentSizePtr = NULL; > > Nit: it's Mozilla style to group the '*' with the type, and to use 'nullptr' > instead of 'NULL', so write |nsIntSize* contentSizePtr = nullptr;|. > > However, you don't need contentSizePtr at all, because |&contentSize| will > work just as well in any context. Yeap, with your API, this is not necessary anymore:) > > @@ +3447,5 @@ > > + if (image) { > > + int32_t width; > > + int32_t height; > > + image->GetWidth(&width); > > + image->GetHeight(&height); > > You can just write |image->GetWidth(&contentSize.width);| and similar for > height. You don't need the intermediate variables. Nice:) > > @@ +3462,5 @@ > > + cellCanvas->Render(mView, > > + aRowIndex, > > + aColumn, > > + scratchArray, > > + getter_AddRefs(canvasCtx)); > > Nit: You should put multiple arguments on a line so that this call to Render > takes up fewer lines. Gotcha. > > @@ +3467,5 @@ > > + } > > + > > + nsICanvasRenderingContextInternal *ctxInternal = NULL; > > + if (canvasCtx) { > > + canvasCtx->QueryInterface(NS_GET_IID(nsICanvasRenderingContextInternal), (void**)&ctxInternal); > > You should never call QueryInterface manually; always use helper functions > like do_QueryInterface. Also, you should store the result in an nsCOMPtr or > you'll leak memory. You can write the above as: Good > > nsCOMPtr<nsICanvasRenderingContextInternal> ctxInternal = > do_QueryInterface(canvasCtx); > > You don't even need to check for null. > > For more information, see here: > > https://developer.mozilla.org/en-US/docs/Using_nsCOMPtr/Reference_Manual > > @@ +3583,5 @@ > > + imgIContainer::FLAG_NONE); > > + } else { > > + nsRefPtr<gfxDrawable> drawable = > > + new gfxSurfaceDrawable(surface, ThebesIntSize(surface->GetSize())); > > + nsLayoutUtils::DrawPixelSnapped(&aRenderingContext, > > You should pull, because your code is out of date. > nsLayoutUtils::DrawPixelSnapped no longer exists. That also explains why you > don't see ImageOps::CreateFromDrawable, which *does* exist and is what you > should use instead. Once you're using ImageOps::CreateFromDrawable, you > won't need two separate code paths either, because you'll have an > imgIContainer in either case. > > @@ +4753,5 @@ > > return NS_OK; > > } > > + > > +nsresult > > +nsTreeBodyFrame::GetCellCanvas(nsITreeCellCanvas ** aCellCanvas) > > Nit: Group the "**" with the type. Similar for SetCellCanvas below. > > @@ +4757,5 @@ > > +nsTreeBodyFrame::GetCellCanvas(nsITreeCellCanvas ** aCellCanvas) > > +{ > > + *aCellCanvas = nullptr; > > + nsWeakFrame weakFrame(this); > > + NS_ENSURE_STATE(weakFrame.IsAlive()); > > Why are you creating an nsWeakFrame here at all? I am copied the code about how nsITreeView is Get/Set, so I am not so sure about this. > > @@ +4758,5 @@ > > +{ > > + *aCellCanvas = nullptr; > > + nsWeakFrame weakFrame(this); > > + NS_ENSURE_STATE(weakFrame.IsAlive()); > > + NS_IF_ADDREF(*aCellCanvas = mCellCanvas); > > Don't use NS_IF_ADDREF. The proper pattern here is: > > nsCOMPtr<nsITreeCellCanvas> canvas = mCellCanvas; > canvas.forget(aCellCanvas); thanks, maybe the treeView's Set Code also need this modification. > > ::: layout/xul/tree/nsTreeBoxObject.cpp > @@ +493,5 @@ > > +NS_IMETHODIMP > > +nsTreeBoxObject::GetCellCanvas(nsITreeCellCanvas ** aCellCanvas) > > +{ > > + nsTreeBodyFrame* body = GetTreeBody(); > > + if (body) > > Nit: Always use braces around 'if' and 'else' bodies, even if they're only > one line. +1
> > ::: layout/xul/tree/nsITreeColumns.idl > @@ +32,4 @@ > > const short TYPE_TEXT = 1; > > const short TYPE_CHECKBOX = 2; > > const short TYPE_PROGRESSMETER = 3; > > + const short TYPE_CANVAS = 4; > > I don't remember whether this is enough to require a uuid bump or not. > > ::: layout/xul/tree/nsTreeBodyFrame.cpp > @@ +99,5 @@ > > + scratchArray->AppendElement(atomArray[i], false); > > + } > > + return scratchArray; > > +} > > + > > Again, a mutable array of nsIAtoms is less desirable. See > nsTreeUtils::TokenizeProperties. > Do you means I need to create a new API that like StringnizeProperties?
(In reply to Colby Russell :crussell from comment #122) > nsITreeCellCanvas sounds like it should be an object that you could draw to, > but really the canvas still needs to be created elsewhere and returned by > this interface's |renderCanvas| method. nsITreeCellRenderer instead? > > (Side note: can you use "-U 8" for your next patch? Three lines of context > makes things kind of difficult to read.) > Can you tell me the full command, cause I don't know what's do you mean.
This is a tentative patch about API. I am looking for suggestion first
Attachment #8482308 - Flags: review?(seth)
Attachment #8482308 - Flags: review?(ajvincent)
Attachment #8482308 - Flags: review?(Sevenspade)
(In reply to Yonggang Luo from comment #130) > (In reply to Colby Russell :crussell from comment #122) > > nsITreeCellCanvas sounds like it should be an object that you could draw to, > > but really the canvas still needs to be created elsewhere and returned by > > this interface's |renderCanvas| method. nsITreeCellRenderer instead? > > > > (Side note: can you use "-U 8" for your next patch? Three lines of context > > makes things kind of difficult to read.) > > > Can you tell me the full command, cause I don't know what's do you mean. See <https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F>. Note the "unified = 8" part in the example .hgrc. Alternatively, if you're creating your patches with hg diff, you can use hg diff -U 8. If you're using mq, that won't work, though, so the .hgrc change is best, and more convenient anyway.
Comment on attachment 8482308 [details] [diff] [review] 0002-Reshape-The-API-to-looking-for-suggestion.patch Review of attachment 8482308 [details] [diff] [review]: ----------------------------------------------------------------- The review field is for people who have the authority to approve changes. I can't review this. feedback+ from me, though. This patch is based on changes to your previous one. When submitting patches, you'll want to submit a whole new patch reflecting all the changes you intend to make, and mark the old one obsolete. Bugzilla already has a built-in interdiff feature for reviewers to show them what changed between patches. (The exception is when you're working on a bug that makes big changes, and you decide ahead of time to split things up into discrete chunks to make them easier to work with and upload part 1/x, part 2/x, et cetera. But this bug isn't one of them.) Wait until Neil (or someone else) gives you a positive review before requesting one from Seth. Thanks for working on this. ::: layout/xul/tree/nsITreeBoxObject.idl @@ +14,4 @@ > interface nsIScriptableRegion; > > +[scriptable, uuid(aceed5f8-e57a-4cdc-b859-cf864904ab1c)] > +interface nsITreeCellRender : nsISupports nsITreeCellRenderer, not nsITreeCellRender. @@ +42,4 @@ > * The canvas handler for rich cell, to rendering anything that canvs > * support, we can use this to rendering beatiful tree cell. > */ > + attribute nsITreeCellRender cellCanvas; |cellRenderer| here, too, not |cellCanvas|.
Attachment #8482308 - Flags: review?(seth)
Attachment #8482308 - Flags: review?(neil)
Attachment #8482308 - Flags: review?(ajvincent)
Attachment #8482308 - Flags: review?(Sevenspade)
Attachment #8482308 - Flags: feedback?(ajvincent)
I forgot to mention that I bigger fan of the approach in the previous patch where the |draw| method would be returning a canvas element (in which case the nsITreeCellRenderer IDL should say that, and not nsISupports, as Seth says). imgIContainer is conceptually "purer", but for practical reasons nsIDOMHTMLCanvasElement would be easier to work with for people who would actually be looking into implementing this. It does occur to me that the APIs don't exist for a scripted renderer to be able to get the correct styles to use to paint the cell... (In reply to Seth Fowler [:seth] from comment #120) > I'm not sure why this should return anything, in > fact. If you need to get the canvas object this contains, you should > probably have a separate getter or attribute that uses a canvas interface > and not just nsISupports. This effectively enforces an "immediate mode" approach, since the tree's one renderer would be limited to one canvas shared between all cells it's responsible for painting. The renderer would have to redo every |draw| call, even for cells that haven't changed, instead of being able to hand back a canvas/imgIContainer with no changes.
(In reply to Colby Russell :crussell from comment #134) > I forgot to mention that I bigger fan of the approach in the previous patch > where the |draw| method would be returning a canvas element (in which case > the nsITreeCellRenderer IDL should say that, and not nsISupports, as Seth > says). imgIContainer is conceptually "purer", but for practical reasons > nsIDOMHTMLCanvasElement would be easier to work with for people who would > actually be looking into implementing this. In my design, returning imgIContainer would not stop users to using nsIDOMHTMLCanvasElement to rendering things. Just to make sure the returning result is imgIContainer, we can expose the API that can convert from nsIDOMHTMLCanvasElement to imgIContainer. If we really want to return nsIDOMHTMLCanvasElement, then we would using a less generic method drawCanvas(...). > > It does occur to me that the APIs don't exist for a scripted renderer to be > able to get the correct styles to use to paint the cell... > > (In reply to Seth Fowler [:seth] from comment #120) > > I'm not sure why this should return anything, in > > fact. If you need to get the canvas object this contains, you should > > probably have a separate getter or attribute that uses a canvas interface > > and not just nsISupports. > > This effectively enforces an "immediate mode" approach, since the tree's one > renderer would be limited to one canvas shared between all cells it's > responsible for painting. The renderer would have to redo every |draw| > call, even for cells that haven't changed, instead of being able to hand Well, this is not so sure, because tree is using invalidate to handling the re-draw, so we can make sure everytime the renderer calling to draw, it's always can be sure that we really need to redraw the things totally. > back a canvas/imgIContainer with no changes.
:seth, does the following implementation are OK? NS_IMETHODIMP imgTools::CreateImageFromCanvas(nsIDOMHTMLCanvasElement *aCanvas, const nsAString& aContextName, imgIContainer **aContainer) { *aContainer = nullptr; NS_ENSURE_ARG_POINTER(aCanvas); nsAutoString contextName(NS_LITERAL_STRING("2d")); if (aContextName != EmptyString()) { contextName = aContextName; } nsCOMPtr<nsISupports> canvasContext; aCanvas->MozGetIPCContext(contextName, getter_AddRefs(canvasContext)); nsCOMPtr<nsICanvasRenderingContextInternal> ctxInternal = do_QueryInterface(canvasContext); if (!ctxInternal) { return NS_OK; } RefPtr<mozilla::gfx::SourceSurface> surface = ctxInternal->GetSurfaceSnapshot(); if (!surface) { return NS_OK; } RefPtr<gfxDrawable> drawable = new gfxSurfaceDrawable(surface, ThebesIntSize(surface->GetSize())); nsCOMPtr<imgIContainer> container = ImageOps::CreateFromDrawable(drawable); *aContainer = container; return NS_OK; }
I still feels pass Context is better: NS_IMETHODIMP imgTools::CreateImageFromCanvasContext(nsISupports *aCanvasContext, imgIContainer **aContainer) { *aContainer = nullptr; nsCOMPtr<nsICanvasRenderingContextInternal> ctxInternal = do_QueryInterface(aCanvasContext); We doesn't need to handle the getContext things.
(In reply to Colby Russell :crussell from comment #134) > This effectively enforces an "immediate mode" approach, since the tree's one > renderer would be limited to one canvas shared between all cells it's > responsible for painting. The renderer would have to redo every |draw| > call, even for cells that haven't changed, instead of being able to hand > back a canvas/imgIContainer with no changes. Yeah, that doesn't sound desirable, though I still find the API strange. I'll have to rely on people more experienced with XUL to suggest a better alternative if there is one, because I don't know really know this part of the code at all.
(In reply to Yonggang Luo from comment #136) > :seth, does the following implementation are OK? Almost. > nsCOMPtr<nsICanvasRenderingContextInternal> ctxInternal = > do_QueryInterface(canvasContext); > if (!ctxInternal) { > return NS_OK; > } > RefPtr<mozilla::gfx::SourceSurface> surface = > ctxInternal->GetSurfaceSnapshot(); > if (!surface) { > return NS_OK; > } This is all good. > RefPtr<gfxDrawable> drawable = > new gfxSurfaceDrawable(surface, ThebesIntSize(surface->GetSize())); This is fine too, but you should use an nsRefPtr rather than a RefPtr here. You basically > nsCOMPtr<imgIContainer> container = ImageOps::CreateFromDrawable(drawable); > *aContainer = container; The imgIContainer you're creating here will get destroyed as soon as this method returns. To return the imgIContainer correctly, you need to replace |*aContainer = container;| with |container.forget(aContainer);|. By the way, I agree with you that it's more convenient to just pass in the canvas context if that will work for you.
Whoops, I failed to finish a sentence above. What I meant to write was: "You basically always want to use nsRefPtr for concrete types and nsCOMPtr for interfaces. RefPtr is basically only used with Moz2D code, which is why it's used for SourceSurface."
(In reply to Seth Fowler [:seth] from comment #140) > Whoops, I failed to finish a sentence above. What I meant to write was: "You > basically always want to use nsRefPtr for concrete types and nsCOMPtr for > interfaces. RefPtr is basically only used with Moz2D code, which is why it's > used for SourceSurface." This is the update version of CreateImageFromCanvasContext NS_IMETHODIMP imgTools::CreateImageFromCanvasContext(nsISupports *aCanvasContext, imgIContainer **aContainer) { *aContainer = nullptr; nsCOMPtr<nsICanvasRenderingContextInternal> contextInternal = do_QueryInterface(aCanvasContext); if (!contextInternal) { return NS_OK; } RefPtr<mozilla::gfx::SourceSurface> surface = contextInternal->GetSurfaceSnapshot(); if (!surface) { return NS_OK; } nsRefPtr<gfxDrawable> drawable = new gfxSurfaceDrawable(surface, ThebesIntSize(surface->GetSize())); nsCOMPtr<imgIContainer> container = ImageOps::CreateFromDrawable(drawable); container.forget(aContainer); return NS_OK; }
(In reply to Yonggang Luo from comment #141) > This is the update version of CreateImageFromCanvasContext Looks good! > *aContainer = nullptr; You don't need this line, though.
I fount in bugzilla, it's very hard to submit patch series.
I'll respond with my feedback comments this weekend. I've been very busy lately, and for that I apologize.
Attached patch cellRenderer.diff (obsolete) — Splinter Review
Upgrade the patch with all kinds of fixes. Now it's cellRenderer and return a more generic ImageContainer.
Attachment #8484232 - Flags: review?(seth)
Attachment #8484232 - Flags: review?(dbaron)
Attachment #8484232 - Flags: review?(ajvincent)
Attachment #8484232 - Flags: review?(Sevenspade)
Comment on attachment 8484232 [details] [diff] [review] cellRenderer.diff Review of attachment 8484232 [details] [diff] [review]: ----------------------------------------------------------------- I'd be much more comfortable with this if you were to include some sample XUL+JS that exercises this code. (A reftest would be fantastic, but I wouldn't expect that. A crashtest would be better than no test at all.) The patch has been through a number of changes since the initial proposal, including the API. In particular I want to see an example implementation of the nsITreeCellRenderer, and how to use it. That will help me shape my thoughts on the API. ::: layout/xul/tree/nsITreeBoxObject.idl @@ +39,5 @@ > attribute nsITreeView view; > > /** > + * The canvas handler for rich cell, to rendering anything that canvs > + * support, we can use this to rendering beatiful tree cell. Spelling typos: canvas, beautiful. ::: layout/xul/tree/nsITreeColumns.idl @@ +31,5 @@ > > const short TYPE_TEXT = 1; > const short TYPE_CHECKBOX = 2; > const short TYPE_PROGRESSMETER = 3; > + const short TYPE_RENDERER = 4; I think you should keep a blank line between the constants and the associated type. But that's just my opinion; if others disagree, go with what they say. ::: layout/xul/tree/nsTreeBodyFrame.cpp @@ +3305,5 @@ > + nsStyleContext* rendererContext = GetPseudoStyleContext(nsCSSAnonBoxes::moztreerenderer); > + bool useImageRegion = true; > + nsCOMPtr<imgIContainer> image; > + if (mCellRenderer) { > + nsAutoString properties = nsTreeUtils::StringizeProperties(mScratchArray); I really think this is unnecessary: you already have properties and mScratchArray established in PaintCell. See http://hg.mozilla.org/mozilla-central/annotate/2255d7d187b2/layout/xul/tree/nsTreeBodyFrame.cpp#l3137 . Neither existing code nor your patch changes properties or mScratchArray. @@ +3312,5 @@ > + if (image) { > + useImageRegion = false; > + } else { > + GetImage(aRowIndex, aColumn, true, rendererContext, useImageRegion, getter_AddRefs(image)); > + } What happens if the Draw call throws an exception? What happens if it doesn't set an image as you expect it to? You're going to fall into the GetImage code, and it's not entirely clear that's what you want. @@ +3434,5 @@ > NS_PRECONDITION(aColumn && aColumn->GetFrame(), "invalid column passed"); > > bool isRTL = StyleVisibility()->mDirection == NS_STYLE_DIRECTION_RTL; > nscoord rightEdge = aCurrX + aRemainingWidth; > + nsStyleContext* imageContext = aStyleContext; I don't understand the purpose of this pointer, since you've already established the pointer in the function's arguments. I think you should add some NS_PRECONDITION lines, in case we get a null pointer for arguments you think you have. (But since we'd crash anyway on a null dereference, maybe not.) @@ +3447,5 @@ > imageContext->StyleMargin()->GetMargin(imageMargin); > imageRect.Deflate(imageMargin); > > + bool useImageRegion = aUseImageRegion; > + nsCOMPtr<imgIContainer> image = aImage; I understand this one even less. You certainly don't need a nsCOMPtr here if you've already got the image. Are you trying to avoid renaming variables in the function? @@ +4757,5 @@ > +{ > + nsWeakFrame weakFrame(this); > + NS_ENSURE_STATE(weakFrame.IsAlive()); > + nsCOMPtr<nsITreeCellRenderer> renderer = mCellRenderer; > + renderer.forget(aCellRenderer); *aCellRenderer = mCellRenderer Then you don't need the local nsCOMPtr. ::: layout/xul/tree/nsTreeBodyFrame.h @@ +634,5 @@ > nsTHashtable<nsPtrHashKey<nsTreeImageListener> > mCreatedListeners; > > + // This is a external tree cell render, it's will rendering the cell > + // content, and resulting imgIContainer, the rendering progress will > + // be canvas or other ways. I don't understand this comment. ::: layout/xul/tree/nsTreeBoxObject.cpp @@ +508,5 @@ > + if (body) { > + return body->SetCellRenderer(aCellRenderer); > + } > + return NS_OK; > +} Are we missing NS_ENSURE_ARG_POINTER(aCellRenderer); on the getter? ::: layout/xul/tree/nsTreeColumns.cpp @@ +317,5 @@ > > // Figure out our column type. Default type is text. > mType = nsITreeColumn::TYPE_TEXT; > static nsIContent::AttrValuesArray typestrings[] = > + {&nsGkAtoms::checkbox, &nsGkAtoms::progressmeter, &nsGkAtoms::renderer, nullptr}; Consider breaking this up into multiple lines: { &nsGkAtoms::checkbox, &nsGkAtoms::progressmeter, &nsGkAtoms::renderer, nullptr }; That would make it easier to add new types later. ::: layout/xul/tree/nsTreeUtils.cpp @@ +51,5 @@ > return NS_OK; > } > > +nsAutoString > +nsTreeUtils::StringizeProperties(const AtomArray& aPropertiesArray) As I mention above, I don't see how you need this method.
Attachment #8484232 - Flags: review?(ajvincent) → feedback-
Attachment #8482308 - Flags: feedback?(ajvincent)
(In reply to Alex Vincent [:WeirdAl] from comment #146) > Comment on attachment 8484232 [details] [diff] [review] > cellRenderer.diff > > Review of attachment 8484232 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'd be much more comfortable with this if you were to include some sample > XUL+JS that exercises this code. (A reftest would be fantastic, but I > wouldn't expect that. A crashtest would be better than no test at all.) > The patch has been through a number of changes since the initial proposal, > including the API. > > In particular I want to see an example implementation of the > nsITreeCellRenderer, and how to use it. That will help me shape my thoughts > on the API. > > ::: layout/xul/tree/nsITreeBoxObject.idl > @@ +39,5 @@ > > attribute nsITreeView view; > > > > /** > > + * The canvas handler for rich cell, to rendering anything that canvs > > + * support, we can use this to rendering beatiful tree cell. > > Spelling typos: canvas, beautiful. > > ::: layout/xul/tree/nsITreeColumns.idl > @@ +31,5 @@ > > > > const short TYPE_TEXT = 1; > > const short TYPE_CHECKBOX = 2; > > const short TYPE_PROGRESSMETER = 3; > > + const short TYPE_RENDERER = 4; > > I think you should keep a blank line between the constants and the > associated type. But that's just my opinion; if others disagree, go with > what they say. > > ::: layout/xul/tree/nsTreeBodyFrame.cpp > @@ +3305,5 @@ > > + nsStyleContext* rendererContext = GetPseudoStyleContext(nsCSSAnonBoxes::moztreerenderer); > > + bool useImageRegion = true; > > + nsCOMPtr<imgIContainer> image; > > + if (mCellRenderer) { > > + nsAutoString properties = nsTreeUtils::StringizeProperties(mScratchArray); > > I really think this is unnecessary: you already have properties and > mScratchArray established in PaintCell. See > http://hg.mozilla.org/mozilla-central/annotate/2255d7d187b2/layout/xul/tree/ > nsTreeBodyFrame.cpp#l3137 . Neither existing code nor your patch changes > properties or mScratchArray. I need to pass properties to external render, so that the render can rendering things accoriding to the properties . > > @@ +3312,5 @@ > > + if (image) { > > + useImageRegion = false; > > + } else { > > + GetImage(aRowIndex, aColumn, true, rendererContext, useImageRegion, getter_AddRefs(image)); > > + } > > What happens if the Draw call throws an exception? What happens if it > doesn't set an image as you expect it to? You're going to fall into the > GetImage code, and it's not entirely clear that's what you want. If there is a exception, then would not handle it, because the exception will inform the user/programmer that the draw code is implemented wrong. and the direct result will cause the drawing to be blank or abnormal, that what we want. > > @@ +3434,5 @@ > > NS_PRECONDITION(aColumn && aColumn->GetFrame(), "invalid column passed"); > > > > bool isRTL = StyleVisibility()->mDirection == NS_STYLE_DIRECTION_RTL; > > nscoord rightEdge = aCurrX + aRemainingWidth; > > + nsStyleContext* imageContext = aStyleContext; > > I don't understand the purpose of this pointer, since you've already > established the pointer in the function's arguments. for the purpose don't rename at multiple place. > > I think you should add some NS_PRECONDITION lines, in case we get a null > pointer for arguments you think you have. (But since we'd crash anyway on a > null dereference, maybe not.) > > @@ +3447,5 @@ > > imageContext->StyleMargin()->GetMargin(imageMargin); > > imageRect.Deflate(imageMargin); Yeap, we need to do a ASSERT, because we can be sure this would not be NULL. > > > > + bool useImageRegion = aUseImageRegion; > > + nsCOMPtr<imgIContainer> image = aImage; > > I understand this one even less. You certainly don't need a nsCOMPtr here > if you've already got the image. Are you trying to avoid renaming variables > in the function? for the purpose don't rename at multiple place. > > @@ +4757,5 @@ > > +{ > > + nsWeakFrame weakFrame(this); > > + NS_ENSURE_STATE(weakFrame.IsAlive()); > > + nsCOMPtr<nsITreeCellRenderer> renderer = mCellRenderer; > > + renderer.forget(aCellRenderer); > > *aCellRenderer = mCellRenderer > > Then you don't need the local nsCOMPtr. Are you sure there won't be memory leaks? > > ::: layout/xul/tree/nsTreeBodyFrame.h > @@ +634,5 @@ > > nsTHashtable<nsPtrHashKey<nsTreeImageListener> > mCreatedListeners; > > > > + // This is a external tree cell render, it's will rendering the cell > > + // content, and resulting imgIContainer, the rendering progress will > > + // be canvas or other ways. > > I don't understand this comment. Yeap, we need a javascript demo for this. > > ::: layout/xul/tree/nsTreeBoxObject.cpp > @@ +508,5 @@ > > + if (body) { > > + return body->SetCellRenderer(aCellRenderer); > > + } > > + return NS_OK; > > +} > > Are we missing NS_ENSURE_ARG_POINTER(aCellRenderer); on the getter? > > ::: layout/xul/tree/nsTreeColumns.cpp > @@ +317,5 @@ > > > > // Figure out our column type. Default type is text. > > mType = nsITreeColumn::TYPE_TEXT; > > static nsIContent::AttrValuesArray typestrings[] = > > + {&nsGkAtoms::checkbox, &nsGkAtoms::progressmeter, &nsGkAtoms::renderer, nullptr}; > > Consider breaking this up into multiple lines: > > { > &nsGkAtoms::checkbox, > &nsGkAtoms::progressmeter, > &nsGkAtoms::renderer, > nullptr > }; > > That would make it easier to add new types later. > > ::: layout/xul/tree/nsTreeUtils.cpp > @@ +51,5 @@ > > return NS_OK; > > } > > > > +nsAutoString > > +nsTreeUtils::StringizeProperties(const AtomArray& aPropertiesArray) > > As I mention above, I don't see how you need this method.
> > I really think this is unnecessary: you already have properties and > mScratchArray established in PaintCell. See > http://hg.mozilla.org/mozilla-central/annotate/2255d7d187b2/layout/xul/tree/ > nsTreeBodyFrame.cpp#l3137 . Neither existing code nor your patch changes > properties or mScratchArray. > The mScratchArray is a join set of Cell/Row/Column, and the properties is just for the specific Cell that's why I need to convert mScratchArray back to nsAutoString and pass it to the drawing code.
If you're worried about the correct Get/Set pattern in C++ XPCOM, take a look at this: http://mxr.mozilla.org/mozilla-release/source/parser/xml/src/nsSAXXMLReader.cpp#356 I don't think SAXXMLReader will be converted to WebIDl anytime soon... :)
Comment on attachment 8482308 [details] [diff] [review] 0002-Reshape-The-API-to-looking-for-suggestion.patch Sorry for the delay in responding to this review. Unfortunately I'm not sure what I'm supposed to be reviewing here because this diff is not against mozilla-central.
Is there a description of what it is you're doing? I'd suggest putting it in a wiki, since this bug is too long to find anything in. (And how does it differ from what Paul was doing?) Also, is this a patch against mozilla-central or against ESR31? If the latter, please submit patches against central for review, not ESR31.
Flags: needinfo?(luoyonggang)
(In reply to Alex Vincent [:WeirdAl] from comment #146) > @@ +4757,5 @@ > > +{ > > + nsWeakFrame weakFrame(this); > > + NS_ENSURE_STATE(weakFrame.IsAlive()); > > + nsCOMPtr<nsITreeCellRenderer> renderer = mCellRenderer; > > + renderer.forget(aCellRenderer); > > *aCellRenderer = mCellRenderer > > Then you don't need the local nsCOMPtr. I don't think that's correct, since your approach doesn't do an addref.
Comment on attachment 8484232 [details] [diff] [review] cellRenderer.diff Review of attachment 8484232 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for continuing to push forward! I'll have to r- this just because of the imgITools changes. I'm willing to reconsider if you explain why it's necessary, though. Also, I'm frankly starting to get confused about what's happening here. I think all the reviewers will find this much easier to understand if you rebase the code you have on top of mozilla-central tip. Furthermore, it'd be helpful if you divide your code into smaller, logically related patches. This seems like it's just a diff from the previous patch, although I'm not sure, and that's pretty confusing to review in a bug as long as this. ::: image/public/imgITools.idl @@ +90,5 @@ > in long aWidth, > in long aHeight, > [optional] in AString outputOptions); > > + imgIContainer createImageFromCanvasContext(in nsISupports canvasContext); Why are you adding this? If you don't have a very good reason to do this from JavaScript, then you should not be adding this to imgITools. If you're only calling this from C++, then ImageOps would be the place to add it, but if there's only one caller you shouldn't even do that. ::: layout/xul/tree/nsITreeColumns.idl @@ +31,5 @@ > > const short TYPE_TEXT = 1; > const short TYPE_CHECKBOX = 2; > const short TYPE_PROGRESSMETER = 3; > + const short TYPE_RENDERER = 4; I agree with Alex; restore the blank line that used to be here. ::: layout/xul/tree/nsTreeBodyFrame.h @@ +117,5 @@ > nsresult BeginUpdateBatch(); > nsresult EndUpdateBatch(); > nsresult ClearStyleAndImageCaches(); > + nsresult GetCellRenderer(nsITreeCellRenderer ** aCellRenderer); > + nsresult SetCellRenderer(nsITreeCellRenderer * aCellRenderer); This doesn't match the style guide. Please write |nsITreeCellRenderer*|, with no space between the type name and the star. ::: layout/xul/tree/nsTreeBoxObject.cpp @@ +491,5 @@ > return NS_OK; > } > > +NS_IMETHODIMP > +nsTreeBoxObject::GetCellRenderer(nsITreeCellRenderer ** aCellRenderer) This violates the style guide; please use |nsITreeCellRender**|, with no space. @@ +497,5 @@ > + nsTreeBodyFrame* body = GetTreeBody(); > + if (body) { > + return body->GetCellRenderer(aCellRenderer); > + } > + return NS_OK; Should you return NS_OK if GetTreeBody() returns null? I'm not saying you shouldn't, but make sure you've thought about it. (This and the previous comment also apply to SetCellRenderer.) ::: layout/xul/tree/nsTreeUtils.cpp @@ +60,5 @@ > + properties += NS_LITERAL_STRING(" "); > + } > + properties += aPropertiesArray[i]->GetUTF16String(); > + } > + return std::move(properties); Don't use |std::move| in Mozilla code. You need the |Move()| function from |mozilla/Move.h|. (You'd have found this out eventually anyway, because this won't build on all platforms.)
Attachment #8484232 - Flags: review?(seth) → review-
(In reply to Alex Vincent [:WeirdAl] from comment #149) > If you're worried about the correct Get/Set pattern in C++ XPCOM, take a > look at this: > http://mxr.mozilla.org/mozilla-release/source/parser/xml/src/nsSAXXMLReader. > cpp#356 > > I don't think SAXXMLReader will be converted to WebIDl anytime soon... :) Please don't suggest that people use NS_IF_ADDREF.
Is there any easy way to submit a series of patches? Bugzilla limit only to submit a patch.
Flags: needinfo?(luoyonggang)
Attached patch tree.zip (obsolete) — Splinter Review
Update with all comments. The xul demo is coming.
Attachment #8495489 - Flags: superreview?(seth)
Restoring needinfo? from comment 151. (In reply to Yonggang Luo from comment #155) > Is there any easy way to submit a series of patches? > Bugzilla limit only to submit a patch. You can either: (1) use the "Create an attachment" form multiple times (2) use the bzexport mercurial extension (easier)
Flags: needinfo?(luoyonggang)
Attached file richtree.zip
The richtree test case.
Attachment #8474218 - Attachment is obsolete: true
Attachment #8474221 - Attachment is obsolete: true
Attachment #8477845 - Attachment is obsolete: true
Attachment #8482308 - Attachment is obsolete: true
Attachment #8484232 - Attachment is obsolete: true
Attachment #8495489 - Attachment is obsolete: true
Attachment #8482308 - Flags: review?(neil)
Attachment #8484232 - Flags: review?(dbaron)
Attachment #8484232 - Flags: review?(Sevenspade)
Attachment #8495489 - Flags: superreview?(seth)
Flags: needinfo?(luoyonggang)
Complete patches set with crashtest.
Attachment #8496482 - Flags: superreview?(dbaron)
Attachment #8496482 - Flags: review?(seth)
Attachment #8496482 - Flags: review?(neil)
Attachment #8496482 - Flags: review?(dbaron)
Attachment #8496482 - Flags: review?(ajvincent)
Attachment #8496482 - Flags: review?(Sevenspade)
Attachment #8496483 - Flags: superreview?(dbaron)
Attachment #8496483 - Flags: review?(seth)
Attachment #8496483 - Flags: review?(neil)
Attachment #8496483 - Flags: review?(dbaron)
Attachment #8496483 - Flags: review?(ajvincent)
Attachment #8496483 - Flags: review?(Sevenspade)
Attachment #8496484 - Flags: superreview?(dbaron)
Attachment #8496484 - Flags: review?(seth)
Attachment #8496484 - Flags: review?(neil)
Attachment #8496484 - Flags: review?(dbaron)
Attachment #8496484 - Flags: review?(ajvincent)
Attachment #8496484 - Flags: review?(Sevenspade)
Attachment #8496485 - Flags: superreview?(dbaron)
Attachment #8496485 - Flags: review?(seth)
Attachment #8496485 - Flags: review?(neil)
Attachment #8496485 - Flags: review?(dbaron)
Attachment #8496485 - Flags: review?(ajvincent)
Attachment #8496485 - Flags: review?(Sevenspade)
Attachment #8496485 - Attachment is obsolete: true
Attachment #8496485 - Flags: superreview?(dbaron)
Attachment #8496485 - Flags: review?(seth)
Attachment #8496485 - Flags: review?(neil)
Attachment #8496485 - Flags: review?(dbaron)
Attachment #8496485 - Flags: review?(ajvincent)
Attachment #8496485 - Flags: review?(Sevenspade)
Attachment #8496486 - Flags: superreview?(dbaron)
Attachment #8496486 - Flags: review?(seth)
Attachment #8496486 - Flags: review?(neil)
Attachment #8496486 - Flags: review?(dbaron)
Attachment #8496486 - Flags: review?(ajvincent)
Attachment #8496486 - Flags: review?(Sevenspade)
Attachment #8496487 - Flags: superreview?(dbaron)
Attachment #8496487 - Flags: review?(seth)
Attachment #8496487 - Flags: review?(neil)
Attachment #8496487 - Flags: review?(dbaron)
Attachment #8496487 - Flags: review?(ajvincent)
Attachment #8496487 - Flags: review?(Sevenspade)
Attachment #8496488 - Flags: superreview?(dbaron)
Attachment #8496488 - Flags: review?(seth)
Attachment #8496488 - Flags: review?(neil)
Attachment #8496488 - Flags: review?(dbaron)
Attachment #8496488 - Flags: review?(ajvincent)
Attachment #8496488 - Flags: review?(Sevenspade)
I can't even start on reviewing this, or figuring out an appropriate alternate reviewer, until you answer comment 151.
Flags: needinfo?(luoyonggang)
Also, you should generally not request review from 5 people per patch unless you have a very good reason.
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #151) > Is there a description of what it is you're doing? I'd suggest putting it > in a wiki, since this bug is too long to find anything in. (And how does it > differ from what Paul was doing?) > > Also, is this a patch against mozilla-central or against ESR31? If the > latter, please submit patches against central for review, not ESR31. Er. it's not against ESR31, it's against tip. I've state it before.
Flags: needinfo?(luoyonggang)
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #167) > Also, you should generally not request review from 5 people per patch unless > you have a very good reason. They reviewed my code before.
Is there a description of what it is you're doing? I'd suggest putting it in a wiki, since this bug is too long to find anything in. (And how does it differ from what Paul was doing?)
Flags: needinfo?(luoyonggang)
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #170) > Is there a description of what it is you're doing? I'd suggest putting it > in a wiki, since this bug is too long to find anything in. (And how does it > differ from what Paul was doing?) Gotcha, I will document it, can you give me a wiki page to document it? At the current time, the richtree.xul demo is the best document
Flags: needinfo?(luoyonggang)
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #170) > Is there a description of what it is you're doing? I'd suggest putting it > in a wiki, since this bug is too long to find anything in. (And how does it > differ from what Paul was doing?) The main difference is that Paul is trying to support multiline support in C++ code, and have little customize possibility, and at that time, we do not have 'Canvas', and now, I was trying to treat a Tree Cell as a canvas, and rendering it through Javascript Code, and I can feed the Canvas with the Tree Cell/Row data, so we can rendering the Cell in any form we want. The Core API is like following: The renderer hook interface: [scriptable, uuid(aceed5f8-e57a-4cdc-b859-cf864904ab1c)] interface nsITreeCellRenderer : nsISupports { /** * Rendering a tree cell's content at the position (row,col), returning * the imgIContainer of the drawn result. */ imgIContainer draw(in nsITreeView view, in long row, in nsITreeColumn col, in AString props); }; The renderer hook [scriptable, uuid(64ba5199-c4f4-4498-bbdc-f8e4c369086c)] interface nsITreeBoxObject : nsISupports { /** * The canvas handler for rich cell, to rendering anything that canvs * support, we can use this to rendering beatiful tree cell. */ attribute nsITreeCellRenderer cellRenderer; } In Javascript: document.getElementById('tree').treeBoxObject.cellRenderer = { // nsISupports QueryInterface: function(aIID) { if (aIID.equals(Ci.nsITreeCellRenderer) || aIID.equals(Ci.nsISupports) || aIID.equals(Ci.nsISupportsWeakReference)) return this; throw Components.results.NS_NOINTERFACE; }, //imgIContainer (ctx) render(in nsITreeView view, in long row, in nsITreeColumn col, in AString props); draw: function(view, row, col, props) { let canvas = window.document.createElementNS("http://www.w3.org/1999/xhtml", "canvas"); let ctx = this.canvas.getContext("2d"); /* Drawing things with ctx */ let imageTools = Cc["@mozilla.org/image/tools;1"]. getService(Ci.imgITools); /* Putting the canvas result into Tree Cell by draw API */ return imageTools.createImageFromCanvasContext(ctx); } } In C++: We have alreayd have DrawImage API, so use it to draw the imgIContainer directly, little modifcation to C++ code.
Forgot to update the UUID of nsITreeBoxObject The main difference is: Paul is trying to support multiline support in C++ code, and can not be customize use Javascript, and at that time, we do not have 'canvas', I was trying to treat a `Tree Cell` as a `Canvas`, and rendering it through Javascript Code, and I can feed the `Canvas` with the Tree Cell/Row data, so we can rendering the Cell in any form we want. The Core API is like following: The renderer hook interface: [scriptable, uuid(aceed5f8-e57a-4cdc-b859-cf864904ab1c)] interface nsITreeCellRenderer : nsISupports { /** * Rendering a tree cell's content at the position (row,col), returning * the imgIContainer of the drawn result. */ imgIContainer draw(in nsITreeView view, in long row, in nsITreeColumn col, in AString props); }; The renderer hook: [scriptable, uuid(64ba5199-c4f4-4498-bbdc-f8e4c369086c)] interface nsITreeBoxObject : nsISupports { /** * The canvas handler for rich cell, to rendering anything that canvs * support, we can use this to rendering beatiful tree cell. */ attribute nsITreeCellRenderer cellRenderer; } In Javascript: document.getElementById('tree').treeBoxObject.cellRenderer = { // nsISupports QueryInterface: function(aIID) { if (aIID.equals(Ci.nsITreeCellRenderer) || aIID.equals(Ci.nsISupports) || aIID.equals(Ci.nsISupportsWeakReference)) return this; throw Components.results.NS_NOINTERFACE; }, //imgIContainer (ctx) render(in nsITreeView view, in long row, in nsITreeColumn col, in AString props); draw: function(view, row, col, props) { let canvas = window.document.createElementNS("http://www.w3.org/1999/xhtml", "canvas"); let ctx = this.canvas.getContext("2d"); /* Drawing things with ctx */ let imageTools = Cc["@mozilla.org/image/tools;1"]. getService(Ci.imgITools); /* Putting the canvas result into Tree Cell by draw API */ return imageTools.createImageFromCanvasContext(ctx); } } In C++: The TreeBoxObject already have PaintImage api, because we return imgIContainer directly, so we can reuse this API to rendering our Canvas context to the surface.
Attachment #8496487 - Attachment is obsolete: true
Attachment #8496488 - Attachment is obsolete: true
Attachment #8496487 - Flags: superreview?(dbaron)
Attachment #8496487 - Flags: review?(seth)
Attachment #8496487 - Flags: review?(neil)
Attachment #8496487 - Flags: review?(dbaron)
Attachment #8496487 - Flags: review?(ajvincent)
Attachment #8496487 - Flags: review?(Sevenspade)
Attachment #8496488 - Flags: superreview?(dbaron)
Attachment #8496488 - Flags: review?(seth)
Attachment #8496488 - Flags: review?(neil)
Attachment #8496488 - Flags: review?(dbaron)
Attachment #8496488 - Flags: review?(ajvincent)
Attachment #8496488 - Flags: review?(Sevenspade)
Attachment #8496494 - Flags: superreview?(dbaron)
Attachment #8496494 - Flags: review?(seth)
Attachment #8496494 - Flags: review?(neil)
Attachment #8496494 - Flags: review?(dbaron)
Attachment #8496494 - Flags: review?(ajvincent)
Comment on attachment 8496482 [details] [diff] [review] 0001-Refinement-the-nsTreeBodyFrame-GetImage-to-reduce-th.patch Seems reasonable to me.
Attachment #8496482 - Flags: review?(neil) → review+
Attachment #8496483 - Flags: review?(neil)
Comment on attachment 8496482 [details] [diff] [review] 0001-Refinement-the-nsTreeBodyFrame-GetImage-to-reduce-th.patch The review field is for asking module owners or module peers whether the patch is good for check-in. <https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_reviewed> The best I can give is some non-authoritative comments, but I'm not even all that involved with Mozilla nowadays, so... I'll say this, though: It looks like you are frustrated with Bugzilla based on your experience with this bug. The bug has a lot of history and activity and an unclear focus. (See comment 69.) If you get much more frustrated with it, I don't think anyone would be too upset with you if you created a new bug, then marked it as blocking this one. If you do that, make sure you CC some appropriate people and leave a comment here pointing to the new bug. In the new bug you'll want to: a) summarize the problem in the first comment, b) attach your patches there, and optionally c) explain alternative approaches you are considering and why You'll probably benefit from it, and I don't speak for anybody else, but I'll bet dbaron and a few others who have a similar perspective would have an easier time with it, too.
Attachment #8496482 - Flags: review?(Sevenspade)
Comment on attachment 8496482 [details] [diff] [review] 0001-Refinement-the-nsTreeBodyFrame-GetImage-to-reduce-th.patch Review of attachment 8496482 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I don't think you need review from 5 people for this particular patch; it's pretty simple. I've unset the review flags for everyone other than me and Neil.
Attachment #8496482 - Flags: superreview?(dbaron)
Attachment #8496482 - Flags: review?(seth)
Attachment #8496482 - Flags: review?(dbaron)
Attachment #8496482 - Flags: review?(ajvincent)
Attachment #8496482 - Flags: review+
I do hope that this patch will give an opening to an easy solution of bug 213945 https://bugzilla.mozilla.org/show_bug.cgi?id=213945 "Mail/message listing/thread pane needs more organization in 3 vertical pane view (like Outlook, Lotus Notes et al.)". This bug was filed in 2003!
Comment on attachment 8496483 [details] [diff] [review] 0002-Add-imgIContainer-createImageFromCanvasContext-in-ns.patch Review of attachment 8496483 [details] [diff] [review]: ----------------------------------------------------------------- I'm the only reviewer you need on an imagelib patch. I've unset the other review flags. It seems that you're adding |createImageFromCanvasContext| in order to support the |nsITreeCellRenderer.draw| API you've added. That seems reasonable to me, but I need to think a little about this API before I r+ this. I've added some initial review comments, though. ::: image/public/imgITools.idl @@ +90,5 @@ > in long aWidth, > in long aHeight, > [optional] in AString outputOptions); > > + imgIContainer createImageFromCanvasContext(in nsISupports canvasContext); This shouldn't take nsISupports. It needs to take a real canvas interface. You also need a documentation comment here. ::: image/src/imgTools.cpp @@ +311,5 @@ > } > > NS_IMETHODIMP > +imgTools::CreateImageFromCanvasContext(nsISupports *aCanvasContext, > + imgIContainer **aContainer) Again, this violates the style guide. Please write |nsISupports*| and |imgIContainer**|, with no space between the type name and the '*'. @@ +316,5 @@ > +{ > + nsCOMPtr<nsICanvasRenderingContextInternal> contextInternal = > + do_QueryInterface(aCanvasContext); > + if (!contextInternal) { > + return NS_OK; Should you really be returning NS_OK if the caller passed in an argument of the wrong type? (Though you won't need this check anyway if you fix this method to not take an nsISupports.) @@ +318,5 @@ > + do_QueryInterface(aCanvasContext); > + if (!contextInternal) { > + return NS_OK; > + } > + RefPtr<mozilla::gfx::SourceSurface> surface = You don't need "mozilla::gfx::", as we have "using mozilla::gfx" at the top of this file. Just write "SourceSurface".
Attachment #8496483 - Flags: superreview?(dbaron)
Attachment #8496483 - Flags: review?(dbaron)
Attachment #8496483 - Flags: review?(ajvincent)
Comment on attachment 8496484 [details] [diff] [review] 0003-Add-a-cell-column-row-type-TYPE_RENDERER-and-corresp.patch Review of attachment 8496484 [details] [diff] [review]: ----------------------------------------------------------------- Seems OK, though this absolutely needs an r+ from someone with more XUL knowledge.
Attachment #8496484 - Flags: review?(seth) → review+
Attachment #8496486 - Flags: review?(seth) → review+
Comment on attachment 8496494 [details] [diff] [review] 0005-Fixes-for-bug-441414-to-add-rich-tree-support-crasht.patch Review of attachment 8496494 [details] [diff] [review]: ----------------------------------------------------------------- The image-related code in this patch looks good! I'm r+'ing that aspect of it. There are some minor issues, which I've pointed out below, but nothing that should be hard to fix. I'm not reviewing the overall design you're using here and the tests. Someone more familiar with XUL code needs to do that. If you end up having to make substantial changes to the design, I'll need to review again. ::: layout/xul/tree/nsITreeBoxObject.idl @@ +19,5 @@ > + /** > + * Rendering a tree cell's content at the position (row,col), returning > + * the imgIContainer of the drawn result. > + */ > + imgIContainer draw(in nsITreeView view, in long row, in nsITreeColumn col, in AString props); Is there seriously no better way to pass these properties than as a space-delimited string? I'm hoping someone who works with this code more can suggest a better alternative. Also, this comment could use rewording, and it definitely needs '@param' explanations for each parameter and an '@return' explanation of the return value. (Read up on Doxygen comments if you're not familiar with this.) Also, a nit: it looks like the '*' on the lines you've added is aligned to the '/' in the '/*' on the first line. They need to be indented by one space; all the stars should be aligned vertically. @@ +39,5 @@ > attribute nsITreeView view; > > /** > + * The canvas handler for rich cell, to rendering anything that canvs > + * support, we can use this to rendering beatiful tree cell. Same nits as the other Doxygen comment in this file: this needs rewording, and it looks like the '*'s are aligned wrong. ::: layout/xul/tree/nsTreeBodyFrame.cpp @@ +3264,5 @@ > + bool useImageRegion = true; > + nsCOMPtr<imgIContainer> image; > + GetImage(aRowIndex, aColumn, false, imageContext, useImageRegion, getter_AddRefs(image)); > + PaintImage(image, aColumn, iconRect, aPresContext, aRenderingContext, > + imageContext, aDirtyRect, remainingWidth, currX, useImageRegion); Nit: |imageContext| should be aligned with |image|. @@ +3309,5 @@ > + nsAutoString properties = nsTreeUtils::StringizeProperties(mScratchArray); > + mCellRenderer->Draw(mView, aRowIndex, aColumn, properties, getter_AddRefs(image)); > + } > + if (image) { > + useImageRegion = false; It seems like it'd make more sense to default |useImageRegion| to |false|. If you get an image from calling |mCellRenderer->Draw|, then it'll already be set to the correct value, and you won't need this line. If you don't, you'll call |GetImage()|, and that will set it to the correct value, because |GetImage()| always sets the value of |useImageRegion|. @@ +3314,5 @@ > + } else { > + GetImage(aRowIndex, aColumn, true, rendererContext, useImageRegion, getter_AddRefs(image)); > + } > + PaintImage(image, aColumn, iconRect, aPresContext, aRenderingContext, > + rendererContext, aDirtyRect, remainingWidth, currX, useImageRegion); Nit: It looks like |renderContext| needs to be aligned with |image| on this line. @@ +3447,5 @@ > imageContext->StyleMargin()->GetMargin(imageMargin); > imageRect.Deflate(imageMargin); > > + bool useImageRegion = aUseImageRegion; > + nsCOMPtr<imgIContainer> image = aImage; Don't copy these argument values. Just replace |useImageRegion| by |aUseImageRegion| and |image| by |aImage| in the rest of the method. ::: layout/xul/tree/nsTreeBodyFrame.h @@ +634,5 @@ > nsTHashtable<nsPtrHashKey<nsTreeImageListener> > mCreatedListeners; > > + // This is a external tree cell render, it's will rendering the cell > + // content, and resulting imgIContainer, the rendering progress will > + // be canvas or other ways. This is another comment that needs rewording. I hesitate to make suggestions as the XUL folks may know better, but maybe something like: "This is an external tree cell renderer. If present, it renders the cell content into an imgIContainer, which is then painted using PaintImage."
Attachment #8496494 - Flags: review?(seth) → review+
(In reply to Seth Fowler [:seth] from comment #180) > > + imgIContainer draw(in nsITreeView view, in long row, in nsITreeColumn col, in AString props); > > Is there seriously no better way to pass these properties than as a > space-delimited string? > > I'm hoping someone who works with this code more can suggest a better > alternative. The properties are communicated to the nsTreeBodyFrame from the view as a space-delimited string. That's a recent development. Trees used to use an nsISupportsArray, and was changed for bug 407956. (See bug 407956, comment 6, in particular.) The renderer implementor is almost definitely the same as the nsITreeView implementor, so they should be familiar with this, having been the one to provide the property string to begin with (minus the prefilled properties). It's wonky, and something like DOMTokenList would be better, but only if nsITreeView.getCellProperties and friends used same data structure, which is starting to sound like it should live in another bug. > > + // This is a external tree cell render, it's will rendering the cell > > + // content, and resulting imgIContainer, the rendering progress will > > + // be canvas or other ways. > > This is another comment that needs rewording. I hesitate to make suggestions > as the XUL folks may know better, but maybe something like: > > "This is an external tree cell renderer. If present, it renders the cell > content into an imgIContainer, which is then painted using PaintImage." That works. Optionally, maybe emphasize that the rendered result is used in lieu of what nsTreeBodyFrame would have rendered itself based on what it learned from the view via |getCellText| and friends.
(In reply to Seth Fowler [:seth] from comment #178) > Comment on attachment 8496483 [details] [diff] [review] > 0002-Add-imgIContainer-createImageFromCanvasContext-in-ns.patch > > Review of attachment 8496483 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm the only reviewer you need on an imagelib patch. I've unset the other > review flags. > > It seems that you're adding |createImageFromCanvasContext| in order to > support the |nsITreeCellRenderer.draw| API you've added. > > That seems reasonable to me, but I need to think a little about this API > before I r+ this. > > I've added some initial review comments, though. > > ::: image/public/imgITools.idl > @@ +90,5 @@ > > in long aWidth, > > in long aHeight, > > [optional] in AString outputOptions); > > > > + imgIContainer createImageFromCanvasContext(in nsISupports canvasContext); > > This shouldn't take nsISupports. It needs to take a real canvas interface. We discussed before, canvasContext is a better option, Please take a look at comment https://bugzilla.mozilla.org/show_bug.cgi?id=441414#c142, There is another reason not use canvas directly, if we use nsIDOMHTMLCanvasElement, we also need to pass a annoy parameter contextName ('2d' or '3d'), and in fact, when we drawing things on the context, the result thing(context) is what we need to paint to the surface, not the Canvas. Please check this carefully. > > You also need a documentation comment here. > > ::: image/src/imgTools.cpp > @@ +311,5 @@ > > } > > > > NS_IMETHODIMP > > +imgTools::CreateImageFromCanvasContext(nsISupports *aCanvasContext, > > + imgIContainer **aContainer) > > Again, this violates the style guide. Please write |nsISupports*| and > |imgIContainer**|, with no space between the type name and the '*'. > > @@ +316,5 @@ > > +{ > > + nsCOMPtr<nsICanvasRenderingContextInternal> contextInternal = > > + do_QueryInterface(aCanvasContext); > > + if (!contextInternal) { > > + return NS_OK; > > Should you really be returning NS_OK if the caller passed in an argument of > the wrong type? (Though you won't need this check anyway if you fix this > method to not take an nsISupports.) If we pass the canvas interface, we still will got the nsISupports for nsICanvasRenderingContextInternal, this won't bypassed, because the image is created from nsICanvasRenderingContextInternal directly, not from canvas. > > @@ +318,5 @@ > > + do_QueryInterface(aCanvasContext); > > + if (!contextInternal) { > > + return NS_OK; > > + } > > + RefPtr<mozilla::gfx::SourceSurface> surface = > > You don't need "mozilla::gfx::", as we have "using mozilla::gfx" at the top > of this file. Just write "SourceSurface". OK
Flags: needinfo?(seth)
(In reply to Seth Fowler [:seth] from comment #180) > Comment on attachment 8496494 [details] [diff] [review] > 0005-Fixes-for-bug-441414-to-add-rich-tree-support-crasht.patch > > Review of attachment 8496494 [details] [diff] [review]: > ----------------------------------------------------------------- > > The image-related code in this patch looks good! I'm r+'ing that aspect of > it. There are some minor issues, which I've pointed out below, but nothing > that should be hard to fix. > > I'm not reviewing the overall design you're using here and the tests. > Someone more familiar with XUL code needs to do that. > > If you end up having to make substantial changes to the design, I'll need to > review again. > > ::: layout/xul/tree/nsITreeBoxObject.idl > @@ +19,5 @@ > > + /** > > + * Rendering a tree cell's content at the position (row,col), returning > > + * the imgIContainer of the drawn result. > > + */ > > + imgIContainer draw(in nsITreeView view, in long row, in nsITreeColumn col, in AString props); > > Is there seriously no better way to pass these properties than as a > space-delimited string? As :crussell state, now TreeView are not using AtomArray to pass parameters. But using space delimited string instead. > > I'm hoping someone who works with this code more can suggest a better > alternative. > > Also, this comment could use rewording, and it definitely needs '@param' > explanations for each parameter and an '@return' explanation of the return > value. (Read up on Doxygen comments if you're not familiar with this.) > > Also, a nit: it looks like the '*' on the lines you've added is aligned to > the '/' in the '/*' on the first line. They need to be indented by one > space; all the stars should be aligned vertically. > > @@ +39,5 @@ > > attribute nsITreeView view; > > > > /** > > + * The canvas handler for rich cell, to rendering anything that canvs > > + * support, we can use this to rendering beatiful tree cell. > > Same nits as the other Doxygen comment in this file: this needs rewording, > and it looks like the '*'s are aligned wrong. > > ::: layout/xul/tree/nsTreeBodyFrame.cpp > @@ +3264,5 @@ > > + bool useImageRegion = true; > > + nsCOMPtr<imgIContainer> image; > > + GetImage(aRowIndex, aColumn, false, imageContext, useImageRegion, getter_AddRefs(image)); > > + PaintImage(image, aColumn, iconRect, aPresContext, aRenderingContext, > > + imageContext, aDirtyRect, remainingWidth, currX, useImageRegion); > > Nit: |imageContext| should be aligned with |image|. > > @@ +3309,5 @@ > > + nsAutoString properties = nsTreeUtils::StringizeProperties(mScratchArray); > > + mCellRenderer->Draw(mView, aRowIndex, aColumn, properties, getter_AddRefs(image)); > > + } > > + if (image) { > > + useImageRegion = false; > > It seems like it'd make more sense to default |useImageRegion| to |false|. > If you get an image from calling |mCellRenderer->Draw|, then it'll already > be set to the correct value, and you won't need this line. If you don't, Agreed, done. > you'll call |GetImage()|, and that will set it to the correct value, because > |GetImage()| always sets the value of |useImageRegion|. > > @@ +3314,5 @@ > > + } else { > > + GetImage(aRowIndex, aColumn, true, rendererContext, useImageRegion, getter_AddRefs(image)); > > + } > > + PaintImage(image, aColumn, iconRect, aPresContext, aRenderingContext, > > + rendererContext, aDirtyRect, remainingWidth, currX, useImageRegion); > > Nit: It looks like |renderContext| needs to be aligned with |image| on this > line. > > @@ +3447,5 @@ > > imageContext->StyleMargin()->GetMargin(imageMargin); > > imageRect.Deflate(imageMargin); > > > > + bool useImageRegion = aUseImageRegion; > > + nsCOMPtr<imgIContainer> image = aImage; > > Don't copy these argument values. Just replace |useImageRegion| by > |aUseImageRegion| and |image| by |aImage| in the rest of the method. Done > > ::: layout/xul/tree/nsTreeBodyFrame.h > @@ +634,5 @@ > > nsTHashtable<nsPtrHashKey<nsTreeImageListener> > mCreatedListeners; > > > > + // This is a external tree cell render, it's will rendering the cell > > + // content, and resulting imgIContainer, the rendering progress will > > + // be canvas or other ways. > > This is another comment that needs rewording. I hesitate to make suggestions > as the XUL folks may know better, but maybe something like: > > "This is an external tree cell renderer. If present, it renders the cell > content into an imgIContainer, which is then painted using PaintImage." done with minor tune.
Attachment #8496483 - Attachment is obsolete: true
Attachment #8496483 - Flags: review?(seth)
Attachment #8497667 - Flags: superreview?(seth)
I need people familiar with XUL tree to review this patch. Sorry for annoy so much people.
Attachment #8496494 - Attachment is obsolete: true
Attachment #8496494 - Flags: superreview?(dbaron)
Attachment #8496494 - Flags: review?(neil)
Attachment #8496494 - Flags: review?(dbaron)
Attachment #8496494 - Flags: review?(ajvincent)
Attachment #8497672 - Flags: superreview?(dbaron)
Attachment #8497672 - Flags: review?(seth)
Attachment #8497672 - Flags: review?(neil)
Attachment #8497672 - Flags: review?(mats)
Attachment #8497672 - Flags: review?(ajvincent)
Comment on attachment 8497672 [details] [diff] [review] 0005-Fixes-for-bug-441414-to-add-rich-tree-support-crasht.patch (I don't think I'm the right reviewer for this patch.)
Attachment #8497672 - Flags: review?(mats)
Attachment #8496484 - Flags: review?(ajvincent)
Attachment #8496486 - Flags: review?(ajvincent)
Comment on attachment 8497672 [details] [diff] [review] 0005-Fixes-for-bug-441414-to-add-rich-tree-support-crasht.patch Review of attachment 8497672 [details] [diff] [review]: ----------------------------------------------------------------- I think I'm going to have to compile this to see everything that's going on... there's a lot. I don't yet feel comfortable granting feedback+, but the sample does help. ::: layout/xul/tree/crashtests/441414-richtree.xul @@ +9,5 @@ > +var Cc = Components.classes; > +var Ci = Components.interfaces; > +var Cu = Components.utils; > + > +var gIdx=[{ Try to be consistent with braces and brackets. Also, what does gIdx represent? (Comments are your friend.) @@ +30,5 @@ > +var treeView = { > + _rowCount: 0, > + updateRowCount: function() { > + this._rowCount = 19173966; //2147483647,19173966, 19173961.133928571428571428571429 > + }, What is the significance of this number? (Put it in a comment.) @@ +38,5 @@ > + if (aIID.equals(Ci.nsITreeView) || > + aIID.equals(Ci.nsISupports)) > + return this; > + throw Components.results.NS_NOINTERFACE; > + }, Import the XPCOMUtils module and use XPCOMUtils.generateQI here please. @@ +52,5 @@ > + if (idx <= 1) { > + node = gIdx[idx]; > + } else { > + node = gIdx[0]; > + } Nit: node = gIdx[idx <= 1 ? idx : 0]; @@ +100,5 @@ > + if (node.isRead) { > + ret += "read "; > + } else { > + ret += "unread "; > + } ret += node.isRead ? "read " : "unread "; @@ +111,5 @@ > +}; > + > +function onStart() { > + treeView.updateRowCount(); > + var treeBoxObject= document.getElementById("threadTree"); Don't call this treeBoxObject. Each tree has a treeBoxObject property, and it means something very different than the tree element. @@ +120,5 @@ > +window.addEventListener("load", onStart, true); > + > +function initThreadTree(threadTreeId, threadTreeBodyId) { > + treeCellCanvas.initialize(document.getElementById(threadTreeId), > + document.getElementById(threadTreeBodyId)); Please put each argument on a different line, or all on the same line. You might just want to say: let tree = document.getElementById(threadTreeId), body = document.getElementById(threadTreeBodyId); treeCellCanvas.initialize(tree, body); @@ +176,5 @@ > +function jsdump(str) { > + Cc['@mozilla.org/consoleservice;1'] > + .getService(Ci.nsIConsoleService) > + .logStringMessage(str); > +} You don't need this function. Instead, Cu.import("resource://gre/modules/Services.jsm"); and then when you need to log, Services.console.logStringMessage(str); @@ +178,5 @@ > + .getService(Ci.nsIConsoleService) > + .logStringMessage(str); > +} > + > +function drawText(ctx, text, font, color, align, x, y, maxWidth, crop) { This function needs some JavaDoc, I think. Maybe it's obvious from reading the argument names, but: Is align is a boolean, a string, etc... Can x and y be decimal numbers, or must they be integers, etc. Is ctx a 2-D canvas or a WebGL canvas? (Note I do *not* recommend JavaDoc on the treeview implementation above, because you've already referred to the nsITreeView interface you're implementing.) @@ +194,5 @@ > + for (var i = 0; i < text.length && i >= finalPos;) { > + var ch = text[i]; > + //replace(/[\.,-\/#!$%\^&\*;:{}=\-_`~()]/g,"") > + //.,-/#!$%^&*;:{}=\-_\'~()]" > + //' \t\n\r\v' I realize it's commented out, but what does it say? @@ +224,5 @@ > + aIID.equals(Ci.nsISupports) || > + aIID.equals(Ci.nsISupportsWeakReference)) > + return this; > + throw Components.results.NS_NOINTERFACE; > + }, Again, XPCOMUtils.generateQI is your friend. @@ +267,5 @@ > + > + this.treeBoxObject.cellRenderer = this; > + this.treeBoxObject.invalidate(); > + }, > + invalidateRow: function(row) { Please put a blank line between functions. @@ +278,5 @@ > + if (this.mousePos == null) { > + this.mousePos = {x:-1, y:-1}; > + } > + switch (e.type) { > + default: This switch statement is meaningless! @@ +289,5 @@ > + > + var isSameColumn = posCol.value != null && posCol.value.id == this.richColumn.id; > + > + let row = posRow.value; > + if (posCol.value == null) { Why not just if (row.value === null)? (Beware 0 == null, but 0 !== null.) @@ +307,5 @@ > + } > + }, > + //nsISupports (ctx) render(in nsITreeView view, in long row, in nsITreeColumn col, in AString props); > + //The underlying code for view is comn\mailnews\base\src\nsMsgDBView.cpp > + draw: function(view, row, col, props) { You've documented the nsITreeCellRenderer interface below. Instead of the nsISupports... line, just say // nsITreeCellRenderer. The reference to underlying code is fine. Throughout this function, you mix commands to the context with gathering data from the view. I recommend gathering all your data up front, and then send commands to the context. Also, this method needs some comments, I think. @@ +316,5 @@ > + ctx.clearRect(0, 0, this.canvas.width, this.canvas.height); > + > + let hasButton = this.relativeRow == row - this.treeBoxObject.getFirstVisibleRow(); > + let hasHover = propertiesHasAtom(props, 'hover'); > + var selected = propertiesHasAtom(props, 'selected'); Consider: let propsArray = props.split(/\s+/); let hasHover = props.indexOf("hover") != -1; let selected = props.indexOf("selected") != -1; @@ +357,5 @@ > + var date = view.getCellText(row, this.dateColumn).split(' ')[0]; > + var title = view.getCellText(row, this.subjectColumn).substring(0, 128); > + > + ctx.beginPath(); > + ctx.arc(500, 50, row % 10 + 10, 0, 2 * 3.14, false); 2 * Math.PI please. @@ +404,5 @@ > + <hbox flex="1"> > + <tree id="threadTree" width="600" hidecolumnpicker="true"> > + <treecols> > + <treecol hideheader="true" id="richCol" flex="1" type="renderer" ignoreincolumnpicker="true" /> > + <treecol hidden="true" hideheader="true" id="senderCol" flex="1" type="rich" ignoreincolumnpicker="true" /> I forget what type="rich" means. Most of these other attributes are new to me... interesting.
Attachment #8497672 - Flags: review?(ajvincent) → feedback-
Comment on attachment 8496484 [details] [diff] [review] 0003-Add-a-cell-column-row-type-TYPE_RENDERER-and-corresp.patch :evilpie, feedback requested because of bug 1081692; I think your patch there might cause a conflict with this bug, which is getting some traction again thanks to Yonggang Luo.
Attachment #8496484 - Flags: feedback?(evilpies)
I think we should stop work on this bug. See https://groups.google.com/forum/#!topic/mozilla.dev.platform/HDJl1iBifB8. At least we should stop reviewing patches here until we have a consensus that adding features to XUL trees is a good thing.
Comment on attachment 8496484 [details] [diff] [review] 0003-Add-a-cell-column-row-type-TYPE_RENDERER-and-corresp.patch I think you just have to add TYPE_RENDERER to the TreeColumn.webidl
Attachment #8496484 - Flags: feedback?(evilpies) → feedback+
Comment on attachment 8497672 [details] [diff] [review] 0005-Fixes-for-bug-441414-to-add-rich-tree-support-crasht.patch Unsetting the review flag since it seems like the consensus right now is not to do this. Feel free to rerequest review if that changes.
Flags: needinfo?(seth)
Attachment #8497672 - Flags: review?(seth)
Attachment #8497667 - Flags: superreview?(seth)
Comment on attachment 8496484 [details] [diff] [review] 0003-Add-a-cell-column-row-type-TYPE_RENDERER-and-corresp.patch As discussed in dev-platform, we don't want to take major feature improvements to XUL.
Attachment #8496484 - Flags: superreview?(dbaron)
Attachment #8496484 - Flags: superreview-
Attachment #8496484 - Flags: review?(neil)
Attachment #8496484 - Flags: review?(dbaron)
Comment on attachment 8496486 [details] [diff] [review] 0004-Add-StringizeProperties-const-AtomArray-aPropertiesA.patch As discussed in dev-platform, we don't want to take major feature improvements to XUL.
Attachment #8496486 - Flags: superreview?(dbaron)
Attachment #8496486 - Flags: superreview-
Attachment #8496486 - Flags: review?(neil)
Attachment #8496486 - Flags: review?(dbaron)
Comment on attachment 8497672 [details] [diff] [review] 0005-Fixes-for-bug-441414-to-add-rich-tree-support-crasht.patch As discussed in dev-platform, we don't want to take major feature improvements to XUL.
Attachment #8497672 - Flags: superreview?(dbaron)
Attachment #8497672 - Flags: superreview-
Attachment #8497672 - Flags: review?(neil)
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #194) > Comment on attachment 8497672 [details] [diff] [review] > 0005-Fixes-for-bug-441414-to-add-rich-tree-support-crasht.patch > > As discussed in dev-platform, we don't want to take major feature > improvements to XUL. I didn't see any HTML5 equivalent for XUL tree element. And this should not be a major feature improvements.
Flags: needinfo?(dbaron)
(In reply to Jakob Kobberholm from comment #59) > Oh and a link to the project of course: > http://www.fossfactory.org/project/p294 Hi Jakob, any chance the bug bounty could be paid to Yonggang Luo even if the patches were never accepted in the end?
Status: NEW → ASSIGNED
Flags: needinfo?(jakob)
Hello Philip. Maybe. A lot of other people have donated money and I don't know if it should be up to me to decide if their money should go to waste. I am really quite disappointed in the nonchalant dismissal of a feature and frankly developer-hostile attitude of the Mozilla people. This is not the image Mozilla usually displays. If for no other reason than to encourage open source contribution (in spite of Mozilla's efforts against it in this case), I am willing to donate my money for the patch. I would be very interested in hearing from other people with money in the pool.
Flags: needinfo?(jakob)
I am already using this feature in productive environment, indeed, I didn't found any equivalent feature like XUL:Tree in HTML5, all those exist HTML simulated grid/tree library doesn't provide as much functional as XUL:tree does, the most appealing feature in XUL:tree is that it rendering all the tree-row dynamically, and I can getting it rendering with CANVAS, that's introduce much possibilities that XUL tree can do.
Have been watching this bug for a few years and must say, it's pretty disappointing that it's not getting anywhere. As far as I see it, then the issue itself still remains - i.e., some of us would like to see a multi-line thread pane. Outlook has done it for years, Apple IOS does it too - and am sure a few other clients have the same functionality. The XUL technology in its current form is limited in a way to where it doesn't allow us to easily add support for a multi-line thread pane, so we need to expand on it. However, Mozilla has decided to abandon all development and features with XUL, which prevents anyone from adding support directly. At this point, the only thing we can do to enable the multi-line thread pane is to re-write whatever it is to use the different technology? Considering it has taken 6 years to decide that the technology is too old, will it take another 6 years to decide that the feature should be implemented like other clients?
+1 on Ryan comment
so perhaps it's time to STOP ANNOYING PEOPLE with "hey this is sooo old" and "+1" notifications! This doesn't really help! Neither to solve the bug, nor to put any attention to it! Even it doesn't help anyone to dig into and to understand the history of this issue! so please: STOP complaining about it! It has been said enough. Thunderbird is not maintained officially, technology is outdated, it's not trivial. Perhaps you should switch so another Mail client if it bothers. There are some out there based on thunderbird that may help you to migrate old accounts easily! http://www.postbox-inc.com/ e.g. is available for Windows and Mac. Perhaps it fit's your needs!
(In reply to Marcel from comment #201) > so perhaps it's time to STOP ANNOYING PEOPLE with "hey this is sooo old" and > "+1" notifications! This doesn't really help! Neither to solve the bug, nor > to put any attention to it! Even it doesn't help anyone to dig into and to > understand the history of this issue! > so please: STOP complaining about it! It has been said enough. Thunderbird > is not maintained officially, technology is outdated, it's not trivial. > Perhaps you should switch so another Mail client if it bothers. There are > some out there based on thunderbird that may help you to migrate old > accounts easily! Complaints are complaints, they happen. Story of life. I get the point. Same as for other clients that can do things as well. Would rather put the attention on the issue itself. As for the underlying issue, let me ask the question differently. What would need to happen in order to get multi-line thread pane in place, since developers have been seemingly forbidden to code with XUL? Is there an official markup that is authorized to develop in, such that if there were other corporations interested in putting money into the pot, they'd at least have an idea as to what type of resources would be necessary for the job?
This bug is not the best place to pursue a discussion of multi-line thread pane in Thunderbird. I've done a couple of posts to tb-planning@mozilla.org in response to comment 202 and comment 201, please discuss there if you want to pursue this from the Thunderbird perspective. (Though as of writing this post those have not appeared yet in tb-planning, so it may take awhile to sort out why).
(In reply to Yonggang Luo from comment #195) > (In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from > comment #194) > > Comment on attachment 8497672 [details] [diff] [review] > > 0005-Fixes-for-bug-441414-to-add-rich-tree-support-crasht.patch > > > > As discussed in dev-platform, we don't want to take major feature > > improvements to XUL. > I didn't see any HTML5 equivalent for XUL tree element. > And this should not be a major feature improvements. This is a major feature; major features add substantial maintenance cost over time, and we're not interested in adding to that cost by adding additional XUL features.
Flags: needinfo?(dbaron)
The maintenance is relative, I can take in charge of this feature.
Blocks: 405913
Is there any way that TB can have some kind of overlay to implement this in Gecko separately from the main tree?
(In reply to Charles from comment #206) > Is there any way that TB can have some kind of overlay to implement this in > Gecko separately from the main tree? In theory, yes it could be added. But I think it is unlikely that we would want to do that. It would be asking us to support XUL feature maintenance that nobody on our team knows much about, when the people that do know something about it are unwilling to take on that burden. We would rather spend our resources trying to develop a non-XUL alternative which has a brighter future.
No assignee, updating the status.
Status: ASSIGNED → NEW
No assignee, updating the status.

Wouldn't it be possible to get an idea from Postbox? Their source code is available and based off of Thunderbird.

I don't think so, the long term plan is to get rid of it. Bug 1446335.

We've dusted off Paul Rouget's work. See bug 213945 comment #214 for details. The solution is geared towards Thunderbird's necessities and it's understandable that such code is not strictly desired in the Mozilla codebase. However, it will serve as a bridge until there is a more powerful tree widget available.

Hi Zoe,
Can you tell approximately how long it will take before I, as average user, can use your temporary solution?
It's great that you took the effort!

Severity: normal → S3
No longer blocks: 213945
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: