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.
Details
- Reviewers
broulik ngraham sitter - Group Reviewers
Plasma - Commits
- R114:08bb4ec7370c: Refactor converter runner
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
- converter_runner_refactoring (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 23998 Build 24016: arc lint + arc unit
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? | |
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. | |
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. |
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.
Add support for other currency symbols, use named groups for regex
Now the plugin can evaluate queries like "4$>¥.
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 |
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.
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.
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 ^^ | |
197 | No newline at end of file | |
runners/converter/converterrunner.cpp | ||
160 | Seeing as this now doesn't do anything you can = default it. |
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 |
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 |
"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 😃
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 |
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. |
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 |
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.
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?
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^^
Allow +/- before values, add testcase
Small detail I forgot to add and a testcase for this.
I'm seeing another issue: everything with a slash is now interpreted as unit of volume, even just calculations: