Custom icons for accounts
Needs ReviewPublic

Authored by mhubner on Feb 17 2018, 7:55 PM.


Group Reviewers

This patch adds the possibility to assign a custom icon to each account.
In case a custom icon has been set up for an account, it is shown instead of the default icon in the accounts view as well as in the accounts combobox (e.g. in the ledger). In case no custom icon is assigned default icon is used (as before).

Diff Detail

R261 KMyMoney
custom_icons (branched from master)
No Linters Available
No Unit Test Coverage
mhubner requested review of this revision.Feb 17 2018, 7:55 PM
mhubner created this revision.
mhubner retitled this revision from This patch adds the possibility to assign a custom icon to each account. to Custom icons for accounts.Feb 17 2018, 7:57 PM
mhubner edited the summary of this revision. (Show Details)
mhubner added a reviewer: KMyMoney.
mhubner added a project: KMyMoney.
mhubner edited the summary of this revision. (Show Details)Feb 17 2018, 8:01 PM
wojnilowicz requested changes to this revision.Feb 18 2018, 3:42 PM
wojnilowicz added a subscriber: wojnilowicz.
wojnilowicz added inline comments.

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.


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.

This revision now requires changes to proceed.Feb 18 2018, 3:42 PM
mhubner added inline comments.Feb 18 2018, 4:36 PM

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.


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

wojnilowicz added inline comments.Feb 18 2018, 5:06 PM

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.


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.

In case you change kmymoney/kmymoneyui.rc you need to bump the version in the beginning of the file.

In my opinion, you should only keep the file location with the account data. You can store this value as part of the KVP so you don't need to enhance the interface. Use MyMoneyAccount::setValue() and MyMoneyAccount::value() as setter and getter. Use kmm_icon_file or similar as name for the key and the file location as the value. After all, this is not a core feature.

Don't store the icons inside the data file. In case you don't find the icon, simply use the standard one. In case you want to keep a (hidden) copy of the icon, make a copy in a local directory (e.g. ~/.local/share/kmymoney/account-icons/). You can see how this is done in the OFX plugin. This one downloads files to that kmymoney directory.


Disable the actions if the current item is a standard account.

mhubner updated this revision to Diff 32681.Apr 20 2018, 11:54 PM

Custom icons for accounts
don't store icons in database as discussed in review.

mhubner marked 7 inline comments as done.Apr 20 2018, 11:58 PM
christiand added inline comments.

I would prefer to name the "remove" action "reset to default" because that is what it actually does.


Qt can also process gif, pbm, pgm, ppm, xbm and xpm images, please add them in the filter list.

wojnilowicz added inline comments.Apr 22 2018, 5:40 PM

Agreed. You either "set" and "unset" (reset) or "add" and "remove". I think those two pairs of words should go together and not mix with each other.


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.


Maybe I'm not seeing something, but It's not possible to set custom icon if it hasn't been set before. Is that correct?


It would be nice if you could check if the user doesn't give us bmp file sized 2000 px x 2000 px.

It would also be nice if you could scale the icon user provided and copy it so that the fileName will never become invalid.


Is that proxy method really needed?


Is that proxy method really needed?


You should pass by reference like that

const QString& iconFilename

Can you check, if you can use Icons::loadIconFromApplicationCache() and Icons::storeIconInApplicationCache() for your storage?

wojnilowicz resigned from this revision.Mar 10 2019, 6:51 AM