Add Binary Data units (bits, kilobytes, kibibytes ... yottabytes)
ClosedPublic

Authored by JJRcop on Apr 1 2019, 6:08 PM.

Details

Summary

New data category BinaryDataCategory featuring bits and bytes, both SI units and powers of two, from normal bits all the way up to Yibibytes.

This only features ( positive SI prefix | IEC power of two prefix ) + ( bits | bytes ), and does not include units like nibbles (half a byte), octets (alternative form of byte), or prefixes that are less than 1. My reasoning behind this is to keep it clean and uncluttered.

FEATURE: 402798
FIXED-IN: 5.61

Test Plan

Open KRunner and attempt to use new units.
New units show up successfully.

Diff Detail

Repository
R292 KUnitConversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
JJRcop created this revision.Apr 1 2019, 6:08 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptApr 1 2019, 6:08 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
JJRcop requested review of this revision.Apr 1 2019, 6:08 PM
JJRcop retitled this revision from Adds Binary Data units (bits, kilobytes, kibibytes ... yottabytes) to Add Binary Data units (bits, kilobytes, kibibytes ... yottabytes).Apr 1 2019, 6:26 PM
ngraham added a subscriber: ngraham.

Thanks very much for this, it looks great! This will land in 5.58 not 5.57 since we don't typically commit patches with string changes in the last two weeks before frameworks tagging (which happens in five days, see https://community.kde.org/Schedules/Frameworks). So please change all the @since tags. Thanks!

JJRcop updated this revision to Diff 55227.Apr 1 2019, 7:46 PM

Update @since from 5.57 to 5.58

ngraham accepted this revision.Apr 1 2019, 7:58 PM

Thanks, this looks good to me and works great! @broulik?

This revision is now accepted and ready to land.Apr 1 2019, 7:58 PM
JJRcop added a comment.EditedApr 1 2019, 9:38 PM

I think that's out of scope for this patch. It should be done in another patch so we can refactor Frequency and Length to use KFormat as well.

apol added a subscriber: apol.Apr 1 2019, 10:53 PM

I think that's out of scope for this patch. It should be done in another patch so we can refactor Frequency and Length to use KFormat as well.

This can't be out of scope since as soon as you add it here we'll have to maintain API, so it can't actually be done as an iteration.

Aleix

cfeck added a subscriber: cfeck.Apr 25 2019, 9:41 AM

KFormat knows about the prefixes, but doesn't know their name. I would say adding translations for "megabytes" etc. to KCoreAddons is out of scope.

So can someone clarify what's requested for @JJRcop now?

JJRcop edited the test plan for this revision. (Show Details)Apr 25 2019, 3:53 PM
JJRcop set the repository for this revision to R292 KUnitConversion.
JJRcop updated this revision to Diff 59308.Jun 7 2019, 2:11 AM

Updated version number in comments to 5.59 since 5.58 came out earlier in May.

@cfeck @apol @aacid It's not clear to me what's being requested. Can someone clarify?

aacid added a comment.Jun 7 2019, 5:17 PM

@ngraham what is being requested is using existing code that does this instead of reinveinting the wheel.

@aacid Are you asking me to use the KFormat functions for the KUnitConversion category I've created, or do you not think this should be supported by KUnitConversion at all?

aacid added a comment.Jun 7 2019, 10:29 PM

A little of both?

First i ideally wouldn't want translators to have to translate this again when they're already doing it for KFormat, so using KFormat here if possible would be nice.

Second, what's the expected use case of this? I don't see anyone needed a function that converts from kibibit to kilobye. Please convince me it's useful :)

Does this enable the runner translation without any other code?

JJRcop added a comment.EditedJun 8 2019, 12:22 AM

It does enable the runner conversion without any other code, because of the converter addon from R114 Plasma Addons. The screenshots of KRunner seen in the report were generated after building KUnitConversion with this patch on KDE neon dev with no other modifications, not even to KRunner.

I mentioned earlier how Frequency and Length, which are both also covered in KFormat, have their own duplicate implementations in KUnitConversion like the current patch is, and don't currently use KFormat.

I've included all the uncommon unit types to make this complete, but have not added them as CommonUnits.

aacid added a comment.Jun 9 2019, 4:35 PM

Yes i know you've mentioned it earlier.

In my opinion, that someone is doing things wrong is not a reason to not try to aim and do things better.

JJRcop updated this revision to Diff 61526.Jul 10 2019, 4:50 PM

Update version number to 5.60

After some quick research I should have done a while ago, I have determined that KFormat is not nearly the same as what KUnitConversion does.

KFormat can take an input number and output a string with the input formatted properly to the desired unit.

KUnitConversion defines objects for various units with functions to convert between them. You can instantiate these objects using a number and unit type. There are a few objects, some defining the Unit itself, and some defining an instance of a Value in a certain Unit. Both of these objects have methods to convert an input into another Unit, which return either an output number or Value object in the new Unit.

I am not able to use KFormat because KFormat takes an input number and only outputs a formatted string. This is exactly what it's designed for and doesn't need any changes, but is just not compatible with what KUnitConversion does as it handles numbers.

https://api.kde.org/frameworks/kunitconversion/html/index.html
https://api.kde.org/frameworks/kcoreaddons/html/classKFormat.html

I believe no further changes are required for this patch.

Ok, you will need to update your @since when landing this

JJRcop updated this revision to Diff 61883.Jul 16 2019, 11:22 PM

Updated @since to 5.61. @ngraham

ngraham accepted this revision.Jul 17 2019, 2:02 AM
ngraham edited the summary of this revision. (Show Details)

Thanks for your patience here!

This revision was automatically updated to reflect the committed changes.

Very nice first patch, too. May it be the first of many! :)