GNOME Bugzilla – Bug 737983
gtkplacessidebar: Open $HOME after unmounting or ejecting
Last modified: 2014-10-26 10:41:14 UTC
See patch
Created attachment 287823 [details] [review] gtkplacessidebar: Open $HOME after unmounting or ejecting When a drive is ejected or a volume unmounted the current directory doesn't change most of the times being empty or being a directory that user shouldn't take care about, like /run/media Seems more useful to change to $HOME directory in that case so the user can see something useful and familiar just after unmounting.
Review of attachment 287823 [details] [review]: But we only want to switch to home if the unmounted/ejected volume contains the 'current' folder, right ? Is that always the case ?
Created attachment 288163 [details] [review] gtkplacessidebar: Don't change location if clicked on eject button Currently we change the current location if we click the eject button of a mount. Check whether the user actually clicked the eject button and don't change location in that case.
Created attachment 288164 [details] [review] gtkplacessidebar: Open $HOME after unmounting or ejecting When a drive is ejected or a volume unmounted the current directory doesn't change most of the times being empty or being a directory that user shouldn't take care about, like /run/media Seems more useful to change to $HOME directory in that case so the user can see something useful and familiar just after unmounting.
Review of attachment 288163 [details] [review]: looks good
Review of attachment 288164 [details] [review]: ::: gtk/gtkplacessidebar.c @@ +2721,3 @@ + GMount *mount, + GVolume *volume, + GDrive *drive) I think this could have a better name, like current_location_on_volume(), or so @@ +2737,3 @@ + mount_default_location = g_mount_get_default_location (mount); + needed_to_open_home = g_file_has_prefix (sidebar->current_location, mount_default_location) || + file_same_path (sidebar->current_location, mount_default_location); Are you sure the same_path check is necessary ? My understanding of prefix would be <=, so I think the same path case should already be covered by the prefix test. If it isn't, I'd consider that a gio bug. @@ +2742,3 @@ + // this code path is probably never reached since mount always exists, + // and if it doesn't exists we don't offer a way to eject a volume or + // drive in the ui. do it for defensive programming We don't use C99-style comments in GTK+ (mostly for historical reasons at this point), so please use /* */. And follow the convention we use for comment formatting: /* first line * more text */
Created attachment 288214 [details] [review] gtkplacessidebar: Open $HOME after unmounting or ejecting When a drive is ejected or a volume unmounted the current directory doesn't change most of the times being empty or being a directory that user shouldn't take care about, like /run/media Seems more useful to change to $HOME directory in that case so the user can see something useful and familiar just after unmounting.
Review of attachment 288214 [details] [review]: ::: gtk/gtkplacessidebar.c @@ +2737,3 @@ + mount_default_location = g_mount_get_default_location (mount); + current_location_on_volume = g_file_has_prefix (sidebar->current_location, mount_default_location) || + file_same_path (sidebar->current_location, mount_default_location); sorry to come back to this one more time - maybe it would be nicer to avoid open-coding the || in 2 or 3 places, and just have a helper function that does the '<= prefix' check ? like file_prefix_or_same (...) @@ +2971,3 @@ + *and if it doesn't exists we don't offer a way to eject a volume or + *drive in the UI. Do it for defensive programming + */ Almost :-) Now capitalize it and add a space between the * and the text. Sorry for being a pain
Created attachment 288223 [details] [review] gtkplacessidebar: Open $HOME after unmounting or ejecting When a drive is ejected or a volume unmounted the current directory doesn't change most of the times being empty or being a directory that user shouldn't take care about, like /run/media Seems more useful to change to $HOME directory in that case so the user can see something useful and familiar just after unmounting.
(In reply to comment #8) > Review of attachment 288214 [details] [review]: > > ::: gtk/gtkplacessidebar.c > @@ +2737,3 @@ > + mount_default_location = g_mount_get_default_location (mount); > + current_location_on_volume = g_file_has_prefix > (sidebar->current_location, mount_default_location) || > + file_same_path > (sidebar->current_location, mount_default_location); > > sorry to come back to this one more time - maybe it would be nicer to avoid > open-coding the || in 2 or 3 places, and just have a helper function that does > the '<= prefix' check ? like > > file_prefix_or_same (...) > Indeed, much better > @@ +2971,3 @@ > + *and if it doesn't exists we don't offer a way to eject a volume or > + *drive in the UI. Do it for defensive programming > + */ > > Almost :-) Now capitalize it and add a space between the * and the text. Sorry > for being a pain actually sorry on my side for not double-check with existing gtk+ multiline comment formatting.
Review of attachment 288223 [details] [review]: Looks good to commit, with that change ::: gtk/gtkplacessidebar.c @@ +2721,3 @@ + + return strcmp (file1path, file2path) == 0; +} g_file_get_path can return NULL, so g_strcmp0() is advisable here.
Thanks for the tip, didn't know we have such kind of functions. Attachment 288163 [details] pushed as 3c29212 - gtkplacessidebar: Don't change location if clicked on eject button Attachment 288223 [details] pushed as f6870e5 - gtkplacessidebar: Open $HOME after unmounting or ejecting
Review of attachment 288223 [details] [review]: I happened to see this going by and I saw some bugs with the code as committed - please see below. ::: gtk/gtkplacessidebar.c @@ +133,3 @@ GtkTrashMonitor *trash_monitor; GtkSettings *gtk_settings; + GFile *current_location; You don't seem to unref this in dispose() @@ +2720,3 @@ + file2path = g_file_get_path (file2); + + const gchar *file1path; This doesn't look right... - g_file_get_path() returns a newly allocated string that should be freed afterwards - you can just use g_file_equal() instead @@ +2787,3 @@ + } + } + g_object_unref (mount_default_location); Hmm, shouldn't this use g_list_free/g_list_free_full() instead? @@ +4716,3 @@ + if (sidebar->current_location != NULL) + g_object_unref (sidebar->current_location); + sidebar->current_location = g_file_dup (location); Can't you g_object_ref(location) instead of calling g_file_dup()?
Created attachment 288297 [details] [review] gtkplacessidebar: Fix commit f6870e5b79fd9ab Commit f6870e5b79fd9ab introduced a some memory leaks and could be improved in some areas. Fix the memory leaks and apply the improvements.
(In reply to comment #13) > Review of attachment 288223 [details] [review]: > > I happened to see this going by and I saw some bugs with the code as committed > - please see below. > > ::: gtk/gtkplacessidebar.c > @@ +133,3 @@ > GtkTrashMonitor *trash_monitor; > GtkSettings *gtk_settings; > + GFile *current_location; > > You don't seem to unref this in dispose() > > @@ +2720,3 @@ > + file2path = g_file_get_path (file2); > + > + const gchar *file1path; > > This doesn't look right... > - g_file_get_path() returns a newly allocated string that should be freed > afterwards > - you can just use g_file_equal() instead > > @@ +2787,3 @@ > + } > + } > + g_object_unref (mount_default_location); > > Hmm, shouldn't this use g_list_free/g_list_free_full() instead? > > @@ +4716,3 @@ > + if (sidebar->current_location != NULL) > + g_object_unref (sidebar->current_location); > + sidebar->current_location = g_file_dup (location); > > Can't you g_object_ref(location) instead of calling g_file_dup()? Thanks for the comments, and sorry for the bad code
Created attachment 288298 [details] [review] gtkplacessidebar: Fix commit f6870e5b79fd9ab Commit f6870e5b79fd9ab introduced a some memory leaks and could be improved in some areas. Fix the memory leaks and apply the improvements.
*** Bug 737987 has been marked as a duplicate of this bug. ***