| wojnilowicz |
| KMyMoney |
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).
| No Linters Available |
| No Unit Test Coverage |
| kmymoney/mymoney/mymoneyaccount.cpp | ||
|---|---|---|
| 488 | As I see you want to store icons in a XML or DB file. In my opinion that's a bad idea because:
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. | |
| 766 | 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. | |
| kmymoney/mymoney/mymoneyaccount.cpp | ||
|---|---|---|
| 488 | 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. | |
| 766 | Not sure whether I fully get your point here. Where's the extra copy? What do you suggest? | |
| kmymoney/mymoney/mymoneyaccount.cpp | ||
|---|---|---|
| 488 | 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. | |
| 766 | 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.
| kmymoney/views/kaccountsview.cpp | ||
|---|---|---|
| 183 | Disable the actions if the current item is a standard account. | |
| kmymoney/menus/menuenums.h | ||
|---|---|---|
| 65 ↗ | (On Diff #32681) | 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. |
| kmymoney/mymoney/mymoneyaccount.cpp | ||
| 621 | 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. | |
| kmymoney/views/kaccountsview.cpp | ||
| 189 | 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? | |
| 360 | 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. | |
| 361 | Is that proxy method really needed? | |
| 367 | Is that proxy method really needed? | |
| kmymoney/views/kaccountsview_p.h | ||
| 315 | 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?