GNOME Bugzilla – Bug 756568
Some improvements to gtkplacesview
Last modified: 2015-10-15 17:26:39 UTC
Finish to fix most of the issues listed on https://wiki.gnome.org/Apps/Nautilus/Roadmap/OtherPlaces . Attaching here so it's easier for reviewers. Only issues remaining that could be fixable is the address hint, to which we need consensus to either leave as it is or change it as per the mockups.
Created attachment 313260 [details] [review] gtkplacesview: tweak ui to allow more server rows Following design guidance, reduce row height and increase popover height so the user is allowed to see more than 3 rows.
Created attachment 313261 [details] [review] gtkplacesview: rotate server list icon on toggled Disclosure triangles are usually used pointing down, however in this case the popover spawns in the upper direction, which makes it odd looking. Instead of pointing always down or up, point down when not toggled and animate a rotation when toggled.
Created attachment 313262 [details] [review] gtkplacesview: add a clear button to address entry So it allows a quick way to clear the entry.
Created attachment 313263 [details] [review] gtkplacesview: align spinner with header label Use the box margin top instead of the label margin top, so the spinner remains aligned with the header label.
Created attachment 313264 [details] [review] gtkplacesview: remove hover color from rows Since other views are not using hover neither
(In reply to Carlos Soriano from comment #5) > Created attachment 313264 [details] [review] [review] > gtkplacesview: remove hover color from rows > > Since other views are not using hover neither Mathias pointed that it's not a good solution to override activatable for hover. However, the only way to make a different hover style is overriding this .list-row.activatable class, if not, the style is not specifity enough. The original reason for activatable is: https://git.gnome.org/browse/gtk+/commit/?id=207e593075b01815973c4bfcb5a0d90f1de8d11e
(In reply to Carlos Soriano from comment #0) ... > Only issues remaining that could be fixable is the address hint, to which we > need consensus to either leave as it is or change it as per the mockups. I've reported this separately, as bug 756570.
Review of attachment 313260 [details] [review]: this looks ok
Review of attachment 313261 [details] [review]: I don't think I agree that it is "odd looking", really. At least not odd enough to justify custom theming...
Review of attachment 313262 [details] [review]: Looks ok, other than the preexisting problems in the function ::: gtk/gtkplacesview.c @@ +1852,3 @@ supported = FALSE; supported_protocols = g_vfs_get_supported_uri_schemes (g_vfs_get_default ()); + address = g_strdup (gtk_entry_get_text (GTK_ENTRY (priv->address_entry))); Looking at this function, I have two comments: 1. Why dup the address ? I don't see any need for that, saves a g_free and makes the goto out unnecessary. 2. On the other hand, you are leaking the scheme (independent of this patch)
Review of attachment 313263 [details] [review]: ::: gtk/gtkplacesview.c @@ +1995,3 @@ + "spacing", 6, + "margin-top", 6, + NULL); I don't really like the overuse of generic api. I'd much prefer to just gtk_widget_set_margin_top (header, 6);
Review of attachment 313264 [details] [review]: We've discussed this on irc. I dislike the way in which this overrides activatable, and as I said on irc, if we want to declare this list to be more content-nature than action-nature, we should use a more suitable style class for it rather than just hardcoding GtkPlacesView into the theme.
(In reply to Matthias Clasen from comment #9) > Review of attachment 313261 [details] [review] [review]: > > I don't think I agree that it is "odd looking", really. At least not odd > enough to justify custom theming... Allan, then down arrow?
(In reply to Matthias Clasen from comment #10) > Review of attachment 313262 [details] [review] [review]: > > Looks ok, other than the preexisting problems in the function > > ::: gtk/gtkplacesview.c > @@ +1852,3 @@ > supported = FALSE; > supported_protocols = g_vfs_get_supported_uri_schemes (g_vfs_get_default > ()); > + address = g_strdup (gtk_entry_get_text (GTK_ENTRY (priv->address_entry))); > > Looking at this function, I have two comments: > > 1. Why dup the address ? I don't see any need for that, saves a g_free and > makes the goto out unnecessary. > because: https://git.gnome.org/browse/gtk+/commit/?id=ecc698a282ac69e747026f58ac43eee7958be626 > 2. On the other hand, you are leaking the scheme (independent of this patch) Ok
(In reply to Matthias Clasen from comment #11) > Review of attachment 313263 [details] [review] [review]: > > ::: gtk/gtkplacesview.c > @@ +1995,3 @@ > + "spacing", 6, > + "margin-top", 6, > + NULL); > > I don't really like the overuse of generic api. I'd much prefer to just > gtk_widget_set_margin_top (header, 6); Me neither, I was just following how is done all over the class, to be consistent. I will change it.
(In reply to Matthias Clasen from comment #12) > Review of attachment 313264 [details] [review] [review]: > > We've discussed this on irc. I dislike the way in which this overrides > activatable, and as I said on irc, if we want to declare this list to be > more content-nature than action-nature, we should use a more suitable style > class for it rather than just hardcoding GtkPlacesView into the theme. Maybe I'm not good enough on CSS and I'm wrong, but there is no way to override the hover state of the list-box if not doing it this way, because the specifity of list-box.activatable:hover is greater than a custom class for say GtkPlacesViewRow:hover.
Created attachment 313330 [details] [review] gtkplacesview: plug leak
Created attachment 313331 [details] [review] gtkplacesview: align spinner with header label Use the box margin top instead of the label margin top, so the spinner remains aligned with the header label.
(In reply to Carlos Soriano from comment #13) > (In reply to Matthias Clasen from comment #9) > > Review of attachment 313261 [details] [review] [review] [review]: > > > > I don't think I agree that it is "odd looking", really. At least not odd > > enough to justify custom theming... > > Allan, then down arrow? I guess. It's a bit sad to lose the spinning arrow - it's really neat.
Review of attachment 313262 [details] [review]: ok then
Review of attachment 313261 [details] [review]: retracting my opposition. I don't want to make Allan sad
Review of attachment 313330 [details] [review]: ::: gtk/gtkplacesview.c @@ +1876,3 @@ gtk_widget_set_sensitive (priv->connect_button, supported); g_free (address); + g_clear_pointer (&scheme, g_free); Just use g_free here. You are aout to leave the scope, so setting the local variable to NULL is really pointless.
Review of attachment 313331 [details] [review]: ok
Review of attachment 313264 [details] [review]: hmm, I guess that is true (about specificity). Lets go with this for now, then
Created attachment 313390 [details] [review] gtkplacesview: plug leak
Thanks for reviews. Pushed the leak patch with the correction and with a small change to avoid free a non initialized scheme. Attachment 313260 [details] pushed as 831509f - gtkplacesview: tweak ui to allow more server rows Attachment 313261 [details] pushed as f9b6c07 - gtkplacesview: rotate server list icon on toggled Attachment 313262 [details] pushed as 9341f64 - gtkplacesview: add a clear button to address entry Attachment 313264 [details] pushed as d29d54a - gtkplacesview: remove hover color from rows Attachment 313331 [details] pushed as c368683 - gtkplacesview: align spinner with header label Attachment 313390 [details] pushed as 9cc3e63 - gtkplacesview: plug leak