GNOME Bugzilla – Bug 619665
Eject button: display unmount progress
Last modified: 2012-07-12 00:09:45 UTC
Problem Description: In the sidebar, after klicking the unmount icon I receive no information if the click got accepted. Clicking a 2nd time results in an error dialog (...Unmounting in progress...). Expected result: When clicking on the unmount icon in the sidebar, icon should change to a new symbol, indicating the unmount progress. This could be e.g. a spinner icon. Or the unmount icon should become inactive. I don't want a progress information like, displaying the remaining time needed. Just that some action takes place.
https://bugzilla.gnome.org/show_bug.cgi?id=571809 already addresses the error dialog issue. The throbber suggestion is a good one though. Let's just use this bug for that. Setting the status of this bug to new. Thanks for the bug report.
Created attachment 163737 [details] [review] WIP - feedback on approach wanted Not ready WIP patch that I would like some feedback on if this is the right approach or not. I've changed the datastore to make it easier (I think?). It doesn't handle all use cases either (eg eject) and no good error handling. Also - multiple "threads" can be started - bad?
Created attachment 163738 [details] Eye candy And some eye candy :)
Review of attachment 163737 [details] [review]: Quick review of the patch. I've not really considered the way it works, mostly formal issues. Could you explain the change with columns? That will help understanding the whole logic of the patch. ;-) ::: src/nautilus-places-sidebar.c @@ -117,3 @@ - PLACES_SIDEBAR_COLUMN_NO_EJECT, - PLACES_SIDEBAR_COLUMN_BOOKMARK, - PLACES_SIDEBAR_COLUMN_NO_BOOKMARK, Why do you need to remove these columns? Anyway, this already present NO_EJECT column is redundant, feels weird - I guess there must be a reason... If that's not stricly related to the patch, you should remove them in another one, to avoid mixing things. @@ +254,2 @@ PLACES_SIDEBAR_COLUMN_TOOLTIP, tooltip, + PLACES_SIDEBAR_COLUMN_SPINNER, 0, Use FALSE, not 0. @@ +1302,3 @@ + gboolean ret; + + if (!NAUTILUS_IS_PLACES_SIDEBAR (data)) { Is there any reason why this shouldn't be TRUE? Since you control the places where the function is called, that's not necessary. If somebody messes with the code one day, the cast below will warn, and a crash will happen. You're not a public API here... @@ +1308,3 @@ + ret = FALSE; + + NautilusPlacesSidebar *sidebar; Declare the variable at the begginning. You may even declare spin_timeout (NautilusPlacesSidebar *sidebar), since you cast it to GSource when registering the callback anyway. That's cleaner and clearer for everybody. @@ +1311,3 @@ + sidebar = NAUTILUS_PLACES_SIDEBAR (data); + + store = sidebar->store; ...and then you don't need an empty line before this! @@ +1314,3 @@ + + /* Get first row in list store */ + valid = gtk_tree_model_get_iter_first(GTK_TREE_MODEL(store), &iter); Missing space before (. @@ +1330,3 @@ + + /* Make iter point to the next row in the list store */ + valid = gtk_tree_model_iter_next(GTK_TREE_MODEL(store), &iter); I'd use: do { ... } while (valid = ...) @@ +1661,3 @@ + -1); + + g_timeout_add (80, (GSourceFunc)spin_timeout, sidebar); Use a constant to hold 80, since it's used in two different places. @@ +1732,3 @@ column = gtk_tree_view_get_column (GTK_TREE_VIEW (sidebar->tree_view), 0); renderers = gtk_cell_layout_get_cells (GTK_CELL_LAYOUT (column)); + cell = g_list_nth_data (renderers, 1); Hmm... First, better use renderers->data here, and why do you change this isolated line? @@ +2578,3 @@ gtk_tree_view_column_set_attributes (col, cell, "text", PLACES_SIDEBAR_COLUMN_NAME, + //"visible", PLACES_SIDEBAR_COLUMN_NO_EJECT, Stale comment here. And no need to cast with G_OBJECT (). Actually, the g_object_set() should be merged with the other one below. :-)
(In reply to comment #2) > Created an attachment (id=163737) [details] [review] > WIP - feedback on approach wanted > > Not ready WIP patch that I would like some feedback on if this is the right > approach or not. > > I've changed the datastore to make it easier (I think?). As I said in the review: not a good idea to change other things at the same time. Though if you can do some cleaning I'm sure maintainers will be glad to commit it as a separate patch! > It doesn't handle all > use cases either (eg eject) and no good error handling. What do you mean by "eject"? With right-click? Error handling shouldn't be too hard, you just have to remove the callback and let the error dialog do the rest, I guess. > Also - multiple > "threads" can be started - bad? I think you either need to avoid connecting a new callback if one is already running, or make them independent. The former can be done by storing the ID of the GSource in a variable and setting it to NULL when done. The advantage is that you can remove the timeout when the window closes, since you have the ID from the right place. But it would be nice to avoid going over all the bookmarks on each run of spin_timeout(). So passing it an iter on the row to update would be a nice improvement - and have one callback for each spinner. The problem is, will the iter remain valid until the end? Not sure that could work.
Changing component as a part of ongoing reorganisation work.
Marcus, feel like updating your patch? Now that the highlight part has been committed to master, this would be an even-nicer addition,
We should really try to do this for 3.2.
No news on that? Maybe it's not too late... ;-)
Sorry, I've got no time to update the patch at the moment.
*** Bug 599565 has been marked as a duplicate of this bug. ***
See also https://bugzilla.gnome.org/show_bug.cgi?id=670452 and https://bugzilla.redhat.com/show_bug.cgi?id=819492 : this bug is rather more important now, because the 'system wide' notification that it wasn't safe to remove a USB storage device yet was removed in 3.4. As things stand in 3.4 you have no way of knowing when it's safe to remove a stick you just 'ejected' via Nautilus. So it's quite important that something like this be implemented. As an alternative to a spinner, there could be a 'permanent' (bottom-centre of screen) notification that pops up, similar to the system-wide notification from 3.0 and 3.2.
Created attachment 214150 [details] [review] places-sidebar: add a notification while ejecting volumes This is a workarond solution until we come up with something better designed for GNOME 3.6 (by that time we'll also have a proper API in GIO for this, see [1]. [1] https://bugzilla.gnome.org/show_bug.cgi?id=676111
Created attachment 214209 [details] [review] places-sidebar: add a notification while ejecting volumes Updated not to show notifications for optical drives.
Created attachment 218465 [details] [review] places-sidebar: add a notification while ejecting volumes New patch, requires the GIO/GVfs API from bug 676111
Pushed this to master now.