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

ghost · 2021-03-13T10:46:43Z

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.

ghost · 2021-03-13T12:07:12Z

~~This is how it looks with this PR applied:~~ Edit: outdated

jarolrod · 2021-03-15T03:47:30Z

src/qt/guiutil.cpp

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


Suggested change

return QString(QObject::tr("%1 KiB")).arg(bytes / 1024);

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.

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.

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

Changed the title to be open for either approach. Would like to see more opinions.

jarolrod

I think this mebibyte/gibibyte naming scheme can be confusing to non-technical users

hebasto · 2021-03-15T21:25:09Z

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.

ghost · 2021-03-16T07:20:14Z

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?

hebasto · 2021-03-16T08:13:41Z

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.

sipa · 2021-03-18T17:40:03Z

No strong opinion on 1000-based vs 1024-based, but please use unit names consistent with what is chosen.

ghost · 2021-03-18T18:13:36Z

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.

ghost · 2021-03-18T18:22:24Z

Not sure if this * 1.0f is really needed or if there is a nicer solution. Just took it over.

ghost · 2021-03-18T19:11:40Z

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).

jarolrod · 2021-03-18T23:55:47Z

src/qt/guiutil.cpp

-    if(bytes < 1024 * 1024 * 1024)
-        return QObject::tr("%1 MB").arg(bytes / 1024 / 1024);
+    if(bytes < 1000 * 1000)
+        return QObject::tr("%1 kB").arg(bytes / 1000);


@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?

Yes. We could add more translation context later, when bitcoin/bitcoin#21465 approach is implemented.

hebasto

Approach ACK 200b503

@wodry

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

hebasto · 2021-03-19T14:46:04Z

src/qt/trafficgraphwidget.cpp

+    float inRate = (bytesIn - nLastBytesIn) * 1.0f / timer->interval();
+    float outRate = (bytesOut - nLastBytesOut) * 1.0f / timer->interval();


Why * 1.0f is required?

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

If type conversion is supposed, it would better to apply explicit conversion.

hebasto · 2021-03-19T14:51:11Z

src/qt/trafficgraphwidget.cpp

@@ -128,8 +128,9 @@ void TrafficGraphWidget::updateRates()

    quint64 bytesIn = clientModel->node().getTotalBytesRecv(),
            bytesOut = clientModel->node().getTotalBytesSent();
-    float inRate = (bytesIn - nLastBytesIn) / 1024.0f * 1000 / timer->interval();
-    float outRate = (bytesOut - nLastBytesOut) / 1024.0f * 1000 / timer->interval();
+    // Rates are in bytes/millisecond which is equal to kilobytes/second


Instead of adding a comment, is it better just choose proper names for variables?
E.g., in_rate_kilobytes_per_sec ?

ghost · 2021-03-19T18:08:02Z

Approach ACK 200b503

@wodry

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

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

hebasto · 2021-03-19T18:26:35Z

May I suggest a change:

