[Kmymoney-devel] Review Request 122364: CSV Importer - Fix display on high-definition displays. Fix interaction between QTable and QWizard in same window.

Sun Feb 22 13:59:26 UTC 2015

> On Feb. 12, 2015, 10:24 nachm., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment346>
> >
> >     This is more a general annotation:
> >     
> >     You are giving the child access to it's parent. This violates the object-oriented principle of modularity. Thus maintaince is harder and the risk of bugs is higher.
> 
> Allan Anderson wrote:
>     Oh dear!  I've been using this since day one of the plugin, and now over a thousand times, for access from one class to common code in another class.  I thought I'd seen this method in use in other places in KMM (which isn't to say it's right.).
>     What to do?  I probably need to do some studying.
>     Hopefully, it shouldn't delay this patch, as that code predates it.

> Hopefully, it shouldn't delay this patch, as that code predates it.

No, it should not delay the patch! I just wanted to mention it.

> On Feb. 12, 2015, 10:24 nachm., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment347>
> >
> >     This should be done in a more modular fashion (general annotation).
> 
> Allan Anderson wrote:
>     Sorry, can you explain.

Imagine you want to change or work with the object pointed to by ```m_pageCompletion```. Then you have to look at ```m_wiz``` as well – from experience I know this is forgotten easily. Also it makes it hard to understand the code. The chance a new developer will give up after a couple of these constructs is high.

This line is in a method called ```init()```. So I assume it is called once to initialize the widget. So I would move the initializing into the class of ```m_pageCompletion```. Actually you can set this simply in Qt Designer and do not need any C++ code.

> On Feb. 12, 2015, 10:24 nachm., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment348>
> >
> >     This part can be removed, the flag is set and removed again. Also you call show() twice.
> 
> Allan Anderson wrote:
>     As above.

Without seeing the code: I think, you should just drop the part where the flag is removed again and the second ```show()```.

> On Feb. 12, 2015, 10:24 nachm., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment349>
> >
> >     You are calling setCurrentIndex() later again. So these lines are not needed as their change is overwritten anyway (or do you need the change signal to get emitted?).
> 
> Allan Anderson wrote:
>     If the second setCurrentIndex() doesn't change the setting, there will be no connect, which is needed to allow other settings
>     to be checked for conflict, etc..  So, yes, the first is to ensure the second does cause a connect.

Do you mean a second "connect" or "ensure it causes an emit of the signal"?

> On Feb. 12, 2015, 10:24 nachm., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment350>
> >
> >     Why do you disconnect and reconnect later? Also this connection should be done in CSVWizard.
> 
> Allan Anderson wrote:
>     This goes back to when I was originally working on the plugins, and if I remember, just above these re-connects, I populate all the comboboxes with their items, which was causing multiple connects and upsetting the indexes.  There is a brief note there.
>     Agreed about moving them, but do we want more code at the moment?  Certainly, I'm happy to move them.  They too predate this patch.

You can prevent multiple connections with ```connect( sender, signal, receiver, slot, Qt::UniqueConnection)```.

"Agreed about moving them, but do we want more code at the moment?" - I agree.

> On Feb. 12, 2015, 10:24 nachm., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment351>
> >
> >     ```if (m_wiz->m_pageIntro->isVisible() || m_wiz->m_pageLinesDate->isVisible())```
> 
> Allan Anderson wrote:
>     Done.  I have been trying to watch for that.

It is nice of you to change this :) I was not expecting you to do so, I just wanted to mention it for future development.

> On Feb. 12, 2015, 10:24 nachm., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment352>
> >
> >     Usually you want member variables to be private.
> 
> Allan Anderson wrote:
>     A relic from my earlier days, I'm afraid.  I'll start to look at these as I get the chance.

Just do it if it has a benefit for you. Such changes usually take a lot of time without visible advantages.

> On Feb. 12, 2015, 10:24 nachm., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment353>
> >
> >     I am not sure here; do these values depend on a user selection? If so it is usualy better to "calculate" them on request. Having a copy quickly leads to inconsistent data.
> 
> Allan Anderson wrote:
>     Yes, for the Banking and Investment wizards, etc, a group of similar, required settings are tested to ensure all required (and optional alternatives) fields have been selected.  So, you're saying not to keep these results, but to perform the tests as needed?  I was using them as a cache, to reduce the library overheads.

If the value is not requested each millisecond the overhead won't be notable (under certain circumstances it can be even faster not to use this cache).

> On Feb. 12, 2015, 10:24 nachm., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment354>
> >
> >     Library includes should be above project includes.
> 
> Allan Anderson wrote:
>     I think sometimes I've done that, without knowing it was a requirement/style issue.  What about the header #include in a cpp file?

You are right: the header "xyz.h" should get included in "xyz.cpp" first.

> On Feb. 12, 2015, 10:24 nachm., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment355>
> >
> >     This method looks very similar to the following ```…ColumnSelected()``` methods. They could be compacted with some template magic.
> 
> Allan Anderson wrote:
>     Hmmm. I'll have a play with that later.

Can I offer my help?

> On Feb. 12, 2015, 10:24 nachm., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment356>
> >
> >     Can this ```<center>``` work? There is no new paragraph or line break.
> 
> Allan Anderson wrote:
>     Yes, it does work the way I expected.  The second sentence is centered.

Thanks, this is now to me :)

- Christian

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122364/#review75942
-----------------------------------------------------------

On Feb. 3, 2015, 1:27 vorm., Allan Anderson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122364/
> -----------------------------------------------------------
> 
> (Updated Feb. 3, 2015, 1:27 vorm.)
> 
> 
> Review request for KMymoney.
> 
> 
> Bugs: 340656
>     http://bugs.kde.org/show_bug.cgi?id=340656
> 
> 
> Repository: kmymoney
> 
> 
> Description
> -------
> 
> Fix display on high-definition monitors. Fix interaction between the import preview table widget and the parameter entry wizard, caused by them both resizing dynamically within the same window. This was achieved by creating a new class and UI for the wizard and transferring most of the existing relevant code out of the large csvdialog.cpp file into the new class and file.  Unfortunately, because 11 UIs are affected and the transfer of existing logic into a new class, the patch is quite large, although there is little new code involved.
> 
> 
> Diffs
> -----
> 
>   kmymoney/plugins/csvimport/csvwizard.h PRE-CREATION 
>   kmymoney/plugins/csvimport/csvwizard.cpp PRE-CREATION 
>   kmymoney/plugins/csvimport/csvwizard.ui PRE-CREATION 
>   kmymoney/plugins/csvimport/introwizardpage.ui 2bf93f0 
>   kmymoney/plugins/csvimport/investmentdlg.cpp 5e7ce06 
>   kmymoney/plugins/csvimport/investmentwizardpage.ui d3f2857 
>   kmymoney/plugins/csvimport/investprocessing.h 3c08dee 
>   kmymoney/plugins/csvimport/investprocessing.cpp 2b6b2d1 
>   kmymoney/plugins/csvimport/lines-datewizardpage.ui 01d7253 
>   kmymoney/plugins/csvimport/redefinedlgdecl.ui 26d8b62 
>   kmymoney/plugins/csvimport/separatorwizardpage.ui 21136f2 
>   kmymoney/plugins/csvimport/CMakeLists.txt 36e5afc 
>   kmymoney/plugins/csvimport/bankingwizardpage.ui 9e1b5cb 
>   kmymoney/plugins/csvimport/completionwizardpage.ui ce61e89 
>   kmymoney/plugins/csvimport/csvdialog.h 780329d 
>   kmymoney/plugins/csvimport/csvdialog.cpp b986317 
>   kmymoney/plugins/csvimport/csvdialog.ui 166b04a 
> 
> Diff: https://git.reviewboard.kde.org/r/122364/diff/
> 
> 
> Testing
> -------
> 
> Tested on 96 DPI and 160 DPI.  Also, the OP of the bug has tested the patch and confirms its performance.
> There is one remaining UI not yet included.  I've completed its testing, but as it's a little-used window, I've held it back to avoid making this patch even larger.
> I'm unable to test on Windows.
> 
> 
> File Attachments
> ----------------
> 
> Updated patch
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/02/03/6e4dad98-936c-4ca0-8804-c4abb9b51438__0002-BUG-340656.patch
> 
> 
> Thanks,
> 
> Allan Anderson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kmymoney-devel/attachments/20150222/0b1c95e2/attachment-0001.html>