Refactor converter runner
ClosedPublic

Authored by alex on Feb 5 2020, 10:56 AM.

Details

Summary

The logic from the converter runner has been refactored and moved to seperate files.
Additionally some foreach marcos have been refactored and the numberValue of the query gets only
calculated once, instead of being calculated for each matching unit.

Test Plan

The plugin should work as before and the fractional units in https://phabricator.kde.org/D22869 are still supported.

Diff Detail

Repository
R114 Plasma Addons
Branch
arcpatch-D27166_2
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 24102
Build 24120: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
alex added a reviewer: sitter.Feb 5 2020, 6:09 PM
alex marked an inline comment as done.

The regex has been implemented and the code is now ~100 lines shorter but provides the same functionality
and the few remaining utility functions have been moved to the runner class.
This should also fix the issues you mentioned :-) .
PS: The regex now includes the of the converter supported currency symbols.

Looks so much better already!

I'd really love a unit test for this though. plasma-workspace's servicesrunner should have an example one

runners/converter/converterrunner.cpp
19

Please move this after the includes. If any header in the future decides to want to use the name as well we're in for an awkward debugging session.

161

I am sure we can do better than hardcoding currency symbols. Perhaps iterate QLocales and get the currencySymbol for each?
(needs checking that performance doesn't degrade)

163

I'd really prefer named capture groups. Popping data out of the captures by index is fairly terrible to read and also easier to mess up in the future.

171–172

unnecessary ref.

177

Please use more descriptive names than u1, such as unit1.
In fact, isn't unit1 "inputUnit" and unit2 "outputUnit"? If so I'd name them thusly.

178

unnecessary ref.

180

s/u/unit

181

unit1 and unit2 should probably be unitString1 and unitString2 so the actual units can be unit1 and unit2. same for value I suppose.

181

s/v/value

184

in all return statements you can use list initialization

return { ok, output };

185

newline missing it seems

runners/converter/converterrunner.h
53

stringToDouble and getValidatedNumberValue seem to have conflicting ideas of api design. either both should use the pair return or both should use the ok pointer.

55

Please use multiple lines to declare multiple members, even when they have the same type. Same line declaration is unnecessarily increasing reading complexity.

alex updated this revision to Diff 75359.Feb 10 2020, 1:02 PM
  • Start implementing requested changes
  • Remove unnecessary class declaration
alex marked 11 inline comments as done.EditedFeb 10 2020, 1:24 PM

Hello,
I have found no way to get the currency symbols, other that hard coded.
If you have a look at https://phabricator.kde.org/source/kunitconversion/browse/master/src/currency.cpp$67 you can see that the symbols are concatenated to the string.
Because of this I don't know how to get just the symbols.

I found currently implemented ones by executing:

qInfo() << inputCategory.allUnits();

and then manually copying the symbols.

And if you try to get the symbols using

for(const auto &s: inputCategory.allUnits()){
        qInfo() << inputCategory.unit(s).symbol();
    }

You get results like:
"PLN"
"GBP"
"JPY"
"EUR"

And more symbols (than the already hard coded ones) are currently not supported by the KUnitConversion library.

alex updated this revision to Diff 76324.Feb 24 2020, 10:56 PM

Add support for other currency symbols, use named groups for regex

Now the plugin can evaluate queries like "4$>¥.

alex added a comment.Feb 24 2020, 11:00 PM

Yes, I had a look, but I thought there may be a way to solve this using the KDE conversion API.
The new changes now support 12 different currency symbols instead of just 3.

12? Why 12? Or I guess the more pertinent question: where is that limit set?

runners/converter/converterrunner.cpp
195

left over debug

alex updated this revision to Diff 76347.Feb 25 2020, 10:07 AM

Use character types for currency symbols in regex

Yeah, I don't think I understand. I am sure a unit test would make things clearer ;) ;)

alex added a comment.EditedFeb 25 2020, 10:30 AM

Yeah, I don't think I understand. I am sure a unit test would make things clearer ;) ;)

Yes, I will add one :-).
And in the last commit I just used a regex feature which matches all currency symbols, this makes it a lot easier.

And a quick question: Should the converter be able to convert values like US$, If yes I would try to implement this.

alex updated this revision to Diff 76487.Feb 26 2020, 6:12 PM

Support combination of currency symbols and letters

Now queries like 4us$>ca$ are supported.
Every currency from the QLocale API which has a currency symbol and support in the unit conversion backend can be used in this runner.

alex marked 3 inline comments as done.Feb 26 2020, 6:14 PM

Neat. I am Mostly just waiting for a test at this point.

alex updated this revision to Diff 77899.Mar 18 2020, 11:38 AM

Add tests for converterrunner

sitter requested changes to this revision.Mar 19 2020, 1:18 PM
sitter added inline comments.
runners/converter/CMakeLists.txt
24

No newline at end of file

runners/converter/autotests/CMakeLists.txt
6

No newline at end of file

runners/converter/autotests/converterrunnertest.cpp
43

= nullptr

In fact, you could make this a std::unique_ptr because currently it is also not getting deleted, not that it matters for a test ^^