--- a/src/qt/guiutil.cpp
+++ b/src/qt/guiutil.cpp
@@ -759,14 +759,14 @@ QString formatNiceTimeOffset(qint64 secs)
 
 QString formatBytes(uint64_t bytes)
 {
-    if (bytes < 1000)
+    if (bytes < 1'000)
         return QObject::tr("%1 B").arg(bytes);
-    if (bytes < 1000 * 1000)
-        return QObject::tr("%1 kB").arg(bytes / 1000);
-    if (bytes < 1000 * 1000 * 1000)
-        return QObject::tr("%1 MB").arg(bytes / 1000 / 1000);
+    if (bytes < 1'000'000)
+        return QObject::tr("%1 kB").arg(bytes / 1'000);
+    if (bytes < 1'000'000'000)
+        return QObject::tr("%1 MB").arg(bytes / 1'000'000);
 
-    return QObject::tr("%1 GB").arg(bytes / 1000 / 1000 / 1000);
+    return QObject::tr("%1 GB").arg(bytes / 1'000'000'000);
 }
 
 qreal calculateIdealFontSize(int width, const QString& text, QFont font, qreal minPointSize, qreal font_size) {
diff --git a/src/qt/trafficgraphwidget.cpp b/src/qt/trafficgraphwidget.cpp
index 9e0b67917..7e12410c8 100644
--- a/src/qt/trafficgraphwidget.cpp
+++ b/src/qt/trafficgraphwidget.cpp
@@ -128,8 +128,8 @@ void TrafficGraphWidget::updateRates()
 
     quint64 bytesIn = clientModel->node().getTotalBytesRecv(),
             bytesOut = clientModel->node().getTotalBytesSent();
-    float in_rate_kilobytes_per_sec = (bytesIn - nLastBytesIn) / timer->interval();
-    float out_rate_kilobytes_per_sec = (bytesOut - nLastBytesOut) / timer->interval();
+    float in_rate_kilobytes_per_sec = static_cast<float>(bytesIn - nLastBytesIn) / timer->interval();
+    float out_rate_kilobytes_per_sec = static_cast<float>(bytesOut - nLastBytesOut) / timer->interval();
     vSamplesIn.push_front(in_rate_kilobytes_per_sec);
     vSamplesOut.push_front(out_rate_kilobytes_per_sec);
     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).

ghost · 2021-03-19T19:07:23Z

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)

hebasto

ACK d09ebc4, tested on Linux Mint 20.1 (Qt 5.12.8) the both "Network Traffic" and "Peers" tabs of the "Node Window".

jarolrod

ACK d09ebc4

Tested on macOS 11.1 Qt 5.15.2

hebasto · 2021-03-22T13:49:12Z

@jonatack Do you mind looking into this PR?

leonardojobim

Tested ACK d09ebc4 on Ubuntu 20.04 Qt 5.12.8

hebasto · 2021-03-23T12:59:41Z

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

jnewbery · 2021-03-23T14:26:52Z

Concept ACK. Using powers of 10 and correctly calling them kB and MB seems reasonable for a user-facing GUI.

…d "Bytes/s" with 1000-based prefix names use 1000-based divisor instead of 1024-based d09ebc4 Fix wrong(1024) divisor for 1000-based prefixes (wodry) Pull request description: 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. ACKs for top commit: hebasto: ACK d09ebc4, tested on Linux Mint 20.1 (Qt 5.12.8) the both "Network Traffic" and "Peers" tabs of the "Node Window". jarolrod: ACK d09ebc4 leonardojobim: Tested ACK bitcoin-core/gui@d09ebc4 on Ubuntu 20.04 Qt 5.12.8 Tree-SHA512: 8f830b08cc3fd36dc8a18f1192959fe55d1644938044bf31d770f7c3bf8475fba6da5019a2d2024d5b2c81a8dab112f360c555367814a14f4d05c89d130f25b0

hebasto added the Feature label Mar 13, 2021

jarolrod reviewed Mar 15, 2021

View reviewed changes

DrahtBot added Needs rebase and removed Needs rebase labels Mar 15, 2021

ghost changed the title ~~gui: Display binary prefixes for power-of-2 byte values~~ Fix incorrect prefix name for values of "Bytes transferred" and "Bytes/s" Mar 15, 2021

jarolrod reviewed Mar 18, 2021

View reviewed changes

hebasto reviewed Mar 19, 2021

View reviewed changes

Fix wrong(1024) divisor for 1000-based prefixes

d09ebc4

hebasto approved these changes Mar 19, 2021

View reviewed changes

ghost changed the title ~~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 Mar 19, 2021

ghost changed the title ~~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 Mar 19, 2021

jarolrod reviewed Mar 22, 2021

View reviewed changes

leonardojobim reviewed Mar 22, 2021

View reviewed changes

maflcko merged commit 837e59e into bitcoin-core:master Mar 23, 2021

ghost deleted the binary-prefix branch March 28, 2021 20:39

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge 837e59e into merged_master (Bitcoin PR bitcoin-core/gui#248)

b4c1a62

bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ghost commented Mar 13, 2021 •

edited by ghost

Loading

ghost commented Mar 13, 2021 •

edited by ghost

Loading

jarolrod Mar 15, 2021

ghost Mar 15, 2021 •

edited by ghost

Loading

jarolrod Mar 15, 2021

ghost Mar 15, 2021

jarolrod left a comment

hebasto commented Mar 15, 2021

ghost commented Mar 16, 2021

hebasto commented Mar 16, 2021

sipa commented Mar 18, 2021

ghost commented Mar 18, 2021

ghost commented Mar 18, 2021

ghost commented Mar 18, 2021

jarolrod Mar 18, 2021 •

edited

Loading

hebasto Mar 19, 2021

hebasto left a comment

hebasto Mar 19, 2021

ghost Mar 19, 2021

hebasto Mar 19, 2021

hebasto Mar 19, 2021

ghost commented Mar 19, 2021

hebasto commented Mar 19, 2021 •

edited

Loading

ghost commented Mar 19, 2021 •

edited by ghost

Loading

hebasto left a comment

jarolrod left a comment

hebasto commented Mar 22, 2021

leonardojobim left a comment

hebasto commented Mar 23, 2021

jnewbery commented Mar 23, 2021

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

		float inRate = (bytesIn - nLastBytesIn) * 1.0f / timer->interval();
		float outRate = (bytesOut - nLastBytesOut) * 1.0f / timer->interval();

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

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

Conversation

ghost commented Mar 13, 2021 • edited by ghost Loading

ghost commented Mar 13, 2021 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Mar 15, 2021 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jarolrod left a comment

Choose a reason for hiding this comment

hebasto commented Mar 15, 2021

ghost commented Mar 16, 2021

hebasto commented Mar 16, 2021

sipa commented Mar 18, 2021

ghost commented Mar 18, 2021

ghost commented Mar 18, 2021

ghost commented Mar 18, 2021

jarolrod Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hebasto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Mar 19, 2021

hebasto commented Mar 19, 2021 • edited Loading

ghost commented Mar 19, 2021 • edited by ghost Loading

hebasto left a comment

Choose a reason for hiding this comment

jarolrod left a comment

Choose a reason for hiding this comment

hebasto commented Mar 22, 2021

leonardojobim left a comment

Choose a reason for hiding this comment

hebasto commented Mar 23, 2021

jnewbery commented Mar 23, 2021

ghost commented Mar 13, 2021 •

edited by ghost

Loading

ghost commented Mar 13, 2021 •

edited by ghost

Loading

ghost Mar 15, 2021 •

edited by ghost

Loading

jarolrod Mar 18, 2021 •

edited

Loading

hebasto commented Mar 19, 2021 •

edited

Loading

ghost commented Mar 19, 2021 •

edited by ghost

Loading