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 693207 - boxes uses different units from all the others
boxes uses different units from all the others
Status: RESOLVED FIXED
Product: gnome-boxes
Classification: Applications
Component: properties
unspecified
Other Linux
: Normal normal
: --
Assigned To: GNOME Boxes maintainer(s)
GNOME Boxes maintainer(s)
Depends on:
Blocks: 686781
 
 
Reported: 2013-02-05 15:20 UTC by Matthias Clasen
Modified: 2016-03-31 14:02 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Dont use IEC units (3.46 KB, patch)
2013-02-06 02:44 UTC, Zeeshan Ali
needs-work Details | Review
properties: Allow configuring format of size strings (3.76 KB, patch)
2013-02-15 15:15 UTC, Zeeshan Ali
committed Details | Review
wizard,libvirt-machine: Don't use IEC units for storage (1.98 KB, patch)
2013-02-15 15:15 UTC, Zeeshan Ali
accepted-commit_now Details | Review
wizard,libvirt-machine: Don't use IEC units for storage (2.74 KB, patch)
2013-02-18 15:00 UTC, Zeeshan Ali
committed Details | Review

Description Matthias Clasen 2013-02-05 15:20:02 UTC
in the boxes ui for changing disk size, I see GiB. That gave me some raised eyebrows in a recent demonstration, and claims of inconsistency. And indeed, comparing the rest of the desktop:

nautilus - GB
disks - GB
baobab - GB
Comment 1 Alexander Larsson 2013-02-05 15:23:19 UTC
Yes, we should use G_FORMAT_SIZE_DEFAULT except for the ram size, which should be G_FORMAT_SIZE_IEC_UNITS.
Comment 2 Zeeshan Ali 2013-02-05 20:58:49 UTC
Yeah, I'll fix this after I'm done with bug#688333.
Comment 3 Zeeshan Ali 2013-02-06 02:44:23 UTC
Created attachment 235281 [details] [review]
Dont use IEC units

Use the default size units of GLib.format_size() for consistancy with
other GNOME modules (e.g nautilus, disks and baobab etc).
Comment 4 Alexander Larsson 2013-02-06 10:23:10 UTC
Review of attachment 235281 [details] [review]:

I don't think this is quite right. The format_size docs say:

 * @G_FORMAT_SIZE_IEC_UNITS: use IEC (base 1024) units with "KiB"-style
 *     suffixes. IEC units should only be used for reporting things with
 *     a strong "power of 2" basis, like RAM sizes or RAID stripe sizes.
 *     Network and storage sizes should be reported in the normal SI units.

So, we should probably still use them for the RAM size.

Also, using different units for the presentations means we have to use the right basis for the step size (i.e. 1000 based for disk, 1024 based for ram).
Comment 5 Zeeshan Ali 2013-02-06 13:01:33 UTC
(In reply to comment #4)
> Review of attachment 235281 [details] [review]:
> 
> I don't think this is quite right. The format_size docs say:
> 
>  * @G_FORMAT_SIZE_IEC_UNITS: use IEC (base 1024) units with "KiB"-style
>  *     suffixes. IEC units should only be used for reporting things with
>  *     a strong "power of 2" basis, like RAM sizes or RAID stripe sizes.
>  *     Network and storage sizes should be reported in the normal SI units.
> 
> So, we should probably still use them for the RAM size.

But IMHO that would look very inconsistent and therefore bad in the UI. :( Wouldn't it?

> Also, using different units for the presentations means we have to use the
> right basis for the step size (i.e. 1000 based for disk, 1024 based for ram).

I changed that in bug#688333.
Comment 6 Alexander Larsson 2013-02-06 13:25:09 UTC
Well, its what our docs say... Guess we should bring it up somewhere if we disagree with it.

The step size *value* was changed, yes, but not the base. With that change it is 
64 * Osinfo.MEBIBYTES == 64 * 1024 * 1024, which is crap when we render numbers with a base of 1000 * 1000.
Comment 7 Zeeshan Ali 2013-02-06 13:40:21 UTC
(In reply to comment #6)
> Well, its what our docs say... Guess we should bring it up somewhere if we
> disagree with it.

I wonder if the doc writer was thinking about these getting presented at the same place/page in the UI. The only existing GNOME app that I could think of that uses both on the same page is system monitor and that uses IEC units for both RAM and storage. I think our case is more similar to that of system monitor than nautilus as accuracy is kinda important for us too. Although Disks uses GB but it uses the full format. Maybe we should do the same for both for 'current' size label and use the default (short) format on min and max?

> The step size *value* was changed, yes, but not the base. With that change it
> is 
> 64 * Osinfo.MEBIBYTES == 64 * 1024 * 1024, which is crap when we render numbers
> with a base of 1000 * 1000.

Yeah, now I get it.
Comment 8 Zeeshan Ali 2013-02-06 13:56:04 UTC
Oh and mockups use default untis for both: https://github.com/gnome-design-team/gnome-mockups/raw/master/boxes/boxes-install5.5.png
Comment 9 Matthias Clasen 2013-02-12 01:46:54 UTC
a few more cases in point:

gnome-system-monitor: uses GiB for memory sizes... but then also uses it for disk space

details panel, control-center: uses GiB for memory and GB for disk space
Comment 10 Alexander Larsson 2013-02-15 09:16:53 UTC
I think GiB for ram and GB for disk is right. Its somewhat confusing, but so are any other choices and there is at least a reasonable rationale for this particular choice (and its in-line with the glib docs).
Comment 11 Zeeshan Ali 2013-02-15 15:15:14 UTC
Created attachment 236251 [details] [review]
properties: Allow configuring format of size strings

i-e Whether to use default format or IEC.
Comment 12 Zeeshan Ali 2013-02-15 15:15:16 UTC
Created attachment 236252 [details] [review]
wizard,libvirt-machine: Don't use IEC units for storage
Comment 13 Alexander Larsson 2013-02-18 14:31:11 UTC
Both these look ok to me, but you also need to change the step size so that its in the right kind of unit wrt the format of the slide.
Comment 14 Alexander Larsson 2013-02-18 14:31:26 UTC
Review of attachment 236251 [details] [review]:

ack
Comment 15 Alexander Larsson 2013-02-18 14:31:40 UTC
Review of attachment 236252 [details] [review]:

ack (but you also need to change step side)
Comment 16 Zeeshan Ali 2013-02-18 15:00:21 UTC
Created attachment 236603 [details] [review]
wizard,libvirt-machine: Don't use IEC units for storage
Comment 17 Alexander Larsson 2013-02-18 15:30:24 UTC
Review of attachment 236603 [details] [review]:

ack
Comment 18 Zeeshan Ali 2013-02-18 15:36:43 UTC
Attachment 236251 [details] pushed as e581dfd - properties: Allow configuring format of size strings
Attachment 236603 [details] pushed as b8c2d4c - wizard,libvirt-machine: Don't use IEC units for storage