Pluginize KOnlineJobOutbox
ClosedPublic

Authored by wojnilowicz on Mar 31 2018, 5:40 PM.

Details

Summary

This patch makes online job outbox view optional instead of mandatory. User can switch on and off this view and have more space on views pane.
Developers will benefit by saving on build time of this plugin.

It compiles KMyMoneyAccountCombo and links to static models library just like QIF exporter does. Something has to be done with this, but for now switchable outbox is enough feature to be committed.

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.Mar 31 2018, 5:40 PM
wojnilowicz created this revision.
tbaumgart requested changes to this revision.Apr 1 2018, 2:11 PM

Existing jobs (persistent in the file) are not shown in the view after load of file and selecting the view. In case you need help to investigate please let me know.

This revision now requires changes to proceed.Apr 1 2018, 2:11 PM

I analyzed this a bit. The problem is that there are two different objects of type Models::instance()->onlineJobsModel() since the libmodels library is static. One lives with the kmymoney object and contains the data, the other one lives with the now separated view and is empty. The models library therefor needs to be converted into a shared object and then things should work (like with the settings library). @wojnilowicz: can you take care of the conversion as part of this patch?

I analyzed this a bit. The problem is that there are two different objects of type Models::instance()->onlineJobsModel() since the libmodels library is static. One lives with the kmymoney object and contains the data, the other one lives with the now separated view and is empty. The models library therefor needs to be converted into a shared object and then things should work (like with the settings library). @wojnilowicz: can you take care of the conversion as part of this patch?

Yes, your findings are right. I'm tight on time lately, but I will do that. You can make the conversion yourself in the meantime if you'd like.

I want to raise a usability concern. If the user is using the online banking and this page gets disabled (e.g. accidentally, packaging error, some other error) KMyMoney still offers to queue transactions. However, without this plugin the transaction seems to be lost. Queued transactions are also kept during restarts. This could lead to further trouble if the queue is sent after the user forgot about the (hidden) transaction(s).

Independent from this concern I support the idea to isolate the different views better. Maybe Qt's static plugins are an alternative. An easier way would be that this view just gets its own folder with minimal interaction with the rest. Of course this does not change the compile time.

I want to raise a usability concern. If the user is using the online banking and this page gets disabled (e.g. accidentally, packaging error, some other error) KMyMoney still offers to queue transactions. However, without this plugin the transaction seems to be lost. Queued transactions are also kept during restarts. This could lead to further trouble if the queue is sent after the user forgot about the (hidden) transaction(s).

I don't know this problem, because I don't use this view. I think that packaging error is not our business. I think the view should contain more of online job bits but for know it's all that's easy to separate from the rest.

Independent from this concern I support the idea to isolate the different views better. Maybe Qt's static plugins are an alternative. An easier way would be that this view just gets its own folder with minimal interaction with the rest. Of course this does not change the compile time.

What's the advantage of static plugin?

I don't know this problem, because I don't use this view. I think that packaging error is not our business. I think the view should contain more of online job bits but for know it's all that's easy to separate from the rest.

Which bits do you mean?

What's the advantage of static plugin?

It is always there because it is linked into the app. At the same time it offers the same separation of code like plugins (more details are in the Qt Plugin docu).

christiand added inline comments.Apr 8 2018, 9:07 AM
kmymoney/plugins/views/onlinejoboutbox/konlinejoboutboxview.h
59

d-ptr has no advantages in a plugin. ABI compatibility in this sense is not needed.

I don't know this problem, because I don't use this view. I think that packaging error is not our business. I think the view should contain more of online job bits but for know it's all that's easy to separate from the rest.

Which bits do you mean?

"kmymoney/plugins/onlinetasks"

What's the advantage of static plugin?

It is always there because it is linked into the app. At the same time it offers the same separation of code like plugins (more details are in the Qt Plugin docu).

I don't like it. Dynamic plugins offer me separation of code, build and run speed.

wojnilowicz added inline comments.Apr 8 2018, 6:46 PM
kmymoney/plugins/views/onlinejoboutbox/konlinejoboutboxview.h
59

It's not about ABI. It's about krazy2 and build time.

tbaumgart added inline comments.Apr 9 2018, 7:21 PM
kmymoney/plugins/views/onlinejoboutbox/konlinejoboutboxview.h
59

krazy2 does not count here. It might show false positives and has means to control them. Its purpose is to check for framework stuff, which might not be needed for all of KMyMoney.

I don't like it. Dynamic plugins offer me separation of code, build and run speed.

Static plugins offer the exact same amount of code separation as dynamic plugins. They are probably even faster at runtime and I am sure they do not change build time notably.

I don't like it. Dynamic plugins offer me separation of code, build and run speed.

Static plugins offer the exact same amount of code separation as dynamic plugins. They are probably even faster at runtime and I am sure they do not change build time notably.

So when should we use dynamic plugins if static plugins have all the advantages?

kmymoney/plugins/views/onlinejoboutbox/konlinejoboutboxview.h
59

If krazy2 does not count, then maybe let's get rid of it. If it's a rule that it shows false positives then it's not helpful for us.

onlinejobmodel is used only by online job outbox plugin (and was moved there), so sharing of models can be avoided for now. Issue with jobs not appearing in the view was caused by not loaded view's model. Loading and unloading of the model is managed by KMyMoneyView.

Please test and report.

tbaumgart requested changes to this revision.May 3 2018, 8:30 AM

The problem regarding the duplicate existence of the model seem to be solved. The following now does not work anymore. I have no idea how hard it would be to fix it.

Use case: I want to create a new online transaction for a mapped account

  • Open the ledger of a mapped account
  • Press the New credit transfer tool-button or select Account/New credit transfer
  • The account box is filled with all mapped accounts and the one shown in the ledger is pre-selected

That last bit is broken. The account box is empty and one cannot create the online transfer at all.

Also using this route

  • Select outbox view
  • Press New credit transfer button on the right
  • Select account from account box

does not work, as the account selection is empty. To me, this looks like that we now have a second accountsModel() floating around.

This revision now requires changes to proceed.May 3 2018, 8:30 AM

Pushed, because next patch solved remaining issues.

This revision was not accepted when it landed; it landed in state Needs Revision.May 5 2018, 9:30 AM
This revision was automatically updated to reflect the committed changes.