GNOME Bugzilla – Bug 650424
treeview: expander is not easily themable
Last modified: 2013-03-01 19:09:14 UTC
This is a small patchset that fixes some theming glitches with GtkTreeView. - first patch renders a frame around the rows, in addition to the background, which is already rendered. Theming engines (Adwaita) currently have to workaround this bug by setting a gradient with fake tiny color-stops as background-image. - second patch removes an arbitrary +2 in the rendered expander size; this ensures the size specified by the "expander-size" style property is the one actually reaching the theming engine for rendering. - third patch centers the expander in the spacing provided by the "horizontal-separator" style property. Expanders in list views currently look bad (e.g. Nautilus) because they have no spacing from the left treeview side. Padding in the tree view is only specified by the "horizontal-separator" and "vertical-separator" style properties, so it makes sense to use the horizontal padding to center the expander in it.
Created attachment 187982 [details] [review] treeview: render a frame around the rows, after drawing their background
Created attachment 187983 [details] [review] treeview: don't arbitrairly add 2 to the expander size
Created attachment 187984 [details] [review] treeview: center the expander in the horizontal separator allocation This fixes the expander looking bad in list views.
Review of attachment 187982 [details] [review]: It feel like we should really have a better defined set of 'things the theme will draw' like the css box drawing model, but until then, this is probably ok...
Review of attachment 187983 [details] [review]: Sure, if it works. Do we need to bump the default expander size to compensate ?
Review of attachment 187984 [details] [review]: Is the default value for horizontal-separator nonzero ?
Review of attachment 187984 [details] [review]: Is the default value for horizontal-separator nonzero ? Where else is the horizontal-separator used, and might this has unintended side-effects ?
(In reply to comment #7) Thanks for the reviews; I pushed the first two patches to master, along with a 2px bump of the default expander size. > Is the default value for horizontal-separator nonzero ? Where else is the > horizontal-separator used, and might this has unintended side-effects ? Yes, the default value is 2px. The horizontal separator is used between every pair of columns - it's basically a padding between the columns; each column takes up half of the separator space, which is centered on the separator line in the header. I don't think the patch has any side effects, as GtkTreeView seems to consistently use gtk_tree_view_get_arrow_xrange() to calculate the expander position; testing doesn't reveal anything strange going on, but you know you can never be 100% sure with the tree view :) Long-term, I think we should really deprecate all these pseudo-padding style properties (other widgets have a few more) and use 'padding' (and 'margin') consistently, but this is non trivial with the treeview, (and the cell renderers are not widgets, so they don't follow the usual approach).
Could tree view bugs *please* be filed in the GtkTreeView component next time? Otherwise I won't see them. I will review these patches later on, there's a couple of things I am not sure about.
Moving to correct component.
(In reply to comment #8) > (In reply to comment #7) > > Thanks for the reviews; I pushed the first two patches to master, along with a > 2px bump of the default expander size. I can see why you would want to remove the arbitrary + 2 (it has been there since 2001 and I assume nobody knows why). I am a bit divided on raising the default expander size with 2px, because this has raised the default minimum row height with 2px as well. > > Is the default value for horizontal-separator nonzero ? Where else is the > > horizontal-separator used, and might this has unintended side-effects ? > > Yes, the default value is 2px. The horizontal separator is used between every > pair of columns - it's basically a padding between the columns; each column > takes up half of the separator space, which is centered on the separator line > in the header. It's not padding between columns, it is padding around the cells in a column. For the horizontal separator, there's 1/2 before the cells in the column and 1/2 after. See docs/tree-column-sizing.png. From this same image, we can actually see that the expander is meant to be within the padding, which means that indeed 1/2 horizontal-separator would have to be added, but focus-line-width as well.
(In reply to comment #11) > I can see why you would want to remove the arbitrary + 2 (it has been there > since 2001 and I assume nobody knows why). I am a bit divided on raising the > default expander size with 2px, because this has raised the default minimum row > height with 2px as well. Perhaps we can actually solve the expander size problems for real by looking at the same image. Currently, in the style updated handler, EXPANDER_EXTRA_PADDING is added to expander size. Subsequently, expander_size + EXPANDER_EXTRA_PADDING is used in various calculations, including when checking whether a row size is expander_size + EXPANDER_EXTRA_PADDING at minimum. This EXPANDER_EXTRA_PADDING can also be considered arbitrary. Also observe how EXPANDER_EXTRA_PADDING is subtracted from expander_size in gtk_tree_view_draw_arrow(). So indeed, when drawing the arrow we need to use expander_size as size. However, I think that EXPANDER_EXTRA_PADDING should simply be replaced with a proper calculation: add vertical_separator + 2 * focus-line-width to the expander_size instead. And perhaps also rename the variable to expander_size_with_padding? That way all arbitrary values in expander size calculation should be gone :)
*** Bug 653027 has been marked as a duplicate of this bug. ***
Renaming this bug; the only part that still applies is that the expander padding and positioning are not easily themable. We currently have a hack in place in Adwaita to workaround this, but I would like to get rid of it and fix it properly for the next cycle.
Created attachment 237223 [details] [review] treeview: remove extra padding handling We'll replace this with a proper calculation.
Created attachment 237224 [details] [review] treeview: properly calculate the treeview expander size It should be expander-size + horizontal-separator / 2. Rework code calculating the render position of the arrow to account for the larger size.
Created attachment 237225 [details] [review] treeview: properly calculate the treeview expander size It should be expander-size + horizontal-separator / 2. Rework code calculating the render position of the arrow to account for the larger size.
Created attachment 237226 [details] [review] treeview: center expander allocated space This patch centers the expander in the extra space allocated by the horizontal-separator style property.
Attached patchset should implement pretty much what is proposed in comment #11 and comment #12.
Review of attachment 237223 [details] [review]: Looks good to me
Review of attachment 237225 [details] [review]: ok
Review of attachment 237226 [details] [review]: ok
Attachment 237223 [details] pushed as 32bd10b - treeview: remove extra padding handling Attachment 237225 [details] pushed as d0895d6 - treeview: properly calculate the treeview expander size Attachment 237226 [details] pushed as 50065b7 - treeview: center expander allocated space