Differential D13831

Move XML read methods to storage plugin
ClosedPublic
Actions

Authored by wojnilowicz on Jul 1 2018, 4:37 PM.

Details

Reviewers

tbaumgart

Group Reviewers

KMyMoney

Commits

R261:e4d384d34a91: Move XML read methods to storage plugin

Summary

This is a step in XML encapsulation direction. Because overall it's a big undertaking, only read methods from mymoneystoragexml has been moved. Save methods will follow later. Because it's an error prone undertaking, old read methods have been left untouched for a quick and transparent testing. MyMoneyObjects are loaded in mymoneystoragexml as follows:
*1) using old way (constructor accepting QDomElement),

using new way (static read methods in mymoneystoragexml),

*3) returned object from old and new way are compared and inequality is signaled in terminal,
*4) returned object from old way is discarded and object from new way is going further,

*only for testing, will be removed with underlying code in the accepted patch

Test Plan

All tests pass.

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.Jul 1 2018, 4:37 PM

wojnilowicz created this revision.

Did you ever thought about using Q_ENUM? See https://woboq.com/blog/q_enum.html for some details. It looks cleaner if it can be used.

Besides that, I needed the following patch to compile the change:

diff --git a/kmymoney/plugins/views/onlinejoboutbox/onlinejobmodel.cpp b/kmymoney/plugins/views/onlinejoboutbox/onlinejobmodel.cpp
index 69a964a7..c8f6d337 100644
--- a/kmymoney/plugins/views/onlinejoboutbox/onlinejobmodel.cpp
+++ b/kmymoney/plugins/views/onlinejoboutbox/onlinejobmodel.cpp
@@ -144,11 +144,11 @@ QVariant onlineJobModel::data(const QModelIndex & index, int role) const
         return Icons::get(Icon::TaskOngoing);
 
       switch (job.bankAnswerState()) {
-        case onlineJob::acceptedByBank: return Icons::get(Icon::TaskComplete);
-        case onlineJob::sendingError:
-        case onlineJob::abortedByUser:
-        case onlineJob::rejectedByBank: return Icons::get(Icon::TaskReject);
-        case onlineJob::noBankAnswer: break;
+        case eMyMoney::OnlineJob::sendingState::acceptedByBank: return Icons::get(Icon::TaskComplete);
+        case eMyMoney::OnlineJob::sendingState::sendingError:
+        case eMyMoney::OnlineJob::sendingState::abortedByUser:
+        case eMyMoney::OnlineJob::sendingState::rejectedByBank: return Icons::get(Icon::TaskReject);
+        case eMyMoney::OnlineJob::sendingState::noBankAnswer: break;
       }
       if (job.sendDate().isValid()) {
         return Icons::get(Icon::TaskAccepted);
@@ -160,11 +160,11 @@ QVariant onlineJobModel::data(const QModelIndex & index, int role) const
         return i18n("Job is being processed at the moment.");
 
       switch (job.bankAnswerState()) {
-        case onlineJob::acceptedByBank: return i18nc("Arg 1 is a date/time", "This job was accepted by the bank on %1.", job.bankAnswerDate().toString(Qt::DefaultLocaleShortDate));
-        case onlineJob::sendingError: return i18nc("Arg 1 is a date/time", "Sending this job failed (tried on %1).", job.sendDate().toString(Qt::DefaultLocaleShortDate));
-        case onlineJob::abortedByUser: return i18n("Sending this job was manually aborted.");
-        case onlineJob::rejectedByBank: return i18nc("Arg 1 is a date/time", "The bank rejected this job on %1.", job.bankAnswerDate().toString(Qt::DefaultLocaleShortDate));
-        case onlineJob::noBankAnswer:
+        case eMyMoney::OnlineJob::sendingState::acceptedByBank: return i18nc("Arg 1 is a date/time", "This job was accepted by the bank on %1.", job.bankAnswerDate().toString(Qt::DefaultLocaleShortDate));
+        case eMyMoney::OnlineJob::sendingState::sendingError: return i18nc("Arg 1 is a date/time", "Sending this job failed (tried on %1).", job.sendDate().toString(Qt::DefaultLocaleShortDate));
+        case eMyMoney::OnlineJob::sendingState::abortedByUser: return i18n("Sending this job was manually aborted.");
+        case eMyMoney::OnlineJob::sendingState::rejectedByBank: return i18nc("Arg 1 is a date/time", "The bank rejected this job on %1.", job.bankAnswerDate().toString(Qt::DefaultLocaleShortDate));
+        case eMyMoney::OnlineJob::sendingState::noBankAnswer:
           if (job.sendDate().isValid())
             return i18nc("Arg 1 is a date/time", "The bank accepted this job on %1.", job.sendDate().toString(Qt::DefaultLocaleShortDate));
           else if (!job.isValid())

This revision now requires changes to proceed.Jul 10 2018, 6:18 PM

In D13831#290049, @tbaumgart wrote:

Did you ever thought about using Q_ENUM? See https://woboq.com/blog/q_enum.html for some details. It looks cleaner if it can be used.

I believe it needs a class with Q_OBJECT and probably would require some more includes in headers, so I don't want to switch to it now.

In D13831#290468, @wojnilowicz wrote:

In D13831#290049, @tbaumgart wrote:

Did you ever thought about using Q_ENUM? See https://woboq.com/blog/q_enum.html for some details. It looks cleaner if it can be used.

I believe it needs a class with Q_OBJECT and probably would require some more includes in headers, so I don't want to switch to it now.

Ah, that could well be, since it relies on the QMetaObject stuff.

In case I load a file with this patch, the Update all accounts button is grayed out. This works OK on master with the same file. Can you please check all actions once you have fixed this?

This revision now requires changes to proceed.Jul 11 2018, 3:29 PM

In D13831#290516, @tbaumgart wrote:

In D13831#290468, @wojnilowicz wrote:

In D13831#290049, @tbaumgart wrote:

Did you ever thought about using Q_ENUM? See https://woboq.com/blog/q_enum.html for some details. It looks cleaner if it can be used.

I believe it needs a class with Q_OBJECT and probably would require some more includes in headers, so I don't want to switch to it now.

Ah, that could well be, since it relies on the QMetaObject stuff.

In case I load a file with this patch, the Update all accounts button is grayed out. This works OK on master with the same file. Can you please check all actions once you have fixed this?

I cannot reproduce your issue. Update all accounts button isn't grayed out in my case. Could you provide a test case?

I used File/Dump memory with master and D13831 and here is the difference after loading the file

Since the provider is empty because it is missing in version D13831, the return value of MyMoneyAccount::hasOnlineMapping() in KMyMoneyApp::Private::canUpdateAllAccounts() returns false for all accounts, the return value of canUpdateAllAccounts() is also false.

Does this make any difference?

Yes it does.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 14 2018, 12:00 PM

Closed by commit R261:e4d384d34a91: Move XML read methods to storage plugin (authored by wojnilowicz). · Explain Why

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

		Path
M		kmymoney/mymoney/mymoneyaccount.h (9 lines)
M		kmymoney/mymoney/mymoneyaccount.cpp (83 lines)
M		kmymoney/mymoney/mymoneycostcenter.h (9 lines)
M		kmymoney/mymoney/mymoneycostcenter.cpp (10 lines)
M		kmymoney/mymoney/mymoneyenums.h (2 lines)
M		kmymoney/mymoney/mymoneyinstitution.h (9 lines)
M		kmymoney/mymoney/mymoneyinstitution.cpp (33 lines)
M		kmymoney/mymoney/mymoneypayee.h (10 lines)
M		kmymoney/mymoney/mymoneypayee.cpp (42 lines)
M		kmymoney/mymoney/mymoneypayeeidentifiercontainer.h (2 lines)
M		kmymoney/mymoney/mymoneyreport.h (7 lines)
M		kmymoney/mymoney/mymoneyreport.cpp (15 lines)
M		kmymoney/mymoney/mymoneyschedule.h (26 lines)
M		kmymoney/mymoney/mymoneyschedule.cpp (69 lines)
M		kmymoney/mymoney/mymoneysecurity.h (2 lines)
M		kmymoney/mymoney/mymoneysecurity.cpp (35 lines)
M		kmymoney/mymoney/mymoneytag.h (9 lines)
M		kmymoney/mymoney/mymoneytag.cpp (23 lines)
M		kmymoney/mymoney/onlinejob.h (22 lines)
M		kmymoney/mymoney/onlinejob.cpp (52 lines)
M		kmymoney/mymoney/onlinejob_p.h (5 lines)
M		kmymoney/mymoney/onlinejobadministration.h (12 lines)
M		kmymoney/mymoney/tests/mymoneyaccount-test.h (3 lines)
M		kmymoney/mymoney/tests/mymoneyaccount-test.cpp (100 lines)
M		kmymoney/mymoney/tests/mymoneybudget-test.h (2 lines)
M		kmymoney/mymoney/tests/mymoneybudget-test.cpp (20 lines)
M		kmymoney/mymoney/tests/mymoneyinstitution-test.h (3 lines)
M		kmymoney/mymoney/tests/mymoneyinstitution-test.cpp (87 lines)
M		kmymoney/mymoney/tests/mymoneypayee-test.h (3 lines)
M		kmymoney/mymoney/tests/mymoneypayee-test.cpp (58 lines)
M		kmymoney/mymoney/tests/mymoneyreport-test.h (2 lines)
M		kmymoney/mymoney/tests/mymoneyreport-test.cpp (20 lines)
P		kmymoney/mymoney/tests/mymoneyschedule-test.h (10 lines) Copied to kmymoney/plugins/xml/tests/mymoneyxmlcontenthandler-test.h
M		kmymoney/mymoney/tests/mymoneyschedule-test.cpp (716 lines)
M		kmymoney/mymoney/tests/mymoneysecurity-test.h (1 line)
M		kmymoney/mymoney/tests/mymoneysecurity-test.cpp (10 lines)
D	M	kmymoney/mymoney/tests/mymoneytag-test.h (31 lines)
D	M	kmymoney/mymoney/tests/mymoneytag-test.cpp (55 lines)
M		kmymoney/mymoney/tests/mymoneytransaction-test.h (4 lines)
M		kmymoney/mymoney/tests/mymoneytransaction-test.cpp (138 lines)
M		kmymoney/mymoney/tests/onlinejob-test.h (2 lines)
M		kmymoney/mymoney/tests/onlinejob-test.cpp (24 lines)
M		kmymoney/plugins/kbanking/kbanking.cpp (4 lines)
M		kmymoney/plugins/sql/mymoneystoragesql.cpp (12 lines)
M		kmymoney/plugins/sql/mymoneystoragesql_p.h (10 lines)
M		kmymoney/plugins/views/onlinejoboutbox/onlinejobmodel.cpp (20 lines)
M		kmymoney/plugins/xml/CMakeLists.txt (6 lines)
M		kmymoney/plugins/xml/mymoneystoragenames.h (393 lines)
M		kmymoney/plugins/xml/mymoneystoragenames.cpp (868 lines)
M		kmymoney/plugins/xml/mymoneystoragexml.h (7 lines)
M		kmymoney/plugins/xml/mymoneystoragexml.cpp (996 lines)
A	M	kmymoney/plugins/xml/tests/CMakeLists.txt (21 lines)
A	M	kmymoney/plugins/xml/tests/mymoneystoragenames-test.h (51 lines)
A	M	kmymoney/plugins/xml/tests/mymoneystoragenames-test.cpp (224 lines)
P		kmymoney/plugins/xml/tests/mymoneyxmlcontenthandler-test.h (52 lines) Copied from kmymoney/mymoney/tests/mymoneyschedule-test.h
A	M	kmymoney/plugins/xml/tests/mymoneyxmlcontenthandler-test.cpp (1500 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	37017			Jul 1 2018, 4:20 PM	★	★
Diff 2	37566			Jul 11 2018, 1:54 PM	★	★
Diff 3	37707			Jul 13 2018, 4:46 PM	★	★
Diff 4	37740	fcb1549	R261:e4d384d34a91eb6024800ca13ecb1696af72cd96	Jul 14 2018, 11:59 AM	★	★

Move XML read methods to storage pluginClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 37740

kmymoney/mymoney/mymoneyaccount.h

kmymoney/mymoney/mymoneyaccount.cpp

kmymoney/mymoney/mymoneycostcenter.h

kmymoney/mymoney/mymoneycostcenter.cpp

kmymoney/mymoney/mymoneyenums.h

kmymoney/mymoney/mymoneyinstitution.h

kmymoney/mymoney/mymoneyinstitution.cpp

kmymoney/mymoney/mymoneypayee.h

kmymoney/mymoney/mymoneypayee.cpp

kmymoney/mymoney/mymoneypayeeidentifiercontainer.h

kmymoney/mymoney/mymoneyreport.h

kmymoney/mymoney/mymoneyreport.cpp

kmymoney/mymoney/mymoneyschedule.h

kmymoney/mymoney/mymoneyschedule.cpp

kmymoney/mymoney/mymoneysecurity.h

kmymoney/mymoney/mymoneysecurity.cpp

kmymoney/mymoney/mymoneytag.h

kmymoney/mymoney/mymoneytag.cpp

kmymoney/mymoney/onlinejob.h

kmymoney/mymoney/onlinejob.cpp

kmymoney/mymoney/onlinejob_p.h

kmymoney/mymoney/onlinejobadministration.h

kmymoney/mymoney/tests/mymoneyaccount-test.h

kmymoney/mymoney/tests/mymoneyaccount-test.cpp

kmymoney/mymoney/tests/mymoneybudget-test.h

kmymoney/mymoney/tests/mymoneybudget-test.cpp

kmymoney/mymoney/tests/mymoneyinstitution-test.h

kmymoney/mymoney/tests/mymoneyinstitution-test.cpp

kmymoney/mymoney/tests/mymoneypayee-test.h

kmymoney/mymoney/tests/mymoneypayee-test.cpp

kmymoney/mymoney/tests/mymoneyreport-test.h

kmymoney/mymoney/tests/mymoneyreport-test.cpp

kmymoney/mymoney/tests/mymoneyschedule-test.h

kmymoney/mymoney/tests/mymoneyschedule-test.cpp

kmymoney/mymoney/tests/mymoneysecurity-test.h

kmymoney/mymoney/tests/mymoneysecurity-test.cpp

kmymoney/mymoney/tests/mymoneytag-test.h

kmymoney/mymoney/tests/mymoneytag-test.cpp

kmymoney/mymoney/tests/mymoneytransaction-test.h

kmymoney/mymoney/tests/mymoneytransaction-test.cpp

kmymoney/mymoney/tests/onlinejob-test.h

kmymoney/mymoney/tests/onlinejob-test.cpp

kmymoney/plugins/kbanking/kbanking.cpp

kmymoney/plugins/sql/mymoneystoragesql.cpp

kmymoney/plugins/sql/mymoneystoragesql_p.h

kmymoney/plugins/views/onlinejoboutbox/onlinejobmodel.cpp

kmymoney/plugins/xml/CMakeLists.txt

kmymoney/plugins/xml/mymoneystoragenames.h

kmymoney/plugins/xml/mymoneystoragenames.cpp

kmymoney/plugins/xml/mymoneystoragexml.h

kmymoney/plugins/xml/mymoneystoragexml.cpp

kmymoney/plugins/xml/tests/CMakeLists.txt

kmymoney/plugins/xml/tests/mymoneystoragenames-test.h

kmymoney/plugins/xml/tests/mymoneystoragenames-test.cpp

kmymoney/plugins/xml/tests/mymoneyxmlcontenthandler-test.h

kmymoney/plugins/xml/tests/mymoneyxmlcontenthandler-test.cpp

Move XML read methods to storage plugin
ClosedPublic
Actions

Revision Contents
Changeset List