GNOME Bugzilla – Bug 753786
Implement high priority improvements for gtkplacesview
Last modified: 2015-08-21 11:37:27 UTC
See https://wiki.gnome.org/Apps/Nautilus/Roadmap/OtherPlaces
Created attachment 309520 [details] [review] gtkplacesview: style fix
Created attachment 309521 [details] [review] gtkplacesview: add networks in network:/// Previously we had a network item in the sidebar, which now is replaced by the network section on other-locations view. However we were not exposing the networks in network:///. Fetch them and add them in the network section of other-locations view.
Created attachment 309522 [details] [review] gtkplacesview: improve networks feedback Add a spinner when networks are being fetched and make the network section permanent and show a placeholder with a message that no networks were found in case there are no networks. In this way users from previous versions won't be confused with the fact that no networks are shown.
Created attachment 309523 [details] [review] gtkplacesview: improve networks feedback Add a spinner when networks are being fetched and make the network section permanent and show a placeholder with a message that no networks were found in case there are no networks. In this way users from previous versions won't be confused with the fact that no networks are shown.
Created attachment 309576 [details] [review] gtkplacesview: add a loading property So clients of the view can know if the view is busy.
Review of attachment 309520 [details] [review]: A harmless patch. LGTM.
Review of attachment 309521 [details] [review]: This patch was reviewed earlier. LGTM.
Review of attachment 309523 [details] [review]: Another patch that was reviewed earlier. LGTM.
Review of attachment 309576 [details] [review]: Nice work.
Review of attachment 309521 [details] [review]: ::: gtk/gtkplacesview.c @@ +827,3 @@ + for (l = priv->detected_networks; l != NULL; l = l->next) + { + gchar *display_name; This is leaked @@ +828,3 @@ + { + file = g_file_enumerator_get_child (priv->detected_networks_enumerator, l->data); + gchar *display_name; This is leaked @@ +853,3 @@ + g_clear_object (&priv->detected_networks_enumerator); + +} Why are you assigning the enumerator here? You don't actually need to store it in the state, and it will end up being leaked. If you really need to store it, network_enumeration_finished() would be a better place. @@ +854,3 @@ + + priv->detected_networks_enumerator = G_FILE_ENUMERATOR (source_object); +} Doesn't look like you really need to store these in the private state. @@ +858,3 @@ + if (error) + { +static void Should use g_error_matches() @@ +894,3 @@ + { + g_clear_object (&priv->networks_fetching_cancellable); + priv->detected_networks_enumerator = G_FILE_ENUMERATOR (source_object); Should be no need to recreate the cancellable here. @@ +913,3 @@ + network_file = g_file_new_for_uri ("network:///"); + + else Will this line not cause a critical trying to call this on a NULL object when the widget is first constructed?
Review of attachment 309521 [details] [review]: Many thanks for your review Cosimo! Will update the patch soon. ::: gtk/gtkplacesview.c @@ +913,3 @@ + network_file = g_file_new_for_uri ("network:///"); + + else No, quoting from docs "If cancellable is NULL, this function returns immediately for convenience." and it's actually very convenient
Created attachment 309687 [details] [review] gtkplacesview: add networks in network:/// Previously we had a network item in the sidebar, which now is replaced by the network section on other-locations view. However we were not exposing the networks in network:///. Fetch them and add them in the network section of other-locations view.
Created attachment 309689 [details] [review] gtkplacesview: add networks in network:/// Previously we had a network item in the sidebar, which now is replaced by the network section on other-locations view. However we were not exposing the networks in network:///. Fetch them and add them in the network section of other-locations view.
Review of attachment 309689 [details] [review]: Few other minor-ish comments ::: gtk/gtkplacesview.c @@ +868,3 @@ + priv->fetching_networks = FALSE; + + gpointer user_data) I don't think you need to keep these as a state, but if you do they are not cleared in finalize. You should be able to just fetch them locally in populate_networks() though. @@ +902,3 @@ + { + if (error->code != G_IO_ERROR_CANCELLED) + { Use g_error_matches() @@ +913,3 @@ + priv->networks_fetching_cancellable, + network_enumeration_next_files_finished, + g_clear_object (&source_object); You should be able to unref the enumerator here instead of doing it in every callback code path. @@ +928,3 @@ + g_cancellable_cancel (priv->networks_fetching_cancellable); + g_clear_object (&priv->networks_fetching_cancellable); +static void You should set priv->fetching_networks to TRUE here instead of the later patch
Are you going to request a freeze break for these ?
(In reply to Matthias Clasen from comment #16) > Are you going to request a freeze break for these ? It's the freeze request you aproved.
Created attachment 309803 [details] [review] gtkplacesview: style fix
Created attachment 309804 [details] [review] gtkplacesview: add networks in network:/// Previously we had a network item in the sidebar, which now is replaced by the network section on other-locations view. However we were not exposing the networks in network:///. Fetch them and add them in the network section of other-locations view.
Created attachment 309805 [details] [review] gtkplacesview: improve networks feedback Add a spinner when networks are being fetched and make the network section permanent and show a placeholder with a message that no networks were found in case there are no networks. In this way users from previous versions won't be confused with the fact that no networks are shown.
Created attachment 309806 [details] [review] gtkplacesview: add a loading property So clients of the view can know if the view is busy.
UI freeze accepted, so pushing with comments fixed. Thanks Cosimo and Georges! Attachment 309803 [details] pushed as 16bea59 - gtkplacesview: style fix Attachment 309804 [details] pushed as 7347a69 - gtkplacesview: add networks in network:/// Attachment 309805 [details] pushed as 89a3421 - gtkplacesview: improve networks feedback Attachment 309806 [details] pushed as 036ba25 - gtkplacesview: add a loading property