D13583: KFormat: Allow usage of quantities beyond bytes and seconds

[Friedrich W. H. Kossebau]

kossebau added a comment.


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

INLINE COMMENTS

> kformat.h:117
> +     * @see formatValue
> +     */
> +    enum class Unit {

Please add missing "@since 5.48"
(as last item, following https://community.kde.org/Frameworks/Frameworks_Documentation_Policy#Document_Public_and_Protected_Members)

> kformat.h:134
> +     * @see formatValue
> +     */
> +    enum class UnitPrefix {

@since 5.48

> kformat.h:339
> +     * Example:
> +     * format.formatValue(1000, KFormat::Unit::Bit, 1, KFormat::UnitPrefix::Kilo) => "1.0 kbit"
> +     *

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

> kformat.h:356
> +     * @see BinaryUnitDialect
> +     */
> +    QString formatValue(double value,

@since 5.48

> kformat.h:367-369
> +     * format.formatValue(1000, QStringLiteral("bit"), 1, KFormat::UnitPrefix::Kilo) => "1.0 kbit"
> +     * format.formatValue(1000, QStringLiteral("bit/s") => "1.0 kbit/s"
> +     * format.formatValue(12.3e6, QStringLiteral("bit/s") => "12.3 Mbit/s"

same code/endcode treatment please

> kformat.h:380
> +     * @see UnitPrefix
> +     */
> +    QString formatValue(double value,

@since 5.48

> kformat.h:382
> +    QString formatValue(double value,
> +                        QString unit,
> +                        int precision = 1,

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

> kformatprivate.cpp:138-145
> +        case KFormat::Unit::Bit:
> +            unitString = QStringLiteral("bit");
> +            break;
> +        case KFormat::Unit::Byte:
> +            unitString = QStringLiteral("B");
> +            break;
> +        case KFormat::Unit::Meter:

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

> kformatprivate.cpp:154
> +        //: value without prefix, format "<val> <unit>"
> +        return tr("%1 %2", "no Prefix").arg(numString).arg(unitString);
> +    }

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

> kformatprivate.cpp:172
> +    //: value with prefix, format "<val> <prefix><unit>"
> +    return tr("%1 %2%3", "MetricBinaryDialect").arg(numString).arg(prefixString).arg(unitString);
> +}

arg(prefixString, unitString)

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D13583

To: bruns, #frameworks
Cc: kossebau, kde-frameworks-devel, astippich, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20180619/b0437f82/attachment-0001.html>