GNOME Bugzilla – Bug 759225
placesview: implement available space
Last modified: 2015-12-10 00:32:45 UTC
GtkPlacesView is a widget to display locations in the computer, such as root ("/") and volumes, separating the persistent devices from removable ones. From the latest mockups[1], GtkPlacesView would display the available space of local drives like partitions. This, however, is not implemented in the current codebase. Fix that by implementing the measurement of disk space, and adding a new property GtkPlacesView::show-disk-usage which controls the visibility of measured disks.
Created attachment 317003 [details] [review] placesview: implement available space
Created attachment 317004 [details] [review] placesview: implement available space GtkPlacesView is a widget to display locations in the computer, such as root ("/") and volumes, separating the persistent devices from removable ones. From the latest mockups[1], GtkPlacesView would display the available space of local drives like partitions. This, however, is not implemented in the current codebase. Fix that by implementing the measurement of disk space, and adding a new property GtkPlacesView::show-disk-usage which controls the visibility of measured disks.
Review of attachment 317003 [details] [review]: Dangling footnote there - I'd like to see those mockups, actually. ::: gtk/gtkplacesviewprivate.h @@ +90,3 @@ +GDK_AVAILABLE_IN_3_20 +void gtk_places_view_set_show_path (GtkPlacesView *view, + gboolean show_path); Why do we need this api ? Are there any plans to not show either of these ? ::: gtk/gtkplacesviewrow.c @@ +120,3 @@ + formatted_free_size = g_format_size (free_space); + formatted_total_size = g_format_size (total_space); + label = g_strdup_printf (P_("%s / %s available"), formatted_free_size, formatted_total_size); P_() is for properties. You need _() here (and a translator comment) @@ +158,3 @@ + row); + + gtk_label_set_label (row->available_space_label, P_("Measuring...")); P_() is wrong, but I don't think this is right anyway - if we ever see this string in the UI, we must be doing something wrong, like walking the filesystem to measure the size. Also, looking at the finish() implementation, if we ever get an error, we'll leave the string behind in the UI forever. I would suggest to leave the label blank until we have useful data to show.
Created attachment 317035 [details] [review] placesview: implement available space > Dangling footnote there - I'd like to see those mockups, actually. Added to the commit message, but to simplify our lives: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/nautilus/nautilus-next/other-locations.png > Why do we need this api ? > Are there any plans to not show either of these ? It was some noise that slipped through. Removed from the commit. > P_() is for properties. You need _() here (and a translator comment) I confused P_() for Public. Fixed. > > P_() is wrong, but I don't think this is right anyway - if we ever see this > string in the UI, we must be doing something wrong, like walking the > filesystem to measure the size. Also, looking at the finish() > implementation, if we ever get an error, we'll leave the string behind in > the UI forever. > > I would suggest to leave the label blank until we have useful data to show. Fixed.
Created attachment 317036 [details] [review] filechooserwidget: show available disk space in SAVE mode Adapts the file chooser widget to show available space when in SAVE or CREATE_FOLDER mode.
Created attachment 317039 [details] [review] placesview: implement available space Removed more noisy code.
Created attachment 317042 [details] [review] placesview: implement available space Fixed a tiny detail in gtk_places_view_row_set_show_disk_usage implementation.
Review of attachment 317036 [details] [review]: I don't think it makes much sense to hide the information in open mode, tbh. Thats why I asked about the need for the show property as api...
Review of attachment 317039 [details] [review]: ::: gtk/gtkplacesviewprivate.h @@ +88,2 @@ gboolean gtk_places_view_get_loading (GtkPlacesView *view); private headers are for private api. GDK_AVAILABLE_IN markers are for public api. The two don't mix...
Created attachment 317066 [details] [review] placesview: implement available space Indeed it wasn't really necessary, as GtkPlacesView is not a public widget. This patch should be enought for now, I hope.
Review of attachment 317066 [details] [review]: looks fine now, otherwise. ::: gtk/gtkplacesviewrow.c @@ +458,3 @@ row->is_network = is_network; + gtk_widget_set_visible (GTK_WIDGET (row->path_label), !is_network); Is this an unrelated change ? doesn't seem related tp space
Created attachment 317080 [details] [review] placesview: implement available space Indeed, more useless code. Fixed in this patch.
Thanks for the reviews. Attachment 317080 [details] pushed as 67125ae - placesview: implement available space
I know that the mockups use the same notation so this stems from there, but using "/" to mean "of" is ambiguous - it is also used in similar contexts to represent "," (two separate phrases). So A / B free can mean either: A (total), B (free). or A free of B total. After looking at the respective sizes it becomes apparent that this isn't the intention, but it does require a double-take to see that. Can I suggest changing to explicitly using "of" to avoid this ("A of B free"), or maybe "A free, B total"? I know it adds a bit of i18n/l10n overhead.
I see your point. Since English is my second language, I prefer to ask around for native speakers about this issue. Please, give me some time to find a better string.