Fix Freeze: logarithmic vertical axis and negative data range From value
ClosedPublic

Authored by rszczesiak on May 5 2020, 1:19 PM.

Details

Summary

Added a new validator for data range From and To values that ensures
user be unable to specify data range values lower than the precision
limit (always positive). The fields are re-validated when the user
checks/unchecks the 'Logarithmic vertical axis' option.

BUG: 421056

Diff Detail

Repository
R261 KMyMoney
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
rszczesiak requested review of this revision.May 5 2020, 1:19 PM
rszczesiak created this revision.
rszczesiak edited the summary of this revision. (Show Details)May 5 2020, 2:31 PM
rszczesiak updated this revision to Diff 81998.May 5 2020, 3:27 PM
This comment was removed by rszczesiak.
rszczesiak updated this revision to Diff 82011.May 5 2020, 6:05 PM

Fixed typos in the commit message

rszczesiak added a project: KMyMoney.
rszczesiak updated this revision to Diff 82057.EditedMay 6 2020, 7:40 AM

Simplified c'tor arguments. Removed unnecessary if statements. Added indentation.

tbaumgart requested changes to this revision.May 6 2020, 4:04 PM
tbaumgart added inline comments.
kmymoney/plugins/views/reports/reporttabimpl.cpp
206

Who owns this dblVal object and who destroys it?

215

This remove stuff shows up in several places. Seems to be a good candidate for a utility function.

407

I might not fully understand that RE. Can you explain it? If you use anchoredPattern, why do you add a $ at the end? What is %110 and %12? I must be blind.

Since you add the decimal symbol here, you could also capture the part after the decimal symbol directly in the RE and use it later. Though, a match only occurs if the fraction has between 0 and decimals() number of digits, so it could never match and have more to become invalid. So the check below seems somewhat superfluous. Or what is it that I don't see here?

410

Use

s == QLatin1String("0")  or
s == QStringLiteral("0")

here

kmymoney/plugins/views/reports/reporttabimpl.h
107

Make sure to initialize m_logYaxis in the ctor()

160

Use nullptr instead of 0.

This revision now requires changes to proceed.May 6 2020, 4:04 PM
rszczesiak added inline comments.May 6 2020, 6:35 PM
kmymoney/plugins/views/reports/reporttabimpl.cpp
407

This one is tricky :) I should have commented on this RE.
First, you are right - the $ is redundant with anchoredPattern. I forgot to remove it from the committed diff.
Answering your question about %11 and %12, here is the trick. Those are placeholders replaced at runtime with whatever evaluates out of the QString::arg(...) function invocations that follow in order. If fact, the first placeholder is not %110 but %11. I started numbering from two-digit numer, here 11, not from 1, on purpose. The following 0 in the RE is literal digit that the RE expects after decimal point represented by %11. According to documentation (https://doc.qt.io/qt-5/qstring.html#arg-12), QString::arg(...) interprets placeholder strings %1, %2, ..., %99. Therefore, I had to number placeholders from two-digit numer to avoid interpreting %10 as a whole.
Now let me explain what the RE is intended to do. It says something like: Match any string that starts with 0, then has a localized decimal point, and then optionally a few more zeros but no more than our precision minus one. What for? Because I do not what 0., 0.0, 0.00 and so on to be successfully converted to double and immediately rejected by the result >= bottom() test. If that would happen, the user would be unable to input a value such as 0.01. Thus, strings like 0.000 (assume precision=4) ought to be validated as QValidator::Intermediate, which means incomplete but can easly become QValidator::Acceptable if the the next entered digit is non-zero.
When it comes to capturing groups. Well, I could do that. Instead of matching a sequence of zero after decimal point, I could capture any decimal part and reuse it later but still I would need to do another RE matching to check if the captured decimal part is a sequence of zeros to allow 0.0... as Intermediate. I am not sure if it is worth it. In the end of the day, Regular Expressions are not the easiest to read and understand (as might be proven on our example :) ). And also I doubt REs are more efficient than the classic snippet found in lines 413-419.

tbaumgart added inline comments.May 6 2020, 7:22 PM
kmymoney/plugins/views/reports/reporttabimpl.cpp
407

Thank you for the explanation. Completely valid. Agreed, a comment would help. I did not know about the trick to start with a two digit placeholder index.

One problem though: QRegularExpression::anchoredText only appeared in Qt 5.12 and we still support older versions (e.g. 5.9), so we should use ^ and $ as part of the RE.

rszczesiak updated this revision to Diff 82249.May 8 2020, 10:07 AM

Added the requested changes.

rszczesiak updated this revision to Diff 82250.May 8 2020, 10:10 AM

Removed unnecessary function declaration.

This revision was not accepted when it landed; it landed in state Needs Review.May 8 2020, 11:40 AM
This revision was automatically updated to reflect the committed changes.