After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 759225 - placesview: implement available space
placesview: implement available space
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: Other
unspecified
Other All
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2015-12-09 05:33 UTC by Georges Basile Stavracas Neto
Modified: 2015-12-10 00:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
placesview: implement available space (20.27 KB, patch)
2015-12-09 05:33 UTC, Georges Basile Stavracas Neto
reviewed Details | Review
placesview: implement available space (20.37 KB, patch)
2015-12-09 05:49 UTC, Georges Basile Stavracas Neto
none Details | Review
placesview: implement available space (17.89 KB, patch)
2015-12-09 14:10 UTC, Georges Basile Stavracas Neto
none Details | Review
filechooserwidget: show available disk space in SAVE mode (1.17 KB, patch)
2015-12-09 14:11 UTC, Georges Basile Stavracas Neto
none Details | Review
placesview: implement available space (17.53 KB, patch)
2015-12-09 14:26 UTC, Georges Basile Stavracas Neto
reviewed Details | Review
placesview: implement available space (17.54 KB, patch)
2015-12-09 14:38 UTC, Georges Basile Stavracas Neto
none Details | Review
placesview: implement available space (11.87 KB, patch)
2015-12-09 19:12 UTC, Georges Basile Stavracas Neto
none Details | Review
placesview: implement available space (11.45 KB, patch)
2015-12-09 22:28 UTC, Georges Basile Stavracas Neto
committed Details | Review

Description Georges Basile Stavracas Neto 2015-12-09 05:33:23 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.
Comment 1 Georges Basile Stavracas Neto 2015-12-09 05:33:28 UTC
Created attachment 317003 [details] [review]
placesview: implement available space
Comment 2 Georges Basile Stavracas Neto 2015-12-09 05:49:12 UTC
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.
Comment 3 Matthias Clasen 2015-12-09 11:52:20 UTC
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.
Comment 4 Georges Basile Stavracas Neto 2015-12-09 14:10:52 UTC
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.
Comment 5 Georges Basile Stavracas Neto 2015-12-09 14:11:35 UTC
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.
Comment 6 Georges Basile Stavracas Neto 2015-12-09 14:26:04 UTC
Created attachment 317039 [details] [review]
placesview: implement available space

Removed more noisy code.
Comment 7 Georges Basile Stavracas Neto 2015-12-09 14:38:00 UTC
Created attachment 317042 [details] [review]
placesview: implement available space

Fixed a tiny detail in gtk_places_view_row_set_show_disk_usage
implementation.
Comment 8 Matthias Clasen 2015-12-09 14:42:46 UTC
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...
Comment 9 Matthias Clasen 2015-12-09 14:42:47 UTC
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...
Comment 10 Matthias Clasen 2015-12-09 14:43:00 UTC
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...
Comment 11 Georges Basile Stavracas Neto 2015-12-09 19:12:56 UTC
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.
Comment 12 Matthias Clasen 2015-12-09 19:43:20 UTC
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
Comment 13 Georges Basile Stavracas Neto 2015-12-09 22:28:07 UTC
Created attachment 317080 [details] [review]
placesview: implement available space

Indeed, more useless code. Fixed in this patch.
Comment 14 Georges Basile Stavracas Neto 2015-12-09 22:31:32 UTC
Thanks for the reviews.

Attachment 317080 [details] pushed as 67125ae - placesview: implement available space
Comment 15 Stephen 2015-12-10 00:23:17 UTC
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.
Comment 16 Georges Basile Stavracas Neto 2015-12-10 00:32:45 UTC
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.