Fix: For values of “Bytes transferred” and “Bytes/s” with 1000-based prefix names use 1000-based divisor instead of 1024-based #248

1. 
   ghost commented at 10:46 am on March 13, 2021: none

   v.0.21.0

   I saw in the GUI peer window in the “received” column 1007 KB, and after increasing to >=1024 I guess, it switched to 1 MB. I would have expected the display unit to change from KB to MB already at value >=1000.

   I looked into the code, and the values appear to be power-of-2 byte values, so the switching at >=1024 and not >=1000 seems correct. But the unit display is not precisely correct, binary prefixes should be used for power-of-2 byte values.

   To be correct, this PR changes KB/MB/GB to KiB/MiB/GiB. KB to kB and the divisor from 1024 to 1000.

2. hebasto added the label Feature on Mar 13, 2021
3. 
   ghost commented at 12:07 pm on March 13, 2021: none

   This is how it looks with this PR applied: Edit: outdated
4. in src/qt/guiutil.cpp:772 in 71d661359c outdated

   761@@ -762,11 +762,11 @@ QString formatBytes(uint64_t bytes)
   762     if(bytes < 1024)
   763         return QString(QObject::tr("%1 B")).arg(bytes);
   764     if(bytes < 1024 * 1024)
   765-        return QString(QObject::tr("%1 KB")).arg(bytes / 1024);
   766+        return QString(QObject::tr("%1 KiB")).arg(bytes / 1024);
   

   ---

   ---

   

   jarolrod commented at 3:47 am on March 15, 2021:

   0        return QString(QObject::tr("%1 KB")).arg(bytes / 1000);
   

   Shouldn’t we instead refactor to abide by the IEC 80000-13 standard where 1 KB=1000 bytes (base 10)? macOS and Unix use this IEC standard, Windows is one of the only systems still treating 1KB=1024 bytes (base 2).

   Or determine the system type and use its standard. So for Unix/macOS systems use base 10, for Windows use base 2.

   ---

   

   unknown commented at 5:24 pm on March 15, 2021:

   I would be ok with that. Also with KB/s and change the divisor there accordingly form 1024 to 1000 (see e.g. Ubuntu units policy, which says for network bandwidth base-10 should be used). Whatever approach to solve, having unit KB with divisor 1024 is not correct I think. Edit: network bandwidth is something other than network throughput/transfer rate.

   ---

   

   jarolrod commented at 5:29 pm on March 15, 2021:

   @wodry yeah, I would say change this to base-10 instead and update the pr/commit description

   ---

   

   unknown commented at 5:43 pm on March 15, 2021:

   Changed the title to be open for either approach. Would like to see more opinions.
5. 
   jarolrod commented at 3:49 am on March 15, 2021: member

   I think this mebibyte/gibibyte naming scheme can be confusing to non-technical users
6. DrahtBot added the label Needs rebase on Mar 15, 2021
7. DrahtBot removed the label Needs rebase on Mar 15, 2021
8. unknown renamed this:
   gui: Display binary prefixes for power-of-2 byte values
   Fix incorrect prefix name for values of "Bytes transferred" and "Bytes/s"
   on Mar 15, 2021
9. 
   hebasto commented at 9:25 pm on March 15, 2021: member

   Concept ACK on being consistent.

   My vote is for decimal prefixes for data transfer rates and amounts of the transferred data, that means usage of kB/s, kilobyte per second, and kB, kilobyte. Note, that the small letter “k” is a standard prefix that means “kilo”.

   https://en.wikipedia.org/wiki/Data-rate_units#Unit_prefixes:

   The letter K is often used as a non-standard abbreviation for 1,024, especially in “KB” to mean KiB, the kilobyte in its binary sense.

10. 
    ghost commented at 7:20 am on March 16, 2021: none

    My vote is for decimal prefixes

    Thanks, so you would vote to change the divisor from 1024 to 1000 together with renaming KB to kB, right?

11. 
    hebasto commented at 8:13 am on March 16, 2021: member

    My vote is for decimal prefixes

    Thanks, so you would vote to change the divisor from 1024 to 1000 together with renaming KB to kB, right?

    Yes. That is my suggestion.

12. 
    sipa commented at 5:40 pm on March 18, 2021: contributor

    No strong opinion on 1000-based vs 1024-based, but please use unit names consistent with what is chosen.
13. 
    ghost commented at 6:13 pm on March 18, 2021: none

    I updated so the prefixes are unchanged (aside of of KB->kB as @hebasto suggested), and just the divisor is corrected from 1024 to 1000.
14. 
    ghost commented at 6:22 pm on March 18, 2021: none

    Not sure if this * 1.0f is really needed or if there is a nicer solution. Just took it over.
15. 
    ghost commented at 7:11 pm on March 18, 2021: none

    This is how it looks now with the updated PR applied. The 1000-based prefixes have the advantage to be consistent with the 10-based scaling of the network throughput graph horizontal marker lines (e.g. 100 kB/s, 1000 kB/s like in the screenshot).

    

