GNOME Bugzilla – Bug 640741
gtk_tree_view_column_get_cell_position() seems to be broken
Last modified: 2011-09-25 15:49:36 UTC
After the treeview refactoring landed, I see a regression in nautilus. We currently use this code to determine whether a motion event is falling over a pixbuf render, so that we can apply a highlight effect to its pixbuf, which used to work fine until some time ago (you can see this by plugging an USB key or a removable device and hovering over the eject button in nautilus' places sidebar). http://git.gnome.org/browse/nautilus/tree/src/nautilus-places-sidebar.c#n899 Now it seems that the text renderer width is completely ignored, with the final result of the highlighting being off by several pixels. Pasting here a snippet of an IRC conversation with Tristan, might be useful for debugging purposes. <tristan> cosimoc, I think I found something <tristan> need to discuss it with kris <tristan> first, gtk_tree_view_column_get_cell_position() should use the gtk_tree_view_get_cell_area and not get_background_area() <tristan> second, I'm not sure that its valid to call gtk_cell_area_get_cell_allocation() with no height <tristan> so, I'm not sure that calling gtk_tree_view_get_cell_area() with no path can be any use to gtk_tree_view_column_get_cell_position()
tests/testtreepos.c in the gtk repo is supposed to test this.
*** Bug 643368 has been marked as a duplicate of this bug. ***
I am not sure what is actually causing the problem here (I don't have a build of GNOME3/nautilus handy here). Which width of which text renderer is ignored? There seem to be three text renders involved here. Perhaps of one of the text renderers of which the visibility is being toggled? Or is it only off by the horizontal separator width?
(In reply to comment #3) > Which width of which text renderer is ignored? > There seem to be three text renders involved here. Perhaps of one of the text > renderers of which the visibility is being toggled? Or is it only off by the > horizontal separator width? Yes, it's the width of the text renderer used to display the text for Eject entries that's ignored, i.e. this call returns 0 when it shouldn't: http://git.gnome.org/browse/nautilus/tree/src/nautilus-places-sidebar.c#n972
*** Bug 646189 has been marked as a duplicate of this bug. ***
Created attachment 184729 [details] [review] Fix up eject button hover in places sidebar We were calling gtk_tree_view_column_cell_get_position() without properly loading the cell attribute for the right row before. We fix this by calling gtk_tree_view_column_cell_set_cell_data(). With this in place we can also use the x_offset for the position and avoid the whole summing of widths. Due to a bug in Gtk which expands the eject icon cell renderer we have to right align it so that it lines up properly.
Actually, gtk_tree_view_column_cell_get_position() has two bugs in 3.x. 1. It always returns TRUE, but should return FALSE if the passed in cell is not part of the column. I just pushed a fix for this. 2. It returns the cell x_offset relative to bin_window, but should return the x_offset relative to the column. Essentially all code that uses this function is now broken. I think it tries to do the right thing here: { GdkRectangle button_allocation; /* Retrieve column offset */ gtk_widget_get_allocation (priv->button, &button_allocation); *x_offset = allocation.x - button_allocation.x; } But that's just not right, I don't know how the button is allocated, but button_allocation.x is NOT the column offset. It should simply return: *x_offset = allocation.x - cell_area.x (cell_area is the column's background area. If I apply this fix, my code in gimp starts working again, and i beleive so would nautilus. Ok to push?
Matthias: yes this tests is, but has only one column, so column.x is always the same as bin_window.x.
(In reply to comment #7) > But that's just not right, I don't know how the button is allocated, > but button_allocation.x is NOT the column offset. > > It should simply return: > > *x_offset = allocation.x - cell_area.x > > (cell_area is the column's background area. > > If I apply this fix, my code in gimp starts working again, and i beleive > so would nautilus. > > Ok to push? This seems to make sense. Will it work in RTL mode as well?
Gimp always had to check for rtl mode itself, and the bogus code didn't check for rtl either, so it can't get worse :)
Both issues fixed in master: commit c7cf1f531d9bb23ac21421dd5eededb7b6b6268e Author: Michael Natterer <mitch@gimp.org> Date: Sun Sep 25 17:43:45 2011 +0200 GtkTreeViewColumn: fix x_offset returned by cell_get_position() Fixes #640741 - gtk_tree_view_column_get_cell_position() seems to be broken It is supposed to return the offset within the column, but returned the offset within the tree, changed by allocation.x of the column's button (which I don't really unserstand and was clearly not working). gtk/gtktreeviewcolumn.c | 7 +------ 1 files changed, 1 insertions(+), 6 deletions(-) commit 33f7754a713969e26d3cc15b3543c5c3b35851a2 Author: Michael Natterer <mitch@gimp.org> Date: Sun Sep 25 14:50:31 2011 +0200 GtkTreeViewColumn: fix return value of gtk_tree_view_column_cell_get_position() Return FALSE again if the passed in cell is not part of the column. It was always returning TRUE since GtkCellArea was introduced. gtk/gtktreeviewcolumn.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)