Display speeds in bits per second instead of KiB/s
ClosedPublic

Authored by vishalrao on Feb 10 2017, 1:56 PM.

Details

Summary

This is a patch to make the Network Monitor desktop plasmoid widget display the upload and download speeds in bits per second (Mbps, Kbps or bps as the case may be) rather than the fixed "KiB/s" unit.

It appears the input data is always in KiB units so the bps part may never be called but left it in there anyway.

Diff Detail

Repository
R120 Plasma Workspace
Lint
Lint Skipped
Unit
Unit Tests Skipped
vishalrao updated this revision to Diff 11176.Feb 10 2017, 1:56 PM
vishalrao retitled this revision from to Display speeds in bits per second instead of KiB/s.
vishalrao updated this object.
vishalrao edited the test plan for this revision. (Show Details)
vishalrao set the repository for this revision to R120 Plasma Workspace.
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 10 2017, 1:56 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript

In general fine.

applets/systemmonitor/net/contents/ui/net.qml
42–61

formatBitSpeed ?

Otherwise it's confusing

44

the c in i18nc means "context"

so it's used as a description of what the message is for for translators so they know what the string is for, as they can't see the code or where it's used.

Repeating the text doesn't help, we want something like "Transfer speed shown on a graph label "
This is needed so that the german translation doesn't write an entire 200 character long sentence.

52

A context string of "zero" is even more confusing, maybe just return value here.

Zren added a subscriber: Zren.Feb 10 2017, 2:20 PM

Any reason for not using

if (...) {
} else if (...) {
} else {
}

In D4551#85039, @Zren wrote:

Any reason for not using

if (...) {
} else if (...) {
} else {
}

No real reason, the statements are single lines and returns so thought this is okay - should I change to use "else if"?

vishalrao updated this revision to Diff 11178.Feb 10 2017, 2:27 PM

Updated per David's comments.

davidedmundson accepted this revision.Feb 10 2017, 2:30 PM
davidedmundson added a reviewer: davidedmundson.
This revision is now accepted and ready to land.Feb 10 2017, 2:30 PM
sebas added a subscriber: sebas.Feb 10 2017, 2:42 PM
sebas added inline comments.
applets/systemmonitor/net/contents/ui/net.qml
43

Perhaps explain this "magic number" in a comment? (The more obvious this is, the easier it is to revisit in the future)

44

nitpick (to expand on David's explanation), the comment is now redundant (the string already gives the unit). The translators need context, often this means telling them where this string is displayed, so they can make a call what the actual translation would be, and test and verify the correctness. (Not super critical, but since we're talking i18nc semantics anyway, I thought it'd be useful to mention.)

vishalrao updated this revision to Diff 11181.Feb 10 2017, 3:33 PM
vishalrao edited edge metadata.
vishalrao marked 3 inline comments as done.

Update per sebas' comments.

Instead of a comment for the magic number, used a perhaps more recognisable (1024 * 1024) value.

Added the word "displayed" to the i18nc context info.

Let me know if this is okay or anything to further tweak?

vishalrao marked 2 inline comments as done.Feb 10 2017, 3:40 PM
broulik added a subscriber: broulik.EditedFeb 10 2017, 4:16 PM

-1

Can we please stay at *Bytes* per second instead of *Bits*. We don't use that anywhere else, copying files also uses Bytes instead of Bits.

Also, what's up with the 1024 everywhere, *Kilo* is clearly 1000.

vishalrao added a comment.EditedFeb 10 2017, 4:21 PM

-1

Can we please stay at *Bytes* per second instead of *Bits*. We don't use that anywhere else, copying files also uses Bytes instead of Bits.

Also, what's up with the 1024 everywhere, *Kilo* is clearly 1000.

Other places like disk monitor and file copy seem okay to me with bytes. I proposed bits per second for network monitor only since users internet speeds are advertised/shown as "Mbps" or "Kbps" or the like.

For the 1024 or "kilo" - should I change the i18nc wording to say "mebibytes per second" and "kibibytes" in the context info?

Zren added a comment.Feb 10 2017, 4:58 PM

Ah, just noticed the title (bytes => bits), and I agree it should remain bytes, or at least be optional.

I proposed bits per second for network monitor only since users internet speeds are advertised/shown as "Mbps" or "Kbps" or the like.

Do the math (divide by 8) once when you subscribe. Otherwise you'll need to do the mental arithmetic every time you use the graph. Downloads in your browser are calculated in bytes. File storage and transfers are done in bytes. Bits is only used by ISP sellers because it's bigger and is thus marketed that way.

This revision was automatically updated to reflect the committed changes.