Details

Reviewers

tbaumgart

Group Reviewers

KMyMoney

Commits

R261:bfa516d09143: Refactor file menu

Summary

The purpose of this patch is to make file opening transparent. Changes:

no wild attaching and detaching storage

That was cause of a memory leak. Now a storage is created and attached only after successful opening of a file.

new KMyMoneyApp::Private::fileAction

It's central place for gathering all actions, app needs to undertake after a change in load state of a file.

user can choose to save newly created file as sql

Previously only xml was possible. Now user can even choose to not save it at all, which makes app independent from any storage plugin.

app doesn't crash if no storage plugin is loaded

no unnecessary update of actions which depend on load state of file

Test Plan

Created, opened, closed, saved, saved as: XML, SQL, GNC file.

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 requested review of this revision.Jun 7 2018, 3:21 PM

wojnilowicz created this revision.

Here are my findings:

I needed to apply the following patch to compile it:

diff --git a/kmymoney/kmymoney.cpp b/kmymoney/kmymoney.cpp
index a334f7df..c48b4157 100644
--- a/kmymoney/kmymoney.cpp
+++ b/kmymoney/kmymoney.cpp
@@ -1990,7 +1990,7 @@ void KMyMoneyApp::slotShowAllAccounts()
 #ifdef KMM_DEBUG
 void KMyMoneyApp::slotFileFileInfo()
 {
-  if (!d->m_fileOpen) {
+  if (!d->m_fileInfo.isOpened) {
     KMessageBox::information(this, i18n("No KMyMoneyFile open"));
     return;
   }

When the backend is closed via File/Close or Ctrl+W:
- the File/Import and File/Export menus and their entries are not disabled. Selecting them crashes the application.
- the New credit transfer button is not disabled. Pressing it crashes the application
- the Update all accounts button is not disabled. Pressing it crashes the application
- the Tools/Performance test option is not disabled. Selecting it crashes the application
Having one XML file (GZIP) open and using File/Open recent to select another XML (GPG encrypted) shows the message Cannot open file as requested. Error was: No storage object attached to MyMoneyFile /home/thb/devel/kmymoney/kmymoney/mymoney/mymoneyfile.cpp:214 This maybe just a special case, see next entry.
Opening GPG encrypted files does not work at all. I started from the CLI with a filename provided. It always shows the message mentioned in the previous line. This is a show stopper!!
Open an XML file. Make changes. Don't save. Open another file and press Cancel on the Save changes dialog. This will result in the message Cannot open file as requested. Error was: Storage already attached /home/thb/devel/kmymoney/kmymoney/mymoney/mymoneyfile.cpp:335 It should just stop the action without message.
The new Save storage as dialog needs some more explanation what the options are for the user. I strongly suggest to avoid the terms XML and SQL but rather show File based storage and Database system based storage and explain those in some text above the combo box. This text could include information that the selection is depending on the available plugins The current form is not very user friendly

kmymoney/kmymoney.cpp
250	Since it is not necessarily a file, I suggest to call this type `backendInfo` instead of `fileInfo`.
256	Same here: I'd call it `m_backendInfo`
549	needToSave is probably a better name
3496	Wording suggestion: Could not read your data source. Please check the KMyMoney settings that the necessary plugin is enabled.
kmymoney/plugins/xml/xmlstorage.cpp
344	What does this do in context of an anon.xml write?

This revision now requires changes to proceed.Jun 8 2018, 3:08 PM

+ one nitpick from me (in terms of user experience): is there a way to 'default' (if present) to kmy/xml files in the open file dialog filter list?
(Maybe I'm on Windows) but right now when I click File, Open, it defaults to "GnuCash files" since it's top in the filter list.
Is there a way to push "KMyMoney files" to top of the list (when available)?

kmymoney/kmymoney.cpp
2385	(just a minor slip:) `d->m_fileInfo.url.toLocalFile()`
3468	if (!slotFileClose()) return false; could solve Thomas's bullet pt 4 perhaps?

Apologies that I'll add one more finding (in addition to Thomas's observations):

[newly created file from the wizard] .... Now user can even choose to not save it at all, ....

Indeed if the user cancels file saving immediately following the conclusion of the new file wizard, he won't be prompted again at all, e.g. on file close/exit, with or without further changes made to the new file.

Seems like there needs be a new file state/mechanism that covers the 'new born' stage before the first file save (having a filename/db path), so that e.g. it could be properly marked as 'dirty', and to enable the "Save" instead of the "Save as" option in the File menu to provide the first-time saving option.

kmymoney/kmymoney.cpp
3407	Indeed, I'll propose not to prompt for file save at the conclusion of the new file wizard at all—to save it until later in the course of working with the file: say, the user may not be saving the new file at the end of the day at all (say, he might be trying out new features in a playground), or to give him the liberty to enable another storage plugin (say, when he realizes he wanted to save as a kmy file afterall that the xml storage was inadvertently disabled) before saving the newly created file. (At any rate, see my comment above regarding the 'dirty' state of a newly created (and unsaved) file.)

ocoole added inline comments.Jun 8 2018, 5:45 PM

kmymoney/kmymoney.cpp
3468	(sorry, I mean bullet point no. 5)

In D13408#275883, @tbaumgart wrote:

Here are my findings:

I needed to apply the following patch to compile it:

` diff --git a/kmymoney/kmymoney.cpp b/kmymoney/kmymoney.cpp index a334f7df..c48b4157 100644

a/kmymoney/kmymoney.cpp +++ b/kmymoney/kmymoney.cpp @@ -1990,7 +1990,7 @@ void KMyMoneyApp::slotShowAllAccounts() #ifdef KMM_DEBUG void KMyMoneyApp::slotFileFileInfo() {

if (!d->m_fileOpen) { + if (!d->m_fileInfo.isOpened) { KMessageBox::information(this, i18n("No KMyMoneyFile open")); return; } `

When the backend is closed via File/Close or Ctrl+W:

the File/Import and File/Export menus and their entries are not disabled. Selecting them crashes the application.

the New credit transfer button is not disabled. Pressing it crashes the application

the Update all accounts button is not disabled. Pressing it crashes the application

the Tools/Performance test option is not disabled. Selecting it crashes the application

Having one XML file (GZIP) open and using File/Open recent to select another XML (GPG encrypted) shows the message Cannot open file as requested. Error was: No storage object attached to MyMoneyFile /home/thb/devel/kmymoney/kmymoney/mymoney/mymoneyfile.cpp:214 This maybe just a special case, see next entry.

Opening GPG encrypted files does not work at all. I started from the CLI with a filename provided. It always shows the message mentioned in the previous line. This is a show stopper!!

Open an XML file. Make changes. Don't save. Open another file and press Cancel on the Save changes dialog. This will result in the message Cannot open file as requested. Error was: Storage already attached /home/thb/devel/kmymoney/kmymoney/mymoney/mymoneyfile.cpp:335 It should just stop the action without message.

All of the above should be fixed by now. Please retest.

The new Save storage as dialog needs some more explanation what the options are for the user. I strongly suggest to avoid the terms XML and SQL but rather show File based storage and Database system based storage and explain those in some text above the combo box. This text could include information that the selection is depending on the available plugins The current form is not very user friendly

I think we should use XML and SQL terms because we already do that for plugin names and we should be consistent with it. I think the user doesn't need to actively choose any of these options if he isn't aware what he's doing. In case he isn't aware, I've put an explanation to what to do when in doubt.

kmymoney/kmymoney.cpp
250	I prefer calling it storageInfo as backendInfo is too generic.
3407	I consider it as done now.
kmymoney/plugins/xml/xmlstorage.cpp
344	I bet nothing good. I moved it.

Fixed all reported deficiencies.

When KMyMoney is started with the -n command line option
- File export/Schedules to iCalendar is still available (you need to have this plugin enabled)

When KMyMoney is started without options to open the last loaded storage
- File/Close or Ctrl+W don't work anymore. They show an error (see also message on CLI)
- Category/New Category is never enabled
- Use File/Open recent and select a previously open file or database. This causes an error message to appear (without actual contents) and KMyMoney crashes. Show stopper

Select the Ledger view to show the ledger of an account. Switch to Home view. The actions in the Transaction menu are not disabled.

I did not have time to do more testing atm.

kmymoney/kmymoney.cpp
264	Didn't you rename this into `m_storageInfo`?

ocoole added inline comments.Jun 9 2018, 9:06 AM

kmymoney/kmymoney.cpp
2388	(this should be `d->m_storageInfo`... now)

In D13408#276141, @tbaumgart wrote:

When KMyMoney is started with the -n command line option

File export/Schedules to iCalendar is still available (you need to have this plugin enabled)

Fixed.

When KMyMoney is started without options to open the last loaded storage

File/Close or Ctrl+W don't work anymore. They show an error (see also message on CLI)

I cannot reproduce this. If you open KMM without a file, then Ctrl+W shouldn't be available.

Category/New Category is never enabled

I did not change this behaviour. Category/New Category activates on categories view if you select one of the categories.

Use File/Open recent and select a previously open file or database. This causes an error message to appear (without actual contents) and KMyMoney crashes. Show stopper

I cannot reproduce this. Are you sure it's my patch fault?

Select the Ledger view to show the ledger of an account. Switch to Home view. The actions in the Transaction menu are not disabled.

Fixed.

I did not have time to do more testing atm.

wojnilowicz updated this revision to Diff 35878.Jun 9 2018, 9:12 AM

ocoole added inline comments.Jun 9 2018, 11:40 AM

kmymoney/kmymoney.cpp
3412–3413	These two lines (3412 and 3413) will add an empty entry to the recent files list if user cancels save after new file wizard. If user clicks on the empty entry in Open Recent list, app will throw an error and crash. In any case, I gather that the storage plugins already take care to add to the recent files list/`writeLastUsedFile`, so I think these 2 lines can be removed.

Dear Thomas / Łukasz:

Could you try and see if you could reproduce this:

I opened an existing kmy/xml file, made some changes to it, and pressed Save.
It throws an error: "Unable to write changes to '[/path/to/file]' [...]\kmymoney\plugins\xml\xmlstorage.cpp:271"

(Interestingly when I hit Save a second time, the second time succeeds)

IMO this would really be a show stopper indeed coz file saving seems to be, in a sense, broken.

(Many thanks both of you for your efforts!) (I've nothing further to add regarding this patch)

ocoole removed a subscriber: ocoole.Jun 9 2018, 12:22 PM

wojnilowicz updated this revision to Diff 35888.Jun 9 2018, 1:11 PM

In D13408#276223, @ocoole wrote:

Dear Thomas / Łukasz:

Could you try and see if you could reproduce this:

I opened an existing kmy/xml file, made some changes to it, and pressed Save.
It throws an error: "Unable to write changes to '[/path/to/file]' [...]\kmymoney\plugins\xml\xmlstorage.cpp:271"

(Interestingly when I hit Save a second time, the second time succeeds)

IMO this would really be a show stopper indeed coz file saving seems to be, in a sense, broken.

(Many thanks both of you for your efforts!) (I've nothing further to add regarding this patch)

I cannot reproduce this. I think it's not a show stopper, if you cannot reproduce it neither. It would be helpful to know what were you modifying. Anyhow I changed xmlstorage.cpp:271 a little bit so you can observe backtrace and exception messages better.

kmymoney/kmymoney.cpp
3412–3413	Thanks.

wojnilowicz marked an inline comment as done.Jun 9 2018, 1:14 PM

In D13408#276152, @wojnilowicz wrote:

In D13408#276141, @tbaumgart wrote:

When KMyMoney is started without options to open the last loaded storage

File/Close or Ctrl+W don't work anymore. They show an error (see also message on CLI)

I cannot reproduce this. If you open KMM without a file, then Ctrl+W shouldn't be available.

I stated, that you need to open a recent file and then close it. Activate all plugins (the iCalendar one is causing the crash) and also make sure to compile with ENABLE_UNFINISHEDFEATURES set to ON and you will be able to reproduce this and maybe find others.

In D13408#276152, @wojnilowicz wrote:

In D13408#276141, @tbaumgart wrote:

Category/New Category is never enabled

I did not change this behaviour. Category/New Category activates on categories view if you select one of the categories.

Then it might be a leftover from one of your recent changes. Anyway, it needs to be fixed. Otherwise you will not be able to create the first category ever in a new file (except loading templates). And this is new user experience.

This revision now requires changes to proceed.Jun 9 2018, 3:34 PM

Fixed icalendar crash.

In D13408#276278, @tbaumgart wrote:

In D13408#276152, @wojnilowicz wrote:

In D13408#276141, @tbaumgart wrote:

Category/New Category is never enabled

I did not change this behaviour. Category/New Category activates on categories view if you select one of the categories.

Then it might be a leftover from one of your recent changes. Anyway, it needs to be fixed. Otherwise you will not be able to create the first category ever in a new file (except loading templates). And this is new user experience.

Below is new file without templates imported and I can create a new category.

Here's another weirdness: Start KMyMoney which opens the last selected file. Select the categories view and select one category (anyone should do). Close the file with Ctrl-W. Accounts/Update all accounts is still enabled and crashes. Transaction/Select allis also enabled when file is closed.

Fixed issues with Select All Transactions and Update All Accounts.

wojnilowicz updated this revision to Diff 35930.Jun 10 2018, 5:20 AM

tbaumgart requested changes to this revision.Jun 10 2018, 8:43 AM

tbaumgart added inline comments.

kmymoney/kmymoney.cpp
374	The condition needs to be `it_p == pPlugins.online.constEnd()`, otherwise this does not work.
kmymoney/views/kaccountsview.cpp
34	In `canUpdateAllAccounts()` you check, that the provider is present. Here you don't. This will enable the action even if the provider mapped is currently not available. Example: I map one account with KBanking and one with OFX. Then I disable KBanking, but since the OFX one is still found, the `Update account` action will be enabled for the account mapped to KBanking.

This revision now requires changes to proceed.Jun 10 2018, 8:43 AM

In D13408#276248, @wojnilowicz wrote:

In D13408#276223, @ocoole wrote:

Dear Thomas / Łukasz:

Could you try and see if you could reproduce this:
...

I cannot reproduce this. ... Anyhow I changed xmlstorage.cpp:271 a little bit so you can observe backtrace and exception messages better.

Thanks Łukasz. I've identified my last observation: seems like a Windows-specific bug (which's been outstanding for some time). I'll submit a separate patch to address that issue. Thanks for your help! (Meanwhile you may revert the change of printing debug info on xmlstorage.cpp:271.)

wojnilowicz updated this revision to Diff 35947.Jun 10 2018, 12:39 PM

I have done some more testing and I think this is now good to go into the repo. The remaining issues I found are not related to this patch and can be fixed on the fly.

This revision was not accepted when it landed; it landed in state Needs Review.Jun 10 2018, 3:24 PM

Closed by commit R261:bfa516d09143: Refactor file menu (authored by wojnilowicz). · Explain Why

This revision was automatically updated to reflect the committed changes.

		Path
M		kmymoney/dialogs/CMakeLists.txt (4 lines)
D	M	kmymoney/dialogs/kgpgkeyselectiondlg.h (78 lines)
D	M	kmymoney/dialogs/kgpgkeyselectiondlg.cpp (201 lines)
D	M	kmymoney/dialogs/kgpgkeyselectiondlg.ui (139 lines)
A	M	kmymoney/dialogs/ksaveasquestion.h (39 lines)
A	M	kmymoney/dialogs/ksaveasquestion.cpp (62 lines)
A	M	kmymoney/dialogs/ksaveasquestion.ui (75 lines)
M		kmymoney/kmymoney.h (91 lines)
M		kmymoney/kmymoney.cpp (1195 lines)
A	M	kmymoney/kmymoneyenums.h (42 lines)
M		kmymoney/main.cpp (1 line)
M		kmymoney/mymoney/onlinejobadministration.cpp (6 lines)
M		kmymoney/plugins/appinterface.h (3 lines)
M		kmymoney/plugins/csv/export/csvexporter.cpp (11 lines)
M		kmymoney/plugins/csv/import/csvimporter.cpp (5 lines)
M		kmymoney/plugins/gnc/import/CMakeLists.txt (1 line)
M		kmymoney/plugins/gnc/import/gncimporter.h (7 lines)
M		kmymoney/plugins/gnc/import/gncimporter.cpp (77 lines)
M		kmymoney/plugins/gnc/import/mymoneygncreader.h (4 lines)
M		kmymoney/plugins/icalendar/export/icalendarexporter.cpp (5 lines)
M		kmymoney/plugins/icalendar/export/schedulestoicalendar.cpp (3 lines)
M		kmymoney/plugins/interfaces/kmmappinterface.h (2 lines)
M		kmymoney/plugins/interfaces/kmmappinterface.cpp (10 lines)
M		kmymoney/plugins/kmymoneyplugin.h (13 lines)
M		kmymoney/plugins/kmymoneyplugin.cpp (5 lines)
M		kmymoney/plugins/ofx/import/ofximporter.cpp (9 lines)
M		kmymoney/plugins/qif/export/qifexporter.cpp (5 lines)
M		kmymoney/plugins/qif/import/qifimporter.cpp (5 lines)
M		kmymoney/plugins/sql/sqlstorage.h (7 lines)
M		kmymoney/plugins/sql/sqlstorage.cpp (169 lines)
M		kmymoney/plugins/xml/CMakeLists.txt (3 lines)
M		kmymoney/plugins/xml/mymoneystoragexml.h (10 lines)
M		kmymoney/plugins/xml/xmlstorage.h (9 lines)
M		kmymoney/plugins/xml/xmlstorage.cpp (223 lines)
D	M	kmymoney/plugins/xml/xmlstorage.rc (8 lines)
M		kmymoney/views/kaccountsview.cpp (22 lines)
M		kmymoney/views/kgloballedgerview.cpp (12 lines)
M		kmymoney/views/khomeview.cpp (5 lines)
M		kmymoney/views/khomeview_p.h (5 lines)
M		kmymoney/views/kmymoneyview.h (43 lines)
M		kmymoney/views/kmymoneyview.cpp (76 lines)
M		kmymoney/views/viewenums.h (3 lines)
M		kmymoney/wizards/newuserwizard/CMakeLists.txt (3 lines)
M		kmymoney/wizards/newuserwizard/kcurrencypage.cpp (2 lines)
D	M	kmymoney/wizards/newuserwizard/kfilepage.h (58 lines)
D	M	kmymoney/wizards/newuserwizard/kfilepage.cpp (105 lines)
D	M	kmymoney/wizards/newuserwizard/kfilepage.ui (92 lines)
D	M	kmymoney/wizards/newuserwizard/kfilepage_p.h (57 lines)
M		kmymoney/wizards/newuserwizard/knewuserwizard.h (7 lines)
M		kmymoney/wizards/newuserwizard/knewuserwizard.cpp (10 lines)
M		kmymoney/wizards/newuserwizard/knewuserwizard_p.h (5 lines)
M		kmymoney/wizards/newuserwizard/kpreferencepage.h (2 lines)
M		kmymoney/wizards/newuserwizard/kpreferencepage.cpp (7 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	35774			Jun 7 2018, 3:17 PM	★	★
Diff 2	35870			Jun 9 2018, 5:51 AM	★	★
Diff 3	35878			Jun 9 2018, 9:12 AM	★	★
Diff 4	35888			Jun 9 2018, 1:11 PM	★	★
Diff 5	35892			Jun 9 2018, 3:44 PM	★	★
Diff 6	35929			Jun 10 2018, 5:15 AM	★	★
Diff 7	35930			Jun 10 2018, 5:20 AM	★	★
Diff 8	35947			Jun 10 2018, 12:39 PM	★	★
Diff 9	35953	7a3aba0	R261:bfa516d0914387be0910f249ade10a094babfd59	Jun 10 2018, 3:23 PM	★	★

Refactor file menuClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 35953

kmymoney/dialogs/CMakeLists.txt

kmymoney/dialogs/kgpgkeyselectiondlg.h

kmymoney/dialogs/kgpgkeyselectiondlg.cpp

kmymoney/dialogs/kgpgkeyselectiondlg.ui

kmymoney/dialogs/ksaveasquestion.h

kmymoney/dialogs/ksaveasquestion.cpp

kmymoney/dialogs/ksaveasquestion.ui

kmymoney/kmymoney.h

kmymoney/kmymoney.cpp

kmymoney/kmymoneyenums.h

kmymoney/main.cpp

kmymoney/mymoney/onlinejobadministration.cpp

kmymoney/plugins/appinterface.h

kmymoney/plugins/csv/export/csvexporter.cpp

kmymoney/plugins/csv/import/csvimporter.cpp

kmymoney/plugins/gnc/import/CMakeLists.txt

kmymoney/plugins/gnc/import/gncimporter.h

kmymoney/plugins/gnc/import/gncimporter.cpp

kmymoney/plugins/gnc/import/mymoneygncreader.h

kmymoney/plugins/icalendar/export/icalendarexporter.cpp

kmymoney/plugins/icalendar/export/schedulestoicalendar.cpp

kmymoney/plugins/interfaces/kmmappinterface.h

kmymoney/plugins/interfaces/kmmappinterface.cpp

kmymoney/plugins/kmymoneyplugin.h

kmymoney/plugins/kmymoneyplugin.cpp

kmymoney/plugins/ofx/import/ofximporter.cpp

kmymoney/plugins/qif/export/qifexporter.cpp

kmymoney/plugins/qif/import/qifimporter.cpp

kmymoney/plugins/sql/sqlstorage.h

kmymoney/plugins/sql/sqlstorage.cpp

kmymoney/plugins/xml/CMakeLists.txt

kmymoney/plugins/xml/mymoneystoragexml.h

kmymoney/plugins/xml/xmlstorage.h

kmymoney/plugins/xml/xmlstorage.cpp

kmymoney/plugins/xml/xmlstorage.rc

kmymoney/views/kaccountsview.cpp

kmymoney/views/kgloballedgerview.cpp

kmymoney/views/khomeview.cpp

kmymoney/views/khomeview_p.h

kmymoney/views/kmymoneyview.h

kmymoney/views/kmymoneyview.cpp

kmymoney/views/viewenums.h

kmymoney/wizards/newuserwizard/CMakeLists.txt

kmymoney/wizards/newuserwizard/kcurrencypage.cpp

kmymoney/wizards/newuserwizard/kfilepage.h

kmymoney/wizards/newuserwizard/kfilepage.cpp

kmymoney/wizards/newuserwizard/kfilepage.ui

kmymoney/wizards/newuserwizard/kfilepage_p.h

kmymoney/wizards/newuserwizard/knewuserwizard.h

kmymoney/wizards/newuserwizard/knewuserwizard.cpp

kmymoney/wizards/newuserwizard/knewuserwizard_p.h

kmymoney/wizards/newuserwizard/kpreferencepage.h

kmymoney/wizards/newuserwizard/kpreferencepage.cpp

Refactor file menu
ClosedPublic
Actions

Revision Contents
Changeset List