16. in src/qt/guiutil.cpp:765 in a414af954b outdated

    765-    if(bytes < 1024 * 1024)
    766-        return QObject::tr("%1 KB").arg(bytes / 1024);
    767-    if(bytes < 1024 * 1024 * 1024)
    768-        return QObject::tr("%1 MB").arg(bytes / 1024 / 1024);
    769+    if(bytes < 1000 * 1000)
    770+        return QObject::tr("%1 kB").arg(bytes / 1000);
    

    ---

    ---

    

    jarolrod commented at 11:55 pm on March 18, 2021:

    @hebasto: This seems like a good place for some context to translators. For example, look at the French translation below. I don’t know how the decimal unit kB is written in French (Edit: It’s written as ko), but how do we let a translator know that they are supposed to translate as thedecimal unit and not the binary unit?

    ---

    

    hebasto commented at 2:40 pm on March 19, 2021:

    Yes. We could add more translation context later, when https://github.com/bitcoin/bitcoin/issues/21465 approach is implemented.
17. 
    hebasto commented at 2:42 pm on March 19, 2021: member

    Approach ACK 200b503733ab2b78c023571f31157cb81597c3e2 @wodry

    Do you mind applying clang-format-diff.py to your commit?

18. in src/qt/trafficgraphwidget.cpp:133 in 200b503733 outdated

    127@@ -128,8 +128,9 @@ void TrafficGraphWidget::updateRates()
    128 
    129     quint64 bytesIn = clientModel->node().getTotalBytesRecv(),
    130             bytesOut = clientModel->node().getTotalBytesSent();
    131-    float inRate = (bytesIn - nLastBytesIn) / 1024.0f * 1000 / timer->interval();
    132-    float outRate = (bytesOut - nLastBytesOut) / 1024.0f * 1000 / timer->interval();
    133+    // Rates are in bytes/millisecond which is equal to kilobytes/second
    134+    float inRate = (bytesIn - nLastBytesIn) * 1.0f / timer->interval();
    135+    float outRate = (bytesOut - nLastBytesOut) * 1.0f / timer->interval();
    

    ---

    ---

    

    hebasto commented at 2:46 pm on March 19, 2021:

    Why * 1.0f is required?

    ---

    

    unknown commented at 6:00 pm on March 19, 2021:

    I am no C++ expert, just took it over from / 1024.0f * 1000 before. If the 1024.0f was also not needed (I imagined it might have had some reason of C++ type conversion), I will remove the * 1.0f

    ---

    

    hebasto commented at 6:03 pm on March 19, 2021:

    If type conversion is supposed, it would better to apply explicit conversion.
19. in src/qt/trafficgraphwidget.cpp:131 in 200b503733 outdated

    127@@ -128,8 +128,9 @@ void TrafficGraphWidget::updateRates()
    128 
    129     quint64 bytesIn = clientModel->node().getTotalBytesRecv(),
    130             bytesOut = clientModel->node().getTotalBytesSent();
    131-    float inRate = (bytesIn - nLastBytesIn) / 1024.0f * 1000 / timer->interval();
    132-    float outRate = (bytesOut - nLastBytesOut) / 1024.0f * 1000 / timer->interval();
    133+    // Rates are in bytes/millisecond which is equal to kilobytes/second
    

    ---

    ---

    

    hebasto commented at 2:51 pm on March 19, 2021:

    Instead of adding a comment, is it better just choose proper names for variables? E.g., in_rate_kilobytes_per_sec ?
20. 
    ghost commented at 6:08 pm on March 19, 2021: none

    Approach ACK 200b503

    @wodry

    Do you mind applying clang-format-diff.py to your commit?

    Thanks for the tipp, have done that, will update.

