Open
Bug 441414
Opened 16 years ago
Updated 2 years ago
Treerows need a way to hold richer content
Categories
(Core :: XUL, enhancement)
Core
XUL
Tracking
()
NEW
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 | |
updated patch: description box supports "display: none". Should work on Windows and Mac (not tested)
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 :)
Reporter | ||
Comment 1•16 years ago
|
||
Reporter | ||
Comment 2•16 years ago
|
||
Reporter | ||
Comment 3•16 years ago
|
||
More screenshots here: http://www.mozbox.org/pub/tb/multilineView/
Mainly TB related.
Comment 4•16 years ago
|
||
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?
Comment 5•16 years ago
|
||
It'd make sense to have someone sorta familiar with trees look at this instead of me...
Comment 6•16 years ago
|
||
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)?
Reporter | ||
Comment 7•16 years ago
|
||
(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);
}
Comment 8•16 years ago
|
||
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.
Reporter | ||
Comment 9•16 years ago
|
||
(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.
Comment 10•16 years ago
|
||
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.
Comment 11•16 years ago
|
||
(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:'
Reporter | ||
Comment 12•16 years ago
|
||
Reporter | ||
Comment 13•16 years ago
|
||
Blue boxes are description boxes. Red boxes are cells.
Comment 14•16 years ago
|
||
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?
Reporter | ||
Comment 15•16 years ago
|
||
(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...|
|__________|
Comment 16•16 years ago
|
||
(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.
Reporter | ||
Comment 17•16 years ago
|
||
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
Reporter | ||
Comment 18•16 years ago
|
||
Attachment #326397 -
Attachment is obsolete: true
Reporter | ||
Comment 19•16 years ago
|
||
Attachment #326398 -
Attachment is obsolete: true
Reporter | ||
Comment 20•16 years ago
|
||
Reporter | ||
Comment 21•16 years ago
|
||
Comment 22•16 years ago
|
||
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?
Reporter | ||
Comment 23•16 years ago
|
||
The keyboard navigation model is not affected.
Reporter | ||
Comment 24•16 years ago
|
||
Attachment #328912 -
Attachment is obsolete: true
Reporter | ||
Comment 25•16 years ago
|
||
Attachment #328998 -
Attachment is obsolete: true
Reporter | ||
Comment 26•16 years ago
|
||
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.
Reporter | ||
Comment 27•16 years ago
|
||
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.
Comment 28•16 years ago
|
||
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
Comment 29•16 years ago
|
||
Paul, did you want someone to review this?
Reporter | ||
Comment 30•16 years ago
|
||
(In reply to comment #29)
> Paul, did you want someone to review this?
Yes, for a feedback. Who should I ask to ?
Comment 31•16 years ago
|
||
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 32•16 years ago
|
||
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.
Comment 34•16 years ago
|
||
(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+
Comment 35•16 years ago
|
||
This patch seems to have stalled a bit - we would love to be able to use this in TB 3.0...
Reporter | ||
Comment 36•16 years ago
|
||
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?
Reporter | ||
Comment 38•16 years ago
|
||
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)?
Comment 40•16 years ago
|
||
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.
Comment 42•16 years ago
|
||
(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.
Reporter | ||
Comment 45•16 years ago
|
||
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.
Comment 47•16 years ago
|
||
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 | |
+------------------------------------------------------------------------------+
Comment 48•16 years ago
|
||
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| |
+----------------------------------------------------------------------------+
Comment 50•15 years ago
|
||
(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
Comment 51•15 years ago
|
||
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
Comment 52•15 years ago
|
||
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, ¶ms, 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 ?
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...
Comment 54•14 years ago
|
||
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".
Comment 55•14 years ago
|
||
The main issue is supporting wrapped text. This work has been described above as "hugely painful" and "really complicated".
Comment 56•14 years ago
|
||
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.
Comment 57•13 years ago
|
||
(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?
Reporter | ||
Updated•13 years ago
|
Assignee: paul → nobody
Comment 58•13 years ago
|
||
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.
Comment 59•13 years ago
|
||
Oh and a link to the project of course:
http://www.fossfactory.org/project/p294
Comment 60•13 years ago
|
||
Good idea, Jakob. I added $50 (actually $48.25 after deducing PayPal theft) to the sponsorship.
Comment 61•13 years ago
|
||
I put another $5, making this bug worth $165.
Comment 62•13 years ago
|
||
i put some 9,62€ in, perhaps we could get this long awaited message view in Thunderbird with the help of some money ;-)
Comment 63•13 years ago
|
||
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?
Comment 64•13 years ago
|
||
Added $50 to the Fossfactory fund.
Come on you guys, join in if you haven't already. Even $5 or $10 will help.
Comment 65•13 years ago
|
||
Looks like the reward is already at US$372.
But looks like a hard problem.
Status: ASSIGNED → NEW
Whiteboard: [bounty]
Version: unspecified → Trunk
Comment 66•13 years ago
|
||
Will a richlist do the trick, instead of coding a new structure?
Comment 67•13 years ago
|
||
(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.
Comment 68•13 years ago
|
||
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).
Comment 69•13 years ago
|
||
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.
Comment hidden (off-topic) |
Comment hidden (off-topic) |
Comment 72•11 years ago
|
||
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.
Comment 73•10 years ago
|
||
How did PostBox enhancements to TreeView?
Reporter | ||
Comment 74•10 years ago
|
||
(In reply to Yonggang Luo from comment #73)
> How did PostBox enhancements to TreeView?
Apparently, they used the patch attached to this bug.
Comment 75•10 years ago
|
||
(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.
Comment 76•10 years ago
|
||
I am confused, if I use the patch, did this support for vbox/hbox in treecell?
Comment 77•10 years ago
|
||
Does anybody know which version of Mozilla that PostBox are based on?
So I can do easier merge.
Comment 78•10 years ago
|
||
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.
Comment 79•10 years ago
|
||
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.
Comment 80•10 years ago
|
||
(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)
Reporter | ||
Comment 81•10 years ago
|
||
(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).
Comment 82•10 years ago
|
||
(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.
Comment 83•10 years ago
|
||
(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)
Comment 84•10 years ago
|
||
(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
Comment 85•10 years ago
|
||
(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.
Comment 86•10 years ago
|
||
It's seems the multiline already been committed in nsGkAtomList.h
How to deal with this issue?
Comment 87•10 years ago
|
||
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")
Comment 88•10 years ago
|
||
(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
Comment 89•10 years ago
|
||
(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
Comment 90•10 years ago
|
||
What's the replacement in mozilla 31 for nsISupportsArray
It's already removed in xul/tree, now it's use AtomArray instead of nsISupportsArray*
Comment 91•10 years ago
|
||
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?
Comment 92•10 years ago
|
||
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?
Comment 93•10 years ago
|
||
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.
Comment 94•10 years ago
|
||
(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 95•10 years ago
|
||
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?
Comment 96•10 years ago
|
||
(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.
Comment 97•10 years ago
|
||
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
Reporter | ||
Comment 98•10 years ago
|
||
(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).
Comment 99•10 years ago
|
||
(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,
Reporter | ||
Comment 100•10 years ago
|
||
See comment 45 and comment 46.
Comment 101•10 years ago
|
||
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.
Comment 102•10 years ago
|
||
(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.
Reporter | ||
Comment 103•10 years ago
|
||
(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.
Comment 104•10 years ago
|
||
(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.
Comment 105•10 years ago
|
||
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.
```
Comment 106•10 years ago
|
||
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)
Comment 107•10 years ago
|
||
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)
Comment 108•10 years ago
|
||
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)
Reporter | ||
Comment 109•10 years ago
|
||
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)
Reporter | ||
Updated•10 years ago
|
Attachment #8474218 -
Flags: superreview?(paul)
Attachment #8474218 -
Flags: review?(paul)
Reporter | ||
Updated•10 years ago
|
Attachment #8474221 -
Flags: superreview?(paul)
Attachment #8474221 -
Flags: review?(paul)
Comment 110•10 years ago
|
||
(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.
Comment 111•10 years ago
|
||
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.
Comment 112•10 years ago
|
||
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)
Comment 113•10 years ago
|
||
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.
Comment 114•10 years ago
|
||
Minor changes.
Attachment #8477258 -
Attachment is obsolete: true
Attachment #8477440 -
Flags: review?(seth)
Comment 115•10 years ago
|
||
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 116•10 years ago
|
||
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-
Comment 117•10 years ago
|
||
(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.
Comment 118•10 years ago
|
||
(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.
Comment 119•10 years ago
|
||
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 120•10 years ago
|
||
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 121•10 years ago
|
||
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-
Comment 122•10 years ago
|
||
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.
Comment 123•10 years ago
|
||
(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?
Comment 124•10 years ago
|
||
(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.
Comment 125•10 years ago
|
||
(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.
Comment 126•10 years ago
|
||
(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.
Comment 127•10 years ago
|
||
(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.
Comment 128•10 years ago
|
||
(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
Comment 129•10 years ago
|
||
>
> ::: 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?
Comment 130•10 years ago
|
||
(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.
Comment 131•10 years ago
|
||
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)
Comment 132•10 years ago
|
||
(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 133•10 years ago
|
||
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)
Comment 134•10 years ago
|
||
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.
Comment 135•10 years ago
|
||
(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.
Comment 136•10 years ago
|
||
: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;
}
Comment 137•10 years ago
|
||
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.
Comment 138•10 years ago
|
||
(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.
Comment 139•10 years ago
|
||
(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.
Comment 140•10 years ago
|
||
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."
Comment 141•10 years ago
|
||
(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;
}
Comment 142•10 years ago
|
||
(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.
Comment 143•10 years ago
|
||
I fount in bugzilla, it's very hard to submit patch series.
Comment 144•10 years ago
|
||
I'll respond with my feedback comments this weekend. I've been very busy lately, and for that I apologize.
Comment 145•10 years ago
|
||
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 146•10 years ago
|
||
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-
Updated•10 years ago
|
Attachment #8482308 -
Flags: feedback?(ajvincent)
Comment 147•10 years ago
|
||
(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.
Comment 148•10 years ago
|
||
>
> 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.
Comment 149•10 years ago
|
||
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 150•10 years ago
|
||
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)
Comment 152•10 years ago
|
||
(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 153•10 years ago
|
||
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-
Comment 154•10 years ago
|
||
(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.
Comment 155•10 years ago
|
||
Is there any easy way to submit a series of patches?
Bugzilla limit only to submit a patch.
Flags: needinfo?(luoyonggang)
Comment 156•10 years ago
|
||
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)
Comment 158•10 years ago
|
||
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)
Comment 159•10 years ago
|
||
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)
Comment 160•10 years ago
|
||
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)
Comment 161•10 years ago
|
||
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)
Comment 162•10 years ago
|
||
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)
Comment 163•10 years ago
|
||
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)
Comment 164•10 years ago
|
||
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)
Comment 165•10 years ago
|
||
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.
Comment 168•10 years ago
|
||
(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)
Comment 169•10 years ago
|
||
(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)
Comment 171•10 years ago
|
||
(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)
Comment 172•10 years ago
|
||
(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.
Comment 173•10 years ago
|
||
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 174•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8496483 -
Flags: review?(neil)
Comment 175•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8496483 -
Flags: review?(Sevenspade)
Updated•10 years ago
|
Attachment #8496484 -
Flags: review?(Sevenspade)
Updated•10 years ago
|
Attachment #8496486 -
Flags: review?(Sevenspade)
Comment 176•10 years ago
|
||
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+
Comment 177•10 years ago
|
||
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 178•10 years ago
|
||
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 179•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8496486 -
Flags: review?(seth) → review+
Comment 180•10 years ago
|
||
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+
Comment 181•10 years ago
|
||
(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.
Comment 182•10 years ago
|
||
(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)
Comment 183•10 years ago
|
||
(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.
Comment 184•10 years ago
|
||
Attachment #8496483 -
Attachment is obsolete: true
Attachment #8496483 -
Flags: review?(seth)
Attachment #8497667 -
Flags: superreview?(seth)
Comment 185•10 years ago
|
||
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 186•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8496484 -
Flags: review?(ajvincent)
Updated•10 years ago
|
Attachment #8496486 -
Flags: review?(ajvincent)
Comment 187•10 years ago
|
||
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 188•10 years ago
|
||
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 190•10 years ago
|
||
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 191•10 years ago
|
||
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)
Updated•10 years ago
|
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)
Comment 195•10 years ago
|
||
(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)
Comment 196•10 years ago
|
||
(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)
Comment 197•10 years ago
|
||
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.
Updated•10 years ago
|
Flags: needinfo?(jakob)
Comment 198•10 years ago
|
||
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.
Comment 199•10 years ago
|
||
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?
Comment 200•10 years ago
|
||
+1 on Ryan comment
Comment 201•10 years ago
|
||
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!
Comment 202•10 years ago
|
||
(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?
Comment 203•10 years ago
|
||
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)
Comment 205•10 years ago
|
||
The maintenance is relative, I can take in charge of this feature.
Comment 206•8 years ago
|
||
Is there any way that TB can have some kind of overlay to implement this in Gecko separately from the main tree?
Comment 207•8 years ago
|
||
(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.
Comment 209•6 years ago
|
||
No assignee, updating the status.
Comment 210•6 years ago
|
||
Wouldn't it be possible to get an idea from Postbox? Their source code is available and based off of Thunderbird.
Comment 211•6 years ago
|
||
I don't think so, the long term plan is to get rid of it. Bug 1446335.
Comment 212•3 years ago
|
||
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.
Comment 213•3 years ago
|
||
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!
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•