Change KMyMoneyEdit to AmountEdit
ClosedPublic

Authored by azlev on Apr 29 2019, 1:53 AM.

Details

Summary

Refactor KMyMoney to use just AmountEdit

Test Plan

"make test" and explore the interface

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.
azlev requested review of this revision.Apr 29 2019, 1:53 AM
azlev created this revision.
azlev updated this revision to Diff 57243.Apr 30 2019, 9:52 AM
  • Show calculator in all scenarios
tbaumgart requested changes to this revision.May 7 2019, 3:38 PM
tbaumgart added a subscriber: tbaumgart.

I just did a visual inspection of the code and have not compiled nor tested it.

kmymoney/dialogs/kmymoneysplittable.cpp
955

This can be improved to

new AmountEdit(nullptr)

or better

new AmountEdit

kmymoney/dialogs/knewaccountdlg.cpp
426

Better

widget->setText(QString());

kmymoney/dialogs/knewequityentrydlg.cpp
70

Better

d->ui->edtFraction->setText(QLatin1String("100"));

kmymoney/dialogs/stdtransactioneditor.cpp
1010

Better

setText(QString())

1012

Better

setText(QString())

1029

see above

1031

see above

kmymoney/kmymoney.cpp
3559–3560

This just duplicates the previous line. Remove it.

kmymoney/widgets/kguiutils.cpp
237

This does not work :( because AmountEdit::text() returns something different than KMyMoneyEdit::text(). AmountEdit::text() is identical to KMyMoneyEdit::lineedit()->text()

I think

if (!(qobject_cast<AmountEdit*>(widget))->value().isZero()) {

could do the trick.

kmymoney/wizards/newaccountwizard/kloandetailspage.cpp
131

This might have some unwanted influence on the results which I cannot oversee right now. Please make sure to do thorough testing of the loan calculations.

210

Instead of MyMoneyMoney(0) use MyMoneyMoney(). Any MyMoneyMoney object is initialized to zero by default.

219

see above, also next line

242

Why don't you use

d->ui->m_balloonAmount->setValue(val);

directly? There's no need to create a local copy just for the setValue() call.

259–260

see above, also next lines

270–271

Again, why don't you use val directly now that it is a MyMoneyMoney object?

kmymoney/wizards/newloanwizard/keditloanwizard.cpp
138–139

m_nameEdit is not of type AmountEdit. Why did you change it?

This revision now requires changes to proceed.May 7 2019, 3:38 PM
azlev edited the summary of this revision. (Show Details)May 8 2019, 12:29 AM
azlev updated this revision to Diff 57744.May 8 2019, 1:04 AM
azlev marked 15 inline comments as done.
  • Changes suggested by Thomas Baumgart
tbaumgart added a comment.EditedMay 9 2019, 7:27 AM

Cool. This looks good. I have noticed one anomaly which I have not figured out why it happens.

Using master (w/o your change), I open the ledger view and start creating a new transaction using the form at the bottom of the view. I simply TAB through until I reach the amount field. The result is: all characters in the amount field are selected. So pressing any number replaces what is in there. This is the current behavior and OK.

Applying D20887 the cursor is located to the right of the amount and the existing characters are not selected. Pressing any number does not replace the 0,00 in the edit field.

What is more astonishing: Selecting Tools/Prices, selecting a price and pressing the Modify button and then TAB into the amount field selects the amount as I expect it. Strange.

We need to figure out what is going on here and fix it. I expect the old transaction editor to cause this as it also works OK with the new one (needs -DENABLE_UNFINISHEDFEATURES=ON). Can you check all other locations where this widget is used, please?

tbaumgart accepted this revision.May 27 2019, 2:23 PM

Let's move forward with this one. Except this one instance I did not see any problem and we can fix it. The part where the problem is located will be replaced eventually anyway. In case you don't have a dev account for the KDE repo let me know and I'll do the commit.

This revision is now accepted and ready to land.May 27 2019, 2:23 PM

I tried to debug this odd behavior last weekend with no luck.

Not sure if it's good to get technical debt on this.

Hi.

you are right, let's move on. I tried a "arc land" with no success:

PUSHING Pushing changes to "origin/master".
fatal: remote error: service not enabled: /kmymoney
Usage Exception: Push failed! Fix the error and run "arc land" again.

I don't think that I have write access to kmymoney repository.

tbaumgart retitled this revision from WIP: Change KMyMoneyEdit to AmountEdit to Change KMyMoneyEdit to AmountEdit.Jun 8 2019, 12:54 PM
tbaumgart edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.