Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom layperson-friendly format size #252

Merged
merged 19 commits into from
Jan 26, 2023
Merged

Custom layperson-friendly format size #252

merged 19 commits into from
Jan 26, 2023

Conversation

vjr
Copy link
Member

@vjr vjr commented Oct 4, 2022

The GLib.format_size () function returns a single decimal place such as 16.0 GiB (for memory) and 512.1 GB (for storage) while this PR returns 16 GB and 512 GB instead, with the intent of being user-friendly for the layperson. Returns "GB" instead of "GiB" (for example) even if IEC_UNITS is requested.

Before:

about_after2

After:

about_after3

@vjr
Copy link
Member Author

vjr commented Oct 4, 2022

I'm not sure if anything further needs to be done for the newly added translate strings?

@tintou
Copy link
Member

tintou commented Oct 4, 2022

I'm -1 on this, we have to use the right unit.

@vjr vjr requested a review from a team October 4, 2022 08:47
@tintou
Copy link
Member

tintou commented Oct 4, 2022

From the GLib documentation:

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.

@vjr
Copy link
Member Author

vjr commented Oct 4, 2022

@tintou yes I understand your -1 on this :-)

This proposed PR was for the "average everyday layperson" who might see "GiB" for memory and "GB" for storage and wonder what the "GiB" indicates?

For people who don't know/understand the difference between IEC/SI mebi versus mega bytes , for example?

@tintou
Copy link
Member

tintou commented Oct 4, 2022

If people don't know the difference I feel like they can search on their own if they are really interested in this :)

I'm sure that if they don't understand it, they are anyway seeing the unit as a opaque thing so there is no difference between MiB and MB for them 🙃

@vjr
Copy link
Member Author

vjr commented Oct 4, 2022

@tintou how does it look now? (both code and screenshot?)

about_after4

@vjr
Copy link
Member Author

vjr commented Oct 4, 2022

If people don't know the difference I feel like they can search on their own if they are really interested in this :)

I'm sure that if they don't understand it, they are anyway seeing the unit as a opaque thing so there is no difference between MiB and MB for them upside_down_face

Personally I don't agree with "no difference between SI and IEC for ordinary users" - I think (I guess) average user knows what "MB" and "GB" are but not "MiB" and "GiB" which is why I prefer showing SI units for the memory label too, but not a strong opinion.

@vjr
Copy link
Member Author

vjr commented Oct 4, 2022

Note to self - TODO update comment to reflect change in whether to show IEC unit.

@vjr
Copy link
Member Author

vjr commented Oct 5, 2022

I just looked at my Windows and Android devices and they show the memory capacity unit as GB and not GiB even though the value is of course IEC.

I searched on Google for screenshots of system information sections on macOS and iOS and there too it's GB instead of GiB.

I know this is a nitpick PR to propose but whenever I look at the current hardware view in OS 6.1 showing "15.7 GiB memory" instead of the proposed "16 GB memory" it triggers some sort of OCD in me :-P

@avojak
Copy link

avojak commented Oct 5, 2022

Just my 2¢ but I think it’s a good change. I can’t think of the last time I saw a computer advertised using GiB of RAM. I don’t have any evidence at all to back this up, but I think a "layperson" would be more confused seeing 15.7 GiB and wondering what happened to the other 0.3. I don’t like the argument that a user should go look up the difference themselves. Is GB the most technically accurate? No, but it doesn’t need to be in this case. In this context, I would expect my system to report in a format that matches how it would be advertised or generally discussed. If someone really cares that much, there are other system utilities to find exact numbers and whatnot.

@vjr
Copy link
Member Author

vjr commented Oct 15, 2022

Updated the PR back to the original proposal like here: #252 (comment)

about_si_unit

lenemter
lenemter previously approved these changes Nov 1, 2022
@vjr
Copy link
Member Author

vjr commented Nov 1, 2022

Thanks @lenemter for the approval. I'll wait for maybe @danirabbit to comment on whether this change gets merged?

@lenemter lenemter requested a review from danirabbit November 1, 2022 15:16
tintou
tintou previously requested changes Nov 7, 2022
Copy link
Member

@tintou tintou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the consensus is to merge it (which I'm still not convinced), here are a few things to fix before

src/Views/HardwareView.vala Outdated Show resolved Hide resolved
src/Views/HardwareView.vala Outdated Show resolved Hide resolved
@vjr
Copy link
Member Author

vjr commented Nov 7, 2022

Thank you @tintou for the review.

I can look into address this over the weekend but might need help with the ngettext and i18n - anyone? Maybe @lenemter would you?

@lenemter
Copy link
Member

lenemter commented Nov 12, 2022

@vjr I've looked at GLib.format_size source code (https://github.com/GNOME/glib/blob/main/glib/gutils.c#L2837) and edited your function a bit to make it as translatable as GLib one:

    private string custom_format_size (uint64 size, bool iec_unit) {
        uint divisor = iec_unit ? 1024 : 1000;

        string[] units = {_("KB"), _("MB"), _("GB"), _("TB"), _("PB")};

        int unit_index = 0;

        while ((size / divisor) > 0 && (unit_index < units.length)) {
            unit_index++;
            size /= divisor;
        }

        string unit;
        if (unit_index == 0) {
            // TRANSLATORS: Examples of use: "1 byte", "13 bytes"
            unit = ngettext ("byte", "bytes", (ulong) size);
        } else {
            unit = units[unit_index - 1];
        }

        /* TRANSLATORS: The first "%llu" is replaced with the value, the "%s" with a unit of the value.
           The order can be changed with "%$2s %$1llu". An example: "13 bytes" */
        return _("%llu %s").printf (size, unit);
    }

@vjr vjr requested a review from tintou November 12, 2022 14:19
@vjr
Copy link
Member Author

vjr commented Nov 12, 2022

Thanks @lenemter for the code! @tintou would you re-review?

@danirabbit danirabbit requested a review from lenemter January 26, 2023 16:32
@vjr vjr dismissed tintou’s stale review January 26, 2023 18:43

Addressed and approved.

@vjr vjr merged commit aa359cd into master Jan 26, 2023
@vjr vjr deleted the custom-format-size branch January 26, 2023 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants