This covers the implementation of NS_THEME_TREEVIEW, NS_THEME_TREEVIEW_TREEITEM, NS_THEME_TREEVIEW_TWISTY, NS_THEME_TREEVIEW_LINE, NS_THEME_TREEVIEW_HEADER, NS_THEME_TREEVIEW_HEADER_CELL and NS_THEME_TREEVIEW_HEADER_SORTARROW for XUL tree views on GTK.

bryner has most of this done.

(In reply to comment #1) > bryner has most of this done. Bryner, any news on this part? Chris commented a long time ago that you are trying to finish it. It's still a leak in current trunk builds. Due to it's one of the major widgets which doesn't support native theming we should fix it ASAP. It doesn't look nice to have a non-themable treeview in MailNews / Thunderbird.

This patch does a pretty good job to get native theming for NS_THEME_TREEVIEW and NS_THEME_TREEVIEW_HEADER_CELL, but there are a few problems with it: - Two trees with header cells on the same page causes the headers in the second tree to be very slow rendered. I guess this is because the header cells of both trees share the same gdkwindow created by the treeview, but I am not sure. - With clearlooks theme at least, clicking on native tree headers causes the label to be displaced to the left and to the bottom. With this patch the labels will only be displaced to the left. I don't know how to displace the label to the bottom without affecting the style of the headers too much (padding-bottom is already 0px, that can not be adjusted). - The header cells are missing focus lines, because focusing tree header cells seems not to be implemented. (I don't mind to do this, but I don't know how). Further more, I tested the CSS changes on WinXP Luna theme and Classic theme, and they don't seem to change the layout there.

This is an example of two trees, where the second tree is rendered very slowly with patch 282686 applied.

roc, do you have any idea on why the headers in the second tree of attachment 282687 [details], are rendered very slowly with the patch of attachment 282686 [details] [diff] [review] applied?

Strange enough, this example does not show the slow painting problem. On neither of the trees.

> I guess this is because the header cells of both > trees share the same gdkwindow created by the treeview, but I am not sure. Could be. Or maybe we're invalidating too much for some reason? Check the size of the area that's actually being repainted. Or you may just have to do some profiling.

OK, apparently the trees overlap (good old DOMi). Could this (overlapping) be caused by bug 300030? I see now that it also only happens when you resize the window to a certain height, in which way the trees get a computed style height of XXX.5px In Firefox 2.0 the trees (without this patch BTW) only get a computed style height of XXX.467 each. I assume the underlying problem should still be fixed here, right?

The slow painting problem with overlapping trees is not part of this bug. I tested attachment 282687 [details] in an older build (20070911) without the current patch (no native theming), and the problem was the same there.

Comment on attachment 282798 [details] [diff] [review] NS_THEME_TREEVIEW and NS_THEME_TREEVIEW_HEADER_CELL implementation I have some comments myself regarding this patch: >+gint >+moz_gtk_tree_header_cell_get_focus(gboolean* interior_focus, >+ gint* focus_width, gint* focus_pad) >+{ >+ ensure_tree_header_cell_widget(); >+ >+ gtk_widget_style_get (gTreeHeaderCellWidget, >+ "interior-focus", interior_focus, >+ "focus-line-width", focus_width, >+ "focus-padding", focus_pad, >+ NULL); >+ >+ return MOZ_GTK_SUCCESS; >+} >+ Is there are reason why these foo_get_focus functions aren't static? >+static gint >+moz_gtk_treeview_paint(GdkDrawable* drawable, GdkRectangle* rect, >+ GdkRectangle* cliprect, GtkWidgetState* state) >+{ >+ GtkStyle *style; >+ GtkStateType state_type; >+ >+ ensure_tree_view_widget(); >+ >+ /* only handle disabled and normal states, otherwise the whole background >+ * area will be painted differently with other states */ >+ if (state->disabled) >+ state_type = GTK_STATE_INSENSITIVE; >+ else >+ state_type = GTK_STATE_NORMAL; >+ >+ /* In GTK the treeview sets the background of the window >+ * which contains the cells to the treeview base color. >+ * If we don't set it here the background color will not be correct. >+ * */ >+ gtk_widget_modify_bg(gTreeViewWidget, state_type, >+ &gTreeViewWidget->style->base[state_type]); ^ That is just setting styles, no? You could change this back to two lines for each of NORMAL and INSENSITIVE and move it to ensure_tree_view_widget within the condition clause, so this is only done once and not every time a treeview is drawn. >+ >+ style = gTreeViewWidget->style; >+ >+ TSOffsetStyleGCs(style, rect->x, rect->y); >+ >+ gtk_paint_box(style, drawable, state_type, GTK_SHADOW_IN, >+ cliprect, gTreeViewWidget, "tree_view", rect->x, >+ rect->y, rect->width, rect->height); >+ I thought "treeview" was the more standard detail string. Then again there is next to no documentation about different detail strings and I had to write my own list from searching for GTK testcases. >+ return MOZ_GTK_SUCCESS; >+}

This patch addresses the issues from comment #10 (In reply to comment #10) > Is there are reason why these foo_get_focus functions aren't static? Yes, when these functions are static you apparently can't access them from other static functions like moz_gtk_button_paint (I get compile errors when I do that). Any way I merged those functions into one function now, because as things were in the previous patch, we would create an extra GtkButton for a tree header cell (because of moz_gtk_button_get_focus in moz_gtk_button_paint, and that would also be true for the arrow widget). > I thought "treeview" was the more standard detail string. Then again there is > next to no documentation about different detail strings and I had to write my > own list from searching for GTK testcases. You're right, everywhere in gtktreeview.c tree_view is used except for the detail string.

roc, I have a patch to implement twisties, but I want this patch in first, and I also need to talk to you about something in layout/ that is causing problems.

- + You're adding trailing whitespace here. Don't. Doesn't this affect Windows, via your changes to winstripe? Otherwise this looks good, although I'm not sure it will meet 1.9 approval criteria so it might have to wait for landing.

same patch as v2, but doesn't add the extra trailing whitespace. > Doesn't this affect Windows, via your changes to winstripe? Like I said, I tested the CSS changes on WinXP Luna theme and Classic theme, and they don't seem to change the layout there. Should I ask a toolkit peer to review those changes?

Yeah. I don't understand how those changes can not affect Windows.

Comment on attachment 283203 [details] [diff] [review] NS_THEME_TREEVIEW and NS_THEME_TREEVIEW_HEADER_CELL implementation (v3) asking review for style changes in tree.css and listbox.css

Comment on attachment 283203 [details] [diff] [review] NS_THEME_TREEVIEW and NS_THEME_TREEVIEW_HEADER_CELL implementation (v3) mpa=mano given roc's review, I don't have a linux box to test this on myself.

I want someone to explain why this doesn't affect Windows.

Comment on attachment 283203 [details] [diff] [review] NS_THEME_TREEVIEW and NS_THEME_TREEVIEW_HEADER_CELL implementation (v3) (In reply to comment #18) > I want someone to explain why this doesn't affect Windows. > Actually it does affect the Classic theme, but only when there is just one column. (the height of the activated header remains the same because of the other headers)

same patch as v3, but adds tree.css and listbox.css to gnomestripe. Changes in tree/listbox.css with regards to winstripe: treecol, treecolpicker listheader - padding right/left treecol:hover:active, treecolpicker:hover:active listheader[sortable="true"]:hover:active - border-width - border-colors - padding .treecol-image:hover:active - padding treecol:hover:active .treecol-text listheader[sortable="true"]:hover:active .listheader-label - margin (new) tested with and without native theming, with good results. In native GTK apps Clearlooks does move the text when activating the headers (creating the illusion of depressed state), but most other themes don't (from what I tested). However, it does not look wrong when doing that.

I want to add, that the slow painting problems will disappear when you don't calculate a border for the TreeView widget. I even think that for native TreeViews there is no border calculated either (look at Nautilus with "view as list"). But if there is not a border drawn above the TreeView (in Nautilus: the border of the toolbar), it will look weird (screenshot).

(In reply to comment #13) > Otherwise this looks good, although I'm not sure it will meet 1.9 approval > criteria so it might have to wait for landing. > With all the discussions about native theming on Linux recently (see blog of alex faaborg), this may get approval for 1.9? ;)

Comment on attachment 283500 [details] [diff] [review] NS_THEME_TREEVIEW and NS_THEME_TREEVIEW_HEADER_CELL implementation (checked in) I think this is good. However, I'm not sure about 1.9 approval, I'll punt that to others...

Reed points out that gnomestripe isn't actually used anymore. So why does this patch work?

Reed points out that he is wrong. So never mind :-)

1.9 approval should happen, because this basically takes the same route as every other widget, which has proven to be a stable way of drawing widgets.

I should also say that out of every widget we don't draw natively, the treeview header seems to stick out the most because it is a totally incorrect size and colour.

Comment on attachment 283500 [details] [diff] [review] NS_THEME_TREEVIEW and NS_THEME_TREEVIEW_HEADER_CELL implementation (checked in) I'd like to get this in for the beta, as it helps make Firefox on Linux look much better with regards to trees. I've been playing with a build with this patch for a little while, and the trees look so much more integrated with the OS.

Too big for M9, but good for M10.

Checking in widget/src/gtk2/gtk2drawing.c; /cvsroot/mozilla/widget/src/gtk2/gtk2drawing.c,v <-- gtk2drawing.c new revision: 1.33; previous revision: 1.32 done Checking in widget/src/gtk2/gtkdrawing.h; /cvsroot/mozilla/widget/src/gtk2/gtkdrawing.h,v <-- gtkdrawing.h new revision: 1.31; previous revision: 1.30 done Checking in widget/src/gtk2/nsNativeThemeGTK.cpp; /cvsroot/mozilla/widget/src/gtk2/nsNativeThemeGTK.cpp,v <-- nsNativeThemeGTK.cpp new revision: 1.107; previous revision: 1.106 done Checking in toolkit/themes/gnomestripe/global/jar.mn; /cvsroot/mozilla/toolkit/themes/gnomestripe/global/jar.mn,v <-- jar.mn new revision: 1.15; previous revision: 1.14 done Checking in toolkit/themes/gnomestripe/global/listbox.css; /cvsroot/mozilla/toolkit/themes/gnomestripe/global/listbox.css,v <-- listbox.css new revision: 1.3; previous revision: 1.2 done Checking in toolkit/themes/gnomestripe/global/tree.css; /cvsroot/mozilla/toolkit/themes/gnomestripe/global/tree.css,v <-- tree.css new revision: 1.3; previous revision: 1.2 done

Bug is not fixed yet, only a few widgets have been handled so far: (comment #12) > roc, I have a patch to implement twisties, but I want this patch in first and I'm currently working on native sort arrows.

Restore bug status as per comment 31. Teune, then we should mark your latest patch as checked-in.

Can you make a complete list of what else you want to do, and indicate where good stopping-points are? We can't keep landing new code in 1.9 indefinitely.

Amazing work, this stuff looks great. One thing, the list header in the "Applications" tab in preferences now looks a bit funny, is this a regression or cause by the way how the widget is (incorrectly?) packed?

(In reply to comment #33) > Can you make a complete list of what else you want to do, and indicate where > good stopping-points are? We can't keep landing new code in 1.9 indefinitely. > I only want to implement the sort arrows, I almost have a patch ready. I don't know what Michael (Ventnor) is up to, however. (as you can see with the screenshot from attachment 287764 [details], sort arrows definitely should be improved ;)).

The twisties are not as important to me now due to a fundamental roadblock that may not be trivial to get around in 1.9. I'm working on other Linuxey things, though.

(In reply to comment #34) > Created an attachment (id=287764) [details] > Regression? > > Amazing work, this stuff looks great. One thing, the list header in the > "Applications" tab in preferences now looks a bit funny, is this a regression > or cause by the way how the widget is (incorrectly?) packed? > The latter, because that element (richlistbox) only allows one list header, as a workaround a single header is used to pack those 2 headers. And maybe the former too: we (or GTK) seem to ignore style="border: 0; padding: 0;". At this moment, I don't know how to fix this without breaking the layout of normal headers.

For(In reply to comment #37) > I don't know how to fix this without breaking the layout of > normal headers. Forgive my bugspam: The style of the containing header should just be: -moz-appearance: none;

(In reply to comment #38) > Forgive my bugspam: The style of the containing header should just be: > -moz-appearance: none; Can you roll a patch for that?

The corners of listviews are rounded, they are not in normal GTK apps. While this looks kind of nice at the bottom of the list, it seems to break at the top, with the line getting broken open. Can this be fixed (disabling border-radius or whatever does this?)

This fixes the issue described in comment 34, the -moz-appearance: none; addition should be OK for other platforms (tested on WinXP). But unfortunately this introduces another issue: the right edge of the last header overlaps the right border of the listbox. This can be fixed with extra padding on the containing header, but I expect that to leave a small gap to the right for other platforms. Note that this overlap doesn't occur for the containing header, and not for non native themed headers, so it is probably caused by a bug in native theming: I will look into that.

I meant to attach this patch

(In reply to comment #40) > Created an attachment (id=287821) [details] > Another small bug > > The corners of listviews are rounded, they are not in normal GTK apps. While > this looks kind of nice at the bottom of the list, it seems to break at the > top, with the line getting broken open. Can this be fixed (disabling > border-radius or whatever does this?) > Are you sure the rounded corners are not drawn by your GTK theme? Clearlooks the theme I use, just paints square, or not noticeable rounded corners. Note that some (most?) GTK apps don't use borders around lists, like list view in nautilus or archive manager for example. An app I am aware of using borders is Rhythmbox. Can you check if Rhythmbox uses rounded corners? And where can I find the theme you use? Maybe I can test this then.

(In reply to comment #41) > But unfortunately this introduces another issue: the right edge of the last > header overlaps the right border of the listbox. This can be fixed with extra > padding on the containing header, but I expect that to leave a small gap to the > right for other platforms. > Note that this overlap doesn't occur for the containing header, and not for non > native themed headers, so it is probably caused by a bug in native theming: I > will look into that. > OK, I think I've found the problem here: when the total width of the two headers is an odd number of pixels, both headers have a computed style width of exactly half of the total number of pixels (In my case 226.5px). But when painting the headers natively the painted size is rounded up to 227px, that's why the headers overlap the border exactly 1px to the right (2x227 - 2x226.5) when the total width is an odd number. Resizing the window, so the total width of the headers is an even number, fixes the overlap issue.

Comment on attachment 287991 [details] [diff] [review] implement NS_THEME_TREEVIEW_HEADER_SORTARROW (checked in) Some clarification for this patch: > state_type = state->disabled ? GTK_STATE_INSENSITIVE : GTK_STATE_NORMAL; >- >+ > style = gTreeViewWidget->style; This remove some abundant whitespace, included in the previous patch >+ /* hard code these values */ >+ arrow_rect.width = 11; >+ arrow_rect.height = 11; on GTK the arrow size does not change with font size changes, for what I have tested. >@@ -1144,17 +1177,17 @@ moz_gtk_dropdown_arrow_paint(GdkDrawable > (...) > gtk_paint_arrow(style, drawable, state_type, shadow_type, cliprect, >- gHorizScrollbarWidget, "arrow", GTK_ARROW_DOWN, TRUE, >+ gDropdownButtonWidget, "arrow", GTK_ARROW_DOWN, TRUE, > real_arrow_rect.x, real_arrow_rect.y, > real_arrow_rect.width, real_arrow_rect.height); > This code paints the DropdownButton Arrow, but references the gHorizScrollbarWidget, which can be NULL. I hope it is alright to fix this here. >+ case NS_THEME_TREEVIEW_HEADER_SORTARROW: > case NS_THEME_SPINNER_UP_BUTTON: > case NS_THEME_SPINNER_DOWN_BUTTON: > // hard code these sizes > aResult->width = 14; > aResult->height = 13; > break; > } >- I just use these values which are already there, because I'm not sure which are the optimal values for the sort arrow. And I just now notice the '-' here, I hope you don't mind that I'd rather wait for a review, before I update this patch.

@Teune van Steeg, re #43: yes I am sure this is not because of my GTK theme. I'm using Clearlooks too (GNOME 2.20 version). I see the listview is square in the GtkFileChooser dialog for example.

Comment on attachment 287851 [details] [diff] [review] patch for the listheader in the applications preferences pane (checked in). Asking review on this patch. The issue of the right header overlapping the right border is not caused by setting -moz-appearance: none; but it is a bug in the drawing code: floating-point pixels are just rounded. I will file a bug on that, if one doesn't exist yet.

Comment on attachment 287851 [details] [diff] [review] patch for the listheader in the applications preferences pane (checked in). (In reply to comment #44) > Resizing the window, so the total width of the headers is an even number, fixes > the overlap issue. It does, but it still leaves what appears to be a second right-hand border on the right side of the header, which looks off. Overall this looks much better, though. r=myk

Checking in browser/components/preferences/applications.xul; /cvsroot/mozilla/browser/components/preferences/applications.xul,v <-- applications.xul new revision: 1.6; previous revision: 1.5 done

Checking in widget/src/gtk2/gtk2drawing.c; /cvsroot/mozilla/widget/src/gtk2/gtk2drawing.c,v <-- gtk2drawing.c new revision: 1.35; previous revision: 1.34 done Checking in widget/src/gtk2/gtkdrawing.h; /cvsroot/mozilla/widget/src/gtk2/gtkdrawing.h,v <-- gtkdrawing.h new revision: 1.33; previous revision: 1.32 done Checking in widget/src/gtk2/nsNativeThemeGTK.cpp; /cvsroot/mozilla/widget/src/gtk2/nsNativeThemeGTK.cpp,v <-- nsNativeThemeGTK.cpp new revision: 1.109; previous revision: 1.108 done Checking in toolkit/themes/gnomestripe/global/listbox.css; /cvsroot/mozilla/toolkit/themes/gnomestripe/global/listbox.css,v <-- listbox.css new revision: 1.4; previous revision: 1.3 done Checking in toolkit/themes/gnomestripe/global/tree.css; /cvsroot/mozilla/toolkit/themes/gnomestripe/global/tree.css,v <-- tree.css new revision: 1.4; previous revision: 1.3 done

As per comment #35, I think this bug is complete? Can it be resolved as fixed now?

Actually no, I've figured out a way to implement native twisties/expanders/whatever-they're-called. Patch soon.

Comment on attachment 288462 [details] [diff] [review] NS_THEME_TREEVIEW_TWISTY any new patches should go in new bugs, OK?

(In reply to comment #55) > (From update of attachment 288462 [details] [diff] [review]) > any new patches should go in new bugs, OK? The twisty is still part of the treeview family though...

This seems to have broken location bar on xfce/linux. See the gray are on top of the popup.

Checking in toolkit/themes/gnomestripe/global/tree.css; /cvsroot/mozilla/toolkit/themes/gnomestripe/global/tree.css,v <-- tree.css new revision: 1.5; previous revision: 1.4 done Checking in widget/src/gtk2/gtk2drawing.c; /cvsroot/mozilla/widget/src/gtk2/gtk2drawing.c,v <-- gtk2drawing.c new revision: 1.37; previous revision: 1.36 done Checking in widget/src/gtk2/gtkdrawing.h; /cvsroot/mozilla/widget/src/gtk2/gtkdrawing.h,v <-- gtkdrawing.h new revision: 1.34; previous revision: 1.33 done Checking in widget/src/gtk2/nsNativeThemeGTK.cpp; /cvsroot/mozilla/widget/src/gtk2/nsNativeThemeGTK.cpp,v <-- nsNativeThemeGTK.cpp new revision: 1.110; previous revision: 1.109 done

I can confirm the breakage in comment #57, with gnome and linux.

(In reply to comment #57) > This seems to have broken location bar on xfce/linux. See the gray are on top > of the popup. That's filed as bug 403659.

Could this have caused bug 403724? The tree is currently closed so it would be great if someone could look into it asap.

Backed this out to attempt to fix bug 403724

(In reply to comment #62) > Backed this out to attempt to fix bug 403724 Relanded at 2007-11-14 19:47.

the tree lines turned neon green a few times in my bookmark sidebar. just quirk or has anyone else experience that? kubuntu hardy i386 Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007111504 Minefield/3.0b2pre - Build ID: 2007111504

make that different color. they turned blue too. now they're now showing up at all. (the tree lines that is).

J: I'm seeing this, too. Only seems to affect the lines in the Places window, the Sidebar is fine. The color seems to be random ;) Just hide these lines, GTK does not use them at all.

after some viewing/testing, i see that there are more than a few colors it changes. they disappeared once or twice too. but this is all in the bookmarks sidebar, i rarely use the places organizer.

Sounds like an uninitialized variable somewhere.

I filed bug 434080 on the random treeline colour bug.

(In reply to comment #37) > The latter, because that element (richlistbox) only allows one list header, > as a workaround a single header is used to pack those 2 headers. We've run into this, too. Filed bug 676168. (Next time, when something in XUL doesn't work as it should and you're forced to a workaround, can you please file a bug and see that it gets fixed?)