215

No newline at end of file

runners/converter/converterrunner.cpp
160

Seeing as this now doesn't do anything you can = default it.

This revision now requires changes to proceed.Mar 19 2020, 1:18 PM
alex updated this revision to Diff 78007.Mar 19 2020, 2:36 PM

Implement requested changes

alex marked 5 inline comments as done.Mar 19 2020, 2:40 PM
sitter accepted this revision.Mar 19 2020, 2:45 PM
This revision is now accepted and ready to land.Mar 19 2020, 2:45 PM
broulik added a comment.EditedMar 19 2020, 3:01 PM

Nice refactoring work! Quick testing suggests a few issues, however. I haven't verified if they have been present before or are in KUnitConversion, but:

"5 EUR" produces: 4.6095 GBP, 5.467 USD, 7.8745 CAD with too many decimals (@sitter says this is some unrelated bug)
"5 l/100km" (fuel consumption) doesn't yield any result (@sitter says this didn't work before either)
"5 m²" doesn't yield any results. In fact none of the ² or ³ do
Units seem to be matching case sensitive now, i.e. "5 Liter" (German locale) doesn't yield any results, wheras "5 liter" does

runners/converter/autotests/converterrunnertest.cpp
76

I think you should change the locale for the unittest to C or en_US (dunno how, needs someone with more test knowledge :D) since those are translated and fail when I run the test locally

sitter requested changes to this revision.Mar 19 2020, 3:05 PM

Please also add test cases for the regressions Kai highlighted. Thanks!

runners/converter/autotests/converterrunnertest.cpp
76

the servicerunnertest in plasma-workspace is actually doing that by simply calling setlocale I think. that should be good enough so long as it gets called early enough

This revision now requires changes to proceed.Mar 19 2020, 3:05 PM
alex updated this revision to Diff 78016.Mar 19 2020, 3:20 PM

Fix regex, round currencies to two decimal counts

alex added a comment.Mar 19 2020, 3:27 PM

"5 EUR" produces: 4.6095 GBP, 5.467 USD, 7.8745 CAD with too many decimals (@sitter says this is some unrelated bug)
"5 m²" doesn't yield any results. In fact none of the ² or ³ do

Fixed in the latest commit.

Units seem to be matching case sensitive now, i.e. "5 Liter" (German locale) doesn't yield any results, wheras "5 liter" does

I will have a look at this too and update the test 😃

broulik added inline comments.Mar 19 2020, 3:28 PM
runners/converter/converterrunner.cpp
105

There's also units with µ, Ω, . (fl.oz.), not taking into account how those might be localized, so using a very specific regexp looks somewhat brittle to me. Can't really think of a better way, though...

134

I thought KUnitConversion was doing that when requesting a pretty string

alex added inline comments.Mar 19 2020, 3:53 PM
runners/converter/converterrunner.cpp
105

Well the only idea I have is to allow everything as a unit and let the conversion backend decide if it is valid.

alex updated this revision to Diff 78098.Mar 20 2020, 2:37 PM

Rewrite query parsing, fix case sensitive units, add tests for these issues

Very nice! There's two minor nitpicks but then I think it should be good to go, thanks for your endurance :)

runners/converter/autotests/converterrunnertest.cpp
51

This doesn't affect translations. I guess you also want to add a

qputenv("LANG", "en_US")
runners/converter/converterrunner.cpp
126

When there's just one decimal, it doesn't add a zero

alex updated this revision to Diff 78100.Mar 20 2020, 3:00 PM

Always display two decimals in currency conversion, set ENV variable in test

broulik accepted this revision.Mar 20 2020, 3:44 PM

Ship it!

alex marked 7 inline comments as done.Mar 20 2020, 3:54 PM

I don't have developer access, but I applied for it yesterday :-).

I just noticed another minor issue: when I activate a result with the old one it would copy the value into the text field, i.e. I type "5 EUR", select the 4.55 GBP result and then it would copy 4.55 into the text field. This is done for InformationalMatch types, which you don't seem to be using anymore.

alex added a comment.Mar 20 2020, 6:23 PM

If the match type gets set to InformationalMatch the run method will never be called (that is how it was before and for me it makes more sense this way).

What about setting the type to InformationalMatch and adding a copy action? This way the old behavior is the default but the user can easily copy the value.
And maybe adding another action which copies the value and unit to the clipboard?

Sounds good! :)

alex updated this revision to Diff 78121.Mar 20 2020, 7:06 PM

Implement actions for copying results

Also the icon for the matches has been changed: In the .desktop file it is accessories-calculator, but in for the matches edit-copy was used. And displaying the same icon three times in a match looks not very good^^

sitter accepted this revision.Mar 23 2020, 8:34 AM
This revision is now accepted and ready to land.Mar 23 2020, 8:34 AM
alex updated this revision to Diff 78281.Mar 23 2020, 11:44 AM

Allow +/- before values, add testcase

Small detail I forgot to add and a testcase for this.

This revision was automatically updated to reflect the committed changes.

I'm seeing another issue: everything with a slash is now interpreted as unit of volume, even just calculations: