Use price precision setting on investment prices report
AbandonedPublic

Authored by wojnilowicz on Feb 17 2017, 8:37 PM.

Details

Summary

BUG:294268

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.
wojnilowicz retitled this revision from to Use price precision setting on investment prices report.
wojnilowicz updated this object.
wojnilowicz edited the test plan for this revision. (Show Details)
wojnilowicz added a reviewer: KMyMoney.
wojnilowicz set the repository for this revision to R261 KMyMoney.
wojnilowicz added a project: KMyMoney.
wojnilowicz added a subscriber: KMyMoney.
christiand added inline comments.
kmymoney/reports/pivottable.h
164

Hi Lukasz,

I think here it is better to add an overload:

 QString coloredAmount(const MyMoneyMoney& amount, const QString& currencySymbol = QString()) const
{
  return coloredAmount(amount, currencySymbol, KMyMoneyGlobalSettings::pricePrecision());
}

and change this function to:

QString coloredAmount(const MyMoneyMoney& amount, const QString& currencySymbol, int prec) const;
wojnilowicz added inline comments.Feb 17 2017, 9:10 PM
kmymoney/reports/pivottable.h
164

Outcome is the same. Why overload?

christiand added inline comments.Feb 18 2017, 12:38 PM
kmymoney/reports/pivottable.h
164

The only reason is my personal view of style.

The set default value is not the real value to use but a code for something different. If a caller uses the function he has to notice that -1 itself is a useless value and thus encodes something else instead. Also someone could have the idea to explicitly set this parameter to -1 which means you cannot change that behavior in future.

This revision was automatically updated to reflect the committed changes.
conet reopened this revision.Mar 15 2017, 8:37 PM
conet added a subscriber: conet.

This breaks "Net worth" reports which are no longer displayed using the account's currency precision but use the global price precision instead.

No problem with my net worth reports.

conet added a comment.Mar 31 2017, 1:22 PM

No problem with my net worth reports.

Really? Aren't the amounts displayed with pricePrecision positions, instead of the usual 2 (currency precision) positions?

In D4656#99194, @conet wrote:

No problem with my net worth reports.

Really? Aren't the amounts displayed with pricePrecision positions, instead of the usual 2 (currency precision) positions?

Yes they are displayed with pricePrecision, but "This breaks "Net worth" reports which are no longer displayed" is false. Usual for Kuwaiti Dinar is not 2 but 3 (currency precision) positions.

conet added a comment.EditedApr 3 2017, 6:03 AM
In D4656#99194, @conet wrote:

No problem with my net worth reports.

Really? Aren't the amounts displayed with pricePrecision positions, instead of the usual 2 (currency precision) positions?

Yes they are displayed with pricePrecision, but "This breaks "Net worth" reports which are no longer displayed" is false. Usual for Kuwaiti Dinar is not 2 but 3 (currency precision) positions.

Yeah I knew that, so what I was trying to say that it breaks the report for most currencies with a precision of 2. Fixing the report for Kuwaiti Dinar and all other currencies for which the hardcoded value just worked is another issue which should be done.

Cristian,
this shouldn't be problem anymore. Reopen if not.

Łukasz

wojnilowicz abandoned this revision.Apr 15 2017, 1:13 PM