21. 
    hebasto commented at 6:26 pm on March 19, 2021: member

    May I suggest a change:

     0--- a/src/qt/guiutil.cpp
     1+++ b/src/qt/guiutil.cpp
     2@@ -759,14 +759,14 @@ QString formatNiceTimeOffset(qint64 secs)
     3 
     4 QString formatBytes(uint64_t bytes)
     5 {
     6-    if (bytes < 1000)
     7+    if (bytes < 1'000)
     8         return QObject::tr("%1 B").arg(bytes);
     9-    if (bytes < 1000 * 1000)
    10-        return QObject::tr("%1 kB").arg(bytes / 1000);
    11-    if (bytes < 1000 * 1000 * 1000)
    12-        return QObject::tr("%1 MB").arg(bytes / 1000 / 1000);
    13+    if (bytes < 1'000'000)
    14+        return QObject::tr("%1 kB").arg(bytes / 1'000);
    15+    if (bytes < 1'000'000'000)
    16+        return QObject::tr("%1 MB").arg(bytes / 1'000'000);
    17 
    18-    return QObject::tr("%1 GB").arg(bytes / 1000 / 1000 / 1000);
    19+    return QObject::tr("%1 GB").arg(bytes / 1'000'000'000);
    20 }
    21 
    22 qreal calculateIdealFontSize(int width, const QString& text, QFont font, qreal minPointSize, qreal font_size) {
    23diff --git a/src/qt/trafficgraphwidget.cpp b/src/qt/trafficgraphwidget.cpp
    24index 9e0b67917..7e12410c8 100644
    25--- a/src/qt/trafficgraphwidget.cpp
    26+++ b/src/qt/trafficgraphwidget.cpp
    27@@ -128,8 +128,8 @@ void TrafficGraphWidget::updateRates()
    28 
    29     quint64 bytesIn = clientModel->node().getTotalBytesRecv(),
    30             bytesOut = clientModel->node().getTotalBytesSent();
    31-    float in_rate_kilobytes_per_sec = (bytesIn - nLastBytesIn) / timer->interval();
    32-    float out_rate_kilobytes_per_sec = (bytesOut - nLastBytesOut) / timer->interval();
    33+    float in_rate_kilobytes_per_sec = static_cast<float>(bytesIn - nLastBytesIn) / timer->interval();
    34+    float out_rate_kilobytes_per_sec = static_cast<float>(bytesOut - nLastBytesOut) / timer->interval();
    35     vSamplesIn.push_front(in_rate_kilobytes_per_sec);
    36     vSamplesOut.push_front(out_rate_kilobytes_per_sec);
    37     nLastBytesIn = bytesIn;
    

    ?

    It makes qt/guiutil.cpp more readable due to the nice C++17 feature :), and applies correct type conversion (without it the result will be integer by value).

22. Fix wrong(1024) divisor for 1000-based prefixes d09ebc4723
23. 
    ghost commented at 7:07 pm on March 19, 2021: none

    Thanks very much for the high quality coding hints! Updated commit. (and did a successfull local build on Debian Buster (Qt 5.11.3) and test run, too)
24. hebasto approved
25. 
    hebasto commented at 7:35 pm on March 19, 2021: member

    ACK d09ebc47233187ab8dd5a70df4d261353958978c, tested on Linux Mint 20.1 (Qt 5.12.8) the both “Network Traffic” and “Peers” tabs of the “Node Window”.
26. unknown renamed this:
    Fix incorrect prefix name for values of "Bytes transferred" and "Bytes/s"
    Fix: For values of "Bytes transferred" and "Bytes/s" with 1000-based prefix names use divisor 1000 instead of 1024
    on Mar 19, 2021
27. unknown renamed this:
    Fix: For values of "Bytes transferred" and "Bytes/s" with 1000-based prefix names use divisor 1000 instead of 1024
    Fix: For values of "Bytes transferred" and "Bytes/s" with 1000-based prefix names use 1000-based divisor instead of 1024-based
    on Mar 19, 2021
28. 
    jarolrod commented at 1:34 pm on March 22, 2021: member

    ACK d09ebc47233187ab8dd5a70df4d261353958978c

    Tested on macOS 11.1 Qt 5.15.2

29. 
    hebasto commented at 1:49 pm on March 22, 2021: member

    @jonatack Do you mind looking into this PR?
30. 
    leonardojobim commented at 9:19 pm on March 22, 2021: none

    Tested ACK https://github.com/bitcoin-core/gui/pull/248/commits/d09ebc47233187ab8dd5a70df4d261353958978c on Ubuntu 20.04 Qt 5.12.8
31. 
    hebasto commented at 12:59 pm on March 23, 2021: member

    Additional discussion from IRC:

    <Arvidt> Any preference for having base-10 (kB/MB/GB) or base-2 (KiB/MiB/GiB) prefix for GUI network peer tab data transferred/throughput value? See #248 Would be thankful to get some feedback. <luke-jr> Arvidt: base 10 sucks ;) <hebasto> luke-jr: what reasons for? <luke-jr> hebasto: 10 is a multiple of 5 and 2 <luke-jr> but more importantly, networks tend to use base-1024 measurements <sipa> my gigabit ethernet does 1000000000 bits/s <luke-jr> sipa: it does? :o <sipa> yes <luke-jr> hmm <sipa> all network stuff is 1000-based, as far as i know’ <sipa> only disk sizes use 1024-based units conventionally <sipa> or on-disk sizes <luke-jr> RAM typically does even now <sipa> that’s true, RAM is also typically 1024-based

32. 
    jnewbery commented at 2:26 pm on March 23, 2021: contributor

    Concept ACK. Using powers of 10 and correctly calling them kB and MB seems reasonable for a user-facing GUI.
33. MarcoFalke merged this on Mar 23, 2021
34. MarcoFalke closed this on Mar 23, 2021

    ---

35. sidhujag referenced this in commit 547c1e3cdd on Mar 23, 2021
36. unknown deleted the branch on Mar 28, 2021
37. gwillen referenced this in commit b4c1a6296c on Jun 1, 2022
38. bitcoin-core locked this on Aug 16, 2022

Contributors
ghost jarolrod hebasto sipa leonardojobim jnewbery

Labels
Feature