After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 544103 - Eject icon in the sidebar should have light effect when hovered, or something
Eject icon in the sidebar should have light effect when hovered, or something
Status: RESOLVED FIXED
Product: nautilus
Classification: Core
Component: Sidebar
2.23.x
Other All
: Normal normal
: ---
Assigned To: Nautilus Maintainers
Nautilus Maintainers
: 556073 584846 (view as bug list)
Depends on: 586458
Blocks:
 
 
Reported: 2008-07-22 05:06 UTC by Diego Escalante Urrelo (not reading bugmail)
Modified: 2011-09-07 16:21 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
WIP - feedback wanted (5.80 KB, patch)
2010-06-12 13:42 UTC, Marcus Carlson
needs-work Details | Review
Places with two columns (24.89 KB, image/png)
2010-06-14 21:14 UTC, Marcus Carlson
  Details
Updated patch (not finished) (9.27 KB, patch)
2010-07-06 20:05 UTC, Marcus Carlson
needs-work Details | Review
Add highlight to eject icon in places sidebar (#544103) (8.06 KB, patch)
2010-08-06 13:37 UTC, Marcus Carlson
committed Details | Review

Description Diego Escalante Urrelo (not reading bugmail) 2008-07-22 05:06:54 UTC
It's not really "clickable" right now. Looks just like a pretty icon.
The light effect like the panel applets could do the job.
Comment 1 Baptiste Mille-Mathias 2008-07-22 16:36:30 UTC
yeah, I second that.
Comment 2 Christian Neumair 2008-07-22 19:00:09 UTC
Yeah, that would be a good idea, but require some efforts.

We'd probably have to sub class the GtkCellRendererPixbuf, and draw the shade of the icon, depending on the cell renderer state.
Comment 3 Cosimo Cecchi 2008-10-13 05:22:50 UTC
*** Bug 556073 has been marked as a duplicate of this bug. ***
Comment 4 Cosimo Cecchi 2010-04-30 08:28:19 UTC
*** Bug 584846 has been marked as a duplicate of this bug. ***
Comment 5 Omer Akram 2010-05-01 07:20:31 UTC
also reported at: https://launchpad.net/ubuntu/+source/nautilus/+bug/383255
Comment 6 Bastien Nocera 2010-05-20 14:21:14 UTC
This should be mostly solved if you use a symbolic icon in there. Should be a one-liner for somebody to test.
Comment 7 Milan Bouchet-Valat 2010-05-20 14:40:31 UTC
Hm, how would using a symbolic icon solve the hover problem? GtkTreeViewColumn doesn't handle mouse-enter events.
Comment 8 Marcus Carlson 2010-06-12 13:42:26 UTC
Created attachment 163471 [details] [review]
WIP - feedback wanted

I'm not sure how to solve this so here's a first try (patch is against 2.30.1). Is this the correct approach or should it be handled differently?

TODO: 
 * use a pixbuf column instead of string and make a highlighted image (how??)
 * rewrite against git master
 * reset icons on notify-leave
Comment 9 Milan Bouchet-Valat 2010-06-12 14:39:14 UTC
Interesting! Does that work as expected in all cases (sorry, I've not tried the patch)?

IMHO the best approach would mean fixing this so that GtkTreeRenderer gets a signal when pointer enters its area. That would allow all applications to benefit from the feature. Do you think you could try to add this to GTK+ itself? That's bug 586458. I know that's really tricky, but that would be a worthy addition. If you want to work on this, you can ping me on #gtk+ (milanbv), I can help (I can't guarantee that will be enough, though ;-).

About highlighting the icon, the best way to do this is to set the GTK_CELL_RENDERER_PRELIT flag when rendering the cell. This is already used currently when the pointer is on a row and some property I can't remember right now is set. See bug 586458, I've just added more details.
Comment 10 Marcus Carlson 2010-06-12 18:24:16 UTC
@Milan, it seems the nautilus places side bar only has one column packed with renderers so even with the gtk fix it will be the same as the "row" was selected.
That's why clicked_eject_button () is using such a hacky approach to find the position of the eject button instead of doing gtk_tree_view_get_path_at_pos ().

Any ideas?
Comment 11 Milan Bouchet-Valat 2010-06-13 09:02:35 UTC
Ah, yes. If the fix goes into GTK, then what prevents us from splitting the column into two? That would be the most logical solution.
Comment 12 Marcus Carlson 2010-06-14 21:14:46 UTC
Created attachment 163632 [details]
Places with two columns

Milan, I tried changing the list to use two different columns and ... it's ugly. The line between cannot be removed as it depends on the theme (clearlooks doesn't have it for instance, but then you get a break of 1-2px in the horizontal line).

I'd say we must have gtk to have per renderer prelight to have the most flexibility and the prettiest gui in nautilus. (Or to hack nautilus as in the patch).

Or any other ideas?
Comment 13 Milan Bouchet-Valat 2010-06-15 09:50:36 UTC
Argh... The GtkTreeView doesn't know about renderers, only about columns. So we would have to add another check to GtkTreeColumn to only highlight the renderer where the pointer is. Should be doable, but this means yet another check (plus strange effects if the pointer has mouved since the treeview decided to prelight?).

Have you tried with other themes, though? Ubuntu has customized Nautilus's pane, so it may look weirder than e.g. with Clearlooks.

Else, we can disable grid lines manually, that's not completely heretic:
http://library.gnome.org/devel/gtk/stable/GtkTreeView.html#GtkTreeViewGridLines

Note that for other uses we still want the per-cell prelight anyway: for example, a column with a checkbox plus text associated with it.
Comment 14 Cosimo Cecchi 2010-07-03 18:30:09 UTC
Review of attachment 163471 [details] [review]:

Thanks for the patch; your approach seems an acceptable workaround until we have something in GTK+ proper.
Here are some comments.

::: src/nautilus-places-sidebar.c
@@ +2363,3 @@
+	if (path && sidebar->eject_highlight_path && gtk_tree_path_compare (sidebar->eject_highlight_path, path) == 0) {
+		/* Same path - highlight up to date */
+		gtk_tree_path_free (path);		

Usually you don't free parameters passed in to functions.
Free |path| from the caller and copy it here if you need to save it somewhere else.

@@ +2375,3 @@
+
+		gtk_tree_model_get_iter (GTK_TREE_MODEL (sidebar->store), &iter, sidebar->eject_highlight_path);
+		gtk_list_store_set_value (GTK_LIST_STORE (sidebar->store), &iter, PLACES_SIDEBAR_COLUMN_EJECT_ICON, &eject_icon);

It's easier to use gtk_list_store_set (store, &iter, COLUMN, "icon-name", -1), here and below.

@@ +2385,3 @@
+	
+		g_value_init (&eject_icon, G_TYPE_STRING);
+		g_value_set_static_string (&eject_icon, "gtk-disconnect");

Why gtk-disconnect? I'd rahter use a highlight effect on the same 'media-eject' icon here. In this case, you will probably need to have a GdkPixbuf * column for the icons in the model, instead of a string one.

@@ +2396,3 @@
+static gboolean
+bookmarks_motion_event_cb (GtkWidget             *widget,
+			   GdkEventButton        *event,

This is a GdkEventMotion *, not a GdkEventButton *.

@@ +2406,3 @@
+	if (over_eject_button (sidebar, event, &path)) {
+		if (!path) {
+			return FALSE;

Are there cases where the function returns TRUE without setting path? Then you should either fix that, or remove this check.

@@ +2409,3 @@
+		}
+
+		update_eject_buttons (sidebar, path);

You should free path here.
Comment 15 Marcus Carlson 2010-07-06 20:05:52 UTC
Created attachment 165380 [details] [review]
Updated patch (not finished)

Cosimoc, I've got problem with the pixbuf. I don't know how to properly make highlighting correct (mclasen said there's no "symbolic" function) so I tried copying the function from gtkcellrendererpixbuf.c - but can only get it darker not lighter.

Also on todo list is to get reset on onmouseout.

Ideas?
Comment 16 Cosimo Cecchi 2010-07-07 23:28:39 UTC
(In reply to comment #15)

> Cosimoc, I've got problem with the pixbuf. I don't know how to properly make
> highlighting correct (mclasen said there's no "symbolic" function) so I tried
> copying the function from gtkcellrendererpixbuf.c - but can only get it darker
> not lighter.

Have a look at how we highlight the icons when cut in the views: eel_gdk_pixbuf_render (orig_pix, 1, 255, 255, 0, 0); is used for this.
Comment 17 Marcus Carlson 2010-08-06 13:37:06 UTC
Created attachment 167252 [details] [review]
Add highlight to eject icon in places sidebar (#544103)

I'd like some comments on this, seems to work quite good now.
Comment 18 Cosimo Cecchi 2010-08-22 16:51:10 UTC
Marcus, thanks for your efforts.

I committed your patch to master and pushed a bunch of other commits on top of it to fix some issues.

Closing as FIXED.
Comment 19 Milan Bouchet-Valat 2011-09-07 09:23:49 UTC
Reopening, I don't see this behavior in 3.0.2.
Comment 20 Cosimo Cecchi 2011-09-07 16:21:36 UTC
This works fine using git master, I just committed a patch [1] to make the highlight themable as well.

[1] http://git.gnome.org/browse/nautilus/commit/?id=a21d27b66fa307adaf87918f5feb2be44d52fdca