Differential D10614 Diff 27423 kmymoney/mymoney/mymoneyaccount.cpp

Changeset View

Standalone View

View Options

kmymoney/mymoney/mymoneyaccount.cpp

Show All 22 Lines
23// QT Includes 23​// QT Includes
24 24​
25#include <QRegExp> 25​#include <QRegExp>
26#include <QPixmap> 26​#include <QPixmap>
27#include <QPixmapCache> 27​#include <QPixmapCache>
28#include <QPainter> 28​#include <QPainter>
29#include <QIcon> 29​#include <QIcon>
30#include <QDebug> 30​#include <QDebug>
31​#include <QBuffer>
31 32​
32// ---------------------------------------------------------------------------- 33​// ----------------------------------------------------------------------------
33// KDE Includes 34​// KDE Includes
34 35​
35#include <KLocalizedString> 36​#include <KLocalizedString>
36 37​
37// ---------------------------------------------------------------------------- 38​// ----------------------------------------------------------------------------
38// Project Includes 39​// Project Includes
▲ Show 20 Lines • Show All 98 Lines • ▼ Show 20 Line(s)66{
137 // But the rest of the software uses "iban". So correct this: 138​ // But the rest of the software uses "iban". So correct this:
138 if (!value("IBAN").isEmpty()) { 139​ if (!value("IBAN").isEmpty()) {
139 // If "iban" was not set, set it now. If it is set, the user reseted it already, so remove 140​ // If "iban" was not set, set it now. If it is set, the user reseted it already, so remove
140 // the garbage. 141​ // the garbage.
141 if (value(d->getAttrName(Account::Attribute::IBAN)).isEmpty()) 142​ if (value(d->getAttrName(Account::Attribute::IBAN)).isEmpty())
142 setValue(d->getAttrName(Account::Attribute::IBAN), value("IBAN")); 143​ setValue(d->getAttrName(Account::Attribute::IBAN), value("IBAN"));
143 deletePair("IBAN"); 144​ deletePair("IBAN");
144 } 145​ }
146​
147​ if (node.hasAttribute(d->getAttrName(Account::Attribute::CustomIcon)) && !node.attribute(d->getAttrName(Account::Attribute::CustomIcon)).isEmpty()) {
148​ setCustomIcon(node.attribute(d->getAttrName(Account::Attribute::CustomIcon)).toUtf8());
149​ }
145} 150​}
146 151​
147MyMoneyAccount::MyMoneyAccount(const MyMoneyAccount& other) : 152​MyMoneyAccount::MyMoneyAccount(const MyMoneyAccount& other) :
148 MyMoneyObject(*new MyMoneyAccountPrivate(*other.d_func()), other.id()), 153​ MyMoneyObject(*new MyMoneyAccountPrivate(*other.d_func()), other.id()),
149 MyMoneyKeyValueContainer(other) 154​ MyMoneyKeyValueContainer(other)
150{ 155​{
151} 156​}
152 157​
▲ Show 20 Lines • Show All 169 Lines • ▼ Show 20 Line(s)323 return (MyMoneyKeyValueContainer::operator==(right) &&
322 (d->m_lastModified == d2->m_lastModified) && 327​ (d->m_lastModified == d2->m_lastModified) &&
323 (d->m_lastReconciliationDate == d2->m_lastReconciliationDate) && 328​ (d->m_lastReconciliationDate == d2->m_lastReconciliationDate) &&
324 ((d->m_name.length() == 0 && d2->m_name.length() == 0) || (d->m_name == d2->m_name)) && 329​ ((d->m_name.length() == 0 && d2->m_name.length() == 0) || (d->m_name == d2->m_name)) &&
325 ((d->m_number.length() == 0 && d2->m_number.length() == 0) || (d->m_number == d2->m_number)) && 330​ ((d->m_number.length() == 0 && d2->m_number.length() == 0) || (d->m_number == d2->m_number)) &&
326 ((d->m_description.length() == 0 && d2->m_description.length() == 0) || (d->m_description == d2->m_description)) && 331​ ((d->m_description.length() == 0 && d2->m_description.length() == 0) || (d->m_description == d2->m_description)) &&
327 (d->m_openingDate == d2->m_openingDate) && 332​ (d->m_openingDate == d2->m_openingDate) &&
328 (d->m_parentAccount == d2->m_parentAccount) && 333​ (d->m_parentAccount == d2->m_parentAccount) &&
329 (d->m_currencyId == d2->m_currencyId) && 334​ (d->m_currencyId == d2->m_currencyId) &&
330 (d->m_institution == d2->m_institution)); 335​ (d->m_institution == d2->m_institution) &&
336​ (d->m_customIcon.toImage() == d2->m_customIcon.toImage()));
331} 337​}
332 338​
333Account::Type MyMoneyAccount::accountGroup() const 339​Account::Type MyMoneyAccount::accountGroup() const
334{ 340​{
335 Q_D(const MyMoneyAccount); 341​ Q_D(const MyMoneyAccount);
336 switch (d->m_accountType) { 342​ switch (d->m_accountType) {
337 case Account::Type::Checkings: 343​ case Account::Type::Checkings:
338 case Account::Type::Savings: 344​ case Account::Type::Savings:
▲ Show 20 Lines • Show All 134 Lines • ▼ Show 20 Line(s)436{
473 // not stored there after setLastReconciliationDate() has been changed 479​ // not stored there after setLastReconciliationDate() has been changed
474 // See comment there when this will happen 480​ // See comment there when this will happen
475 // deletePair("lastStatementDate"); 481​ // deletePair("lastStatementDate");
476 482​
477 483​
478 //Add in Key-Value Pairs for accounts. 484​ //Add in Key-Value Pairs for accounts.
479 MyMoneyKeyValueContainer::writeXML(document, el); 485​ MyMoneyKeyValueContainer::writeXML(document, el);
480 486​
487​ if (!d->m_customIcon.isNull())
488​ el.setAttribute(d->getAttrName(Account::Attribute::CustomIcon), customIconB64());
wojnilowiczUnsubmitted
Done

As I see you want to store icons in a XML or DB file. In my opinion that's a bad idea because:

  1. it will blow XML file rapidly,
  2. it will introduce obfuscated data to XML file and debugging will be more difficult.

Why don't you use icon name and some local directory for account icons? We do something similar with CSS files customized for user plasma theme.

wojnilowicz: As I see you want to store icons in a XML or DB file. In my opinion that's a bad idea because…
mhubnerAuthorUnsubmitted
Done

ad 1: I thought about splitting storage here (store icons on filesystem and reference in database), but decided against it weighing the expected amount of "blob" data in the DB (it's just a couple of icons...) vs. the additional complexity (extra checks that ensure DB is in sync with filesystem/files didn't get deleted etc.). Will change it if you insist.

ad 2: I see your point about readability here. How about moving the icon stuff into an own node at the very bottom of the XML and then just referencing to it from the account?

Additionally, so far everything seemed to be self contained within the DB and I didn't want to break that pattern; honestly speaking I wasn't aware of the CSS situation. Will look into it.

mhubner: ad 1: I thought about splitting storage here (store icons on filesystem and reference in…
wojnilowiczUnsubmitted
Done

I think your original thought was right. I just don't see icons in XML storage inline or at the bottom. Maybe somebody else will be happy with it.

It's just a couple of icons for you, but in case of equities, which are treated like accounts in KMM, the count can grow rather rapidly and I think we should have that in mind as well.
If I get it correctly, the conversions from and to base64 would require rather a lot of processing power and that's all at each save and load of storage.

wojnilowicz: I think your original thought was right. I just don't see icons in XML storage inline or at the…
489​
481 parent.appendChild(el); 490​ parent.appendChild(el);
482} 491​}
483 492​
484bool MyMoneyAccount::hasReferenceTo(const QString& id) const 493​bool MyMoneyAccount::hasReferenceTo(const QString& id) const
485{ 494​{
486 Q_D(const MyMoneyAccount); 495​ Q_D(const MyMoneyAccount);
487 return (id == d->m_institution) || (id == d->m_parentAccount) || (id == d->m_currencyId); 496​ return (id == d->m_institution) || (id == d->m_parentAccount) || (id == d->m_currencyId);
488} 497​}
▲ Show 20 Lines • Show All 90 Lines • ▼ Show 20 Line(s)
579void MyMoneyAccount::setBalance(const MyMoneyMoney& val) 588​void MyMoneyAccount::setBalance(const MyMoneyMoney& val)
580{ 589​{
581 Q_D(MyMoneyAccount); 590​ Q_D(MyMoneyAccount);
582 d->m_balance = val; 591​ d->m_balance = val;
583} 592​}
584 593​
585QPixmap MyMoneyAccount::accountPixmap(const bool reconcileFlag, const int size) const 594​QPixmap MyMoneyAccount::accountPixmap(const bool reconcileFlag, const int size) const
586{ 595​{
596​ Q_D(const MyMoneyAccount);
587 static const QHash<Account::Type, Icon> accToIco { 597​ static const QHash<Account::Type, Icon> accToIco {
588 {Account::Type::Asset, Icon::ViewAsset}, 598​ {Account::Type::Asset, Icon::ViewAsset},
589 {Account::Type::Investment, Icon::ViewStock}, 599​ {Account::Type::Investment, Icon::ViewStock},
590 {Account::Type::Stock, Icon::ViewStock}, 600​ {Account::Type::Stock, Icon::ViewStock},
591 {Account::Type::MoneyMarket, Icon::ViewStock}, 601​ {Account::Type::MoneyMarket, Icon::ViewStock},
592 {Account::Type::Checkings, Icon::ViewChecking}, 602​ {Account::Type::Checkings, Icon::ViewChecking},
593 {Account::Type::Savings, Icon::ViewSaving}, 603​ {Account::Type::Savings, Icon::ViewSaving},
594 {Account::Type::AssetLoan, Icon::ViewLoanAsset}, 604​ {Account::Type::AssetLoan, Icon::ViewLoanAsset},
595 {Account::Type::Loan, Icon::ViewLoan}, 605​ {Account::Type::Loan, Icon::ViewLoan},
596 {Account::Type::CreditCard, Icon::ViewCreditCard}, 606​ {Account::Type::CreditCard, Icon::ViewCreditCard},
597 {Account::Type::Asset, Icon::ViewAsset}, 607​ {Account::Type::Asset, Icon::ViewAsset},
598 {Account::Type::Cash, Icon::ViewCash}, 608​ {Account::Type::Cash, Icon::ViewCash},
599 {Account::Type::Income, Icon::ViewIncome}, 609​ {Account::Type::Income, Icon::ViewIncome},
600 {Account::Type::Expense, Icon::ViewExpense}, 610​ {Account::Type::Expense, Icon::ViewExpense},
601 {Account::Type::Equity, Icon::ViewEquity} 611​ {Account::Type::Equity, Icon::ViewEquity}
602 }; 612​ };
603 613​
604 Icon ixIcon = accToIco.value(accountType(), Icon::ViewLiability); 614​ Icon ixIcon = accToIco.value(accountType(), Icon::ViewLiability);
605 615​
606 QString kyIcon = accountTypeToString(accountType()) + QString::number(size); 616​ QString kyIcon = accountTypeToString(accountType()) + QString::number(size);
607 QPixmap pxIcon; 617​ QPixmap pxIcon;
608 618​
609 if (!QPixmapCache::find(kyIcon, pxIcon)) { 619​ if (!QPixmapCache::find(kyIcon, pxIcon)) {
610 pxIcon = Icons::get(ixIcon).pixmap(size); // Qt::AA_UseHighDpiPixmaps (in Qt 5.7) doesn't return highdpi pixmap 620​ pxIcon = Icons::get(ixIcon).pixmap(size); // Qt::AA_UseHighDpiPixmaps (in Qt 5.7) doesn't return highdpi pixmap
611 QPixmapCache::insert(kyIcon, pxIcon); 621​ QPixmapCache::insert(kyIcon, pxIcon);
wojnilowiczUnsubmitted
Not Done

I think you should check whether customIconPath is empty first and if not then try to load the icon. In my opinion it's not most likely condition and you place it first.

wojnilowicz: I think you should check whether customIconPath is empty first and if not then try to load the…
612 } 622​ }
613 623​
624​ if (!d->m_customIcon.isNull())
625​ pxIcon = d->m_customIcon;
626​
614 if (isClosed()) 627​ if (isClosed())
615 ixIcon = Icon::AccountClosed; 628​ ixIcon = Icon::AccountClosed;
616 else if (reconcileFlag) 629​ else if (reconcileFlag)
617 ixIcon = Icon::FlagGreen; 630​ ixIcon = Icon::FlagGreen;
618 else if (hasOnlineMapping()) 631​ else if (hasOnlineMapping())
619 ixIcon = Icon::Download; 632​ ixIcon = Icon::Download;
620 else 633​ else
621 return pxIcon; 634​ return pxIcon;
▲ Show 20 Lines • Show All 118 Lines • ▼ Show 20 Line(s)727{
740 return list; 753​ return list;
741} 754​}
742 755​
743bool MyMoneyAccount::hasOnlineMapping() const 756​bool MyMoneyAccount::hasOnlineMapping() const
744{ 757​{
745 Q_D(const MyMoneyAccount); 758​ Q_D(const MyMoneyAccount);
746 return !d->m_onlineBankingSettings.value(QLatin1String("provider")).isEmpty(); 759​ return !d->m_onlineBankingSettings.value(QLatin1String("provider")).isEmpty();
747} 760​}
761​
762​void MyMoneyAccount::setCustomIcon(const QPixmap& pixmap)
763​{
764​ Q_D(MyMoneyAccount);
765​ if (!pixmap.isNull())
766​ d->m_customIcon = pixmap;
wojnilowiczUnsubmitted
Done

In my opinion it's not a good idea, because you hold lots of data of pixmap in memory twice. First in the variable and second in the accounts tree.

wojnilowicz: In my opinion it's not a good idea, because you hold lots of data of pixmap in memory twice.
mhubnerAuthorUnsubmitted
Done

Not sure whether I fully get your point here. Where's the extra copy? What do you suggest?

mhubner: Not sure whether I fully get your point here. Where's the extra copy? What do you suggest?
wojnilowiczUnsubmitted
Done

The icon that you hold in d->m_customIcon is not the icon that you see on your screenshots. They are equal but copies.

My work on KMM is to eliminate copies of code and I don't need more work to be added :) The best would be to load the icon, then set it in accounts tree, then forget it and not hold it in d->m_customIcon. In that way usage of memory isn't growing twice or more with every icon added.

wojnilowicz: The icon that you hold in d->m_customIcon is not the icon that you see on your screenshots.
767​}
768​
769​void MyMoneyAccount::setCustomIcon(const QString& base64image)
770​{
771​ Q_D(MyMoneyAccount);
772​ QByteArray iconBytes = QByteArray::fromBase64(base64image.toUtf8());
773​ d->m_customIcon.loadFromData(iconBytes);
774​}
775​
776​QPixmap MyMoneyAccount::customIcon() const
777​{
778​ Q_D(const MyMoneyAccount);
779​ return d->m_customIcon;
780​}
781​
782​QString MyMoneyAccount::customIconB64() const
783​{
784​ Q_D(const MyMoneyAccount);
785​
786​ QByteArray pixelBytes;
787​ QBuffer iconBuffer(&pixelBytes);
788​ d->m_customIcon.save(&iconBuffer, "PNG");
789​ auto b64string = QString(pixelBytes.toBase64());
790​ return b64string;
791​}
792​
793​void MyMoneyAccount::removeCustomIcon()
794​{
795​ Q_D(MyMoneyAccount);
796​ d->m_customIcon = QPixmap();
797​}
798​
799​bool MyMoneyAccount::hasCustomIcon() const
800​{
801​ Q_D(const MyMoneyAccount);
802​ return !d->m_customIcon.isNull();
803​}