KFormat: Allow usage of quantities beyond bytes and seconds
ClosedPublic

Authored by bruns on Jun 17 2018, 5:51 PM.

Details

Summary

Its useful to allow automatic formatting of quantities like bit per
second (bit/s), but formatByteSize can not be used for this. Generalize
the implementation to allow usage for arbitrary quantities.

Test Plan

make test

Diff Detail

Repository
R244 KCoreAddons
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 80
Build 80: arc lint + arc unit
bruns created this revision.Jun 17 2018, 5:51 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJun 17 2018, 5:51 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
bruns requested review of this revision.Jun 17 2018, 5:51 PM

Some quick feedback, though no in-detail review myself for now, not sure I will be able later, so needs others as well.

src/lib/util/kformat.h
117
134

@since 5.48

339

Please wrap example code with tags for nice highlighting, and for that adapt also string to be real code, e.g. like this:

* @code
* // sets value to "1.0 kbit"
* auto value = format.formatValue(1000, KFormat::Unit::Bit, 1, KFormat::UnitPrefix::Kilo);
* @endcode
356

@since 5.48

367–369

same code/endcode treatment please

380

@since 5.48

382

pass by const reference -> const QString& unit

That the unitstring argument is modified internally in the private method is a current implementation detail which should not leak into the public API

src/lib/util/kformatprivate.cpp
138–145

Are we sure those unit symbols do not need to be localized? What about languages using different scripts (cyrillic, chinese, arabic, etc)?

154

Please use multi-argument arg method with string arguments: arg(numString).arg(unitString) -> arg(prefixString, unitString)

172

arg(prefixString, unitString)

Unfortunately it does not apply here because the base commit cannot be found. Also, may I already add an request? It would be great if the unit Hertz is included since it is used in baloo-widgets

bruns marked 10 inline comments as done.Jun 27 2018, 12:16 AM
bruns added inline comments.
src/lib/util/kformatprivate.cpp
138–145

Yes. As you said, these are symbols. For the names there may be transliterations, the symbols stay the same, to avoid ambiguities. This also matches my experience with datasheets, the text may be chinese, but measurements are given in SI units.

bruns updated this revision to Diff 36735.Jun 27 2018, 12:21 AM
bruns marked an inline comment as done.

added @since 5.48 tags
use @code/@endcode
add KFormat::Unit::Hertz
remaining comments

astippich accepted this revision.EditedJun 29 2018, 7:33 PM

working fine here, but someone more qualified than myself should have a look at the code again

This revision is now accepted and ready to land.Jun 29 2018, 7:33 PM
This revision was automatically updated to reflect the committed changes.

@bruns Please update all the "@since" to 5.49, as this will be only part of the next KF version now.

bruns added a comment.Jul 14 2018, 2:25 PM

@bruns Please update all the "@since" to 5.49, as this will be only part of the next KF version now.

Sorry, updated and pushed

bruns added a comment.Jul 14 2018, 9:05 PM

MSVC seems to be broken, it chokes on
QChar(u'µ')

I don't have access to MSVC, so I can't do anything about it.

Unfortunately we cannot leave it in a broken condition. Could we try encoding it differently perhaps?

Is there some tag which can be used to pull the people in who want to support that Closed Source Microsoft platform in such cases?

I wasted some time of my FLOSS life meanwhile and found https://developercommunity.visualstudio.com/content/problem/198547/error-c2015-for-fixed-encoding-character-literals.html and https://stackoverflow.com/questions/5060560/how-to-print-the-microsecond-symbol-in-c which might be interesting. But at the end of the day it needs to be the Windows people to work on fixing this.

You can add Windows to a review, however there still needs to be a degree of responsibility by those who are actually making commits to projects (especially in the case of Frameworks).

Otherwise we'll end up in a situation where developers who do care about these platforms for their applications are continuously having to fix breakages created by others who don't care at all, which is an unsustainable dynamic.
This would have substantial negative impacts on projects such as Krita, KDevelop, RKWard, Elisa, Digikam, Marble, Peruse, Falkon and Kate among others.

The fix in this case is likely relatively trivial, so it should be done by the original developer.

This outputs 'µ' twice, so QChar(0xB5) should be equivalent:

qWarning() << QString(QChar(0xB5)) << QString(QChar(u'µ'));

You can add Windows to a review, however there still needs to be a degree of responsibility by those who are actually making commits to projects (especially in the case of Frameworks).

Sure, breaking things on purpose or by complete ignorance is not what I mean here. I am searching for responsibility of those who want to see certain platforms supported, to have known ways to approach them in case of such emergencies, where progress is inhibited due to platform-specific issues (and especially when it comes to ClosedSource/questionably licensed platforms which undermine the whole idea oif KDE software).

Or even see those stakeholders proactively care for their platform. See e.g. all the KF5 unit tests which only fail on Windows. That does not help those who work on enhancing KF5 for FLOSS platforms, they cannot be sure if it was their work which fails on that other platform. It also sends a signal of dont-care to their fellows :(

And app developers using KF5 on those platforms need to take responsibility, too. Saying "oh, In version x.y.z things worked for me on all platforms, now don't dare to touch it, as I only care for my app and not somebody else needs" does not work for shared libraries. They are shared between all the app developers, not given to them as free present. We are collaborating here on the KF5 libraries. Delegating platform maintainership on your active fellows is surely not what we want, right?

The fix in this case is likely relatively trivial, so it should be done by the original developer.

Even if trivial, it needs persons not interested in a platform (and here even a platform from the dark side of the software universe) do work for those who are interested in it. Which also means, that very issue which caused the need for a workaround is not reported upstream by the stakeholders or tracked in any way by them. We do not know e.g. which versions of the compilers are affected, so when this workaround can be removed again to clean the code.

Thankfully your work on the staging builds for review requests might improve the situation., catching such breakage before its enters the repos. But it still needs some organized way to have stakeholders of all platforms involved, taking up their duties of maintainership of the platforms. If things should be supported on Android, Windows, etc, there need to be well known point of contacts.

Hi. This commit seems to have introduced a build failure when the optional component PythonModuleGeneration is enabled: https://paste.kde.org/pvh0kutbq/5jo1sl

On a related note, the CI does not have that component enables, which means it is not getting tested. Perhaps the optional components should also be enabled?

bruns added a comment.Jul 15 2018, 3:22 PM

Hi. This commit seems to have introduced a build failure when the optional component PythonModuleGeneration is enabled: https://paste.kde.org/pvh0kutbq/5jo1sl

Apparently it does not handle strongly typed enums - it has to be e.g. KFormat::UnitPrefix::AutoAdjust

In regards to Python, that is not enabled as the bindings generation is not reliable and can easily fail on a highly parallel system (which the CI is, depending on the node it could be a Hexa Core with HT, so run using make -j13)
The generation also had a nasty tendency to fail for certain Frameworks more often than not due to various generation failures which would disappear on subsequent runs.

In terms of various platforms, subscribing the appropriate Phabricator group when issues turn up is probably the best thing to do, we have Windows, FreeBSD and Android for this. We don't have anything for macOS at the moment although interest in that platform is very minimal.

In terms of people who support those platforms, tests haven't been addressed as there are only a few people who work on keeping quite a large amount of software working, which makes getting around to looking at the tests a bit difficult.

I've found that the app developers do care about current versions, the problem arises here as they generally only use released versions rather than master, so any issues they find will be much later down the road when they try to upgrade. The CI does try to keep an eye on this fortunately and pre-review CI should hopefully stop breakages from being fixed after the fact.

Thank you for the clarification @bcooksley. I wonder if there is a way to enforce compilation of certain parts single threaded in make. Or some other way to ensure testing these components.

But in any case, the builds are still failing for me. I hope @bruns or someone else will fix it soon. Thanks.

No problem @siddharthasahu.
For the issue you're having with bindings i'd suggest raising a separate thread about that on kde-frameworks-devel as others might have missed it buried in this review.

Yes. As you said, these are symbols. For the names there may be transliterations, the symbols stay the same, to avoid ambiguities. This also matches my experience with datasheets, the text may be chinese, but measurements are given in SI units.

I just saw¹ this PR and I'd like to say something.
Yes, in scientific papers we use English terms/symbols so that they are understood by others, and to have a system for communication (same goes for IUPAC naming.)
But for users this is wrong, especially with different scripts, and even with right-to-left languages. We are here so that they understand that, not others.
These are the Chinese, Russian, Arabic, Hebrew and Persian Unicode pages showing different units.

Maybe in Chinese you don't use the localized version of the symbols, but I think you use English full-width characters, right? How these will be localized then?
Same case goes for Russian that uses thier cyrillic script.

Arabic, Hebrew and Persian are a different case. It is always considered bad to mix between Arabic and English text, and if so then in extreme hard cases (where you can't translate MySQL or a command-line application that no one really use except for developers, or maybe Qt method names or classes.).
But here we are mixing just few characters with few numbers and things are in a mess. If this was used in QML (where text direction is detected automaticlly), things will go wrong for RTL languages, and you'll see the unit name on the right and the quantity on the left.


Fixed by prepending a RLM to the sentence, as Kate detects text direction automatically as well

Please, if your language uses English symbols, it doesn't mean that every other must follow. KI18n previous (and maybe current, not sure) developers created scripting-enabled translation system so that each language team can do whatever suits thier language.

Thank you.

1: After I saw the new PR using this.

P.S: Check these files in KDE l10n repositories:
https://websvn.kde.org/trunk/l10n-kf5/fr/messages/frameworks/kcoreaddons5_qt.po?view=markup (starting from line 225)
https://websvn.kde.org/trunk/l10n-kf5/ru/messages/frameworks/kcoreaddons5_qt.po?view=markup (starting from line 230)
https://websvn.kde.org/trunk/l10n-kf5/fa/messages/frameworks/kcoreaddons5_qt.po?view=markup (starting from line 325)
https://websvn.kde.org/trunk/l10n-kf5/he/messages/frameworks/kcoreaddons5_qt.po?view=markup (starting from line 247)
https://websvn.kde.org/trunk/l10n-kf5/ar/messages/frameworks/kcoreaddons5_qt.po?view=markup (starting from line 204)