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

Tue Feb 17 22:04:01 UTC 2015

> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > That is a really huge diff. Here are some things I noticed while skimed through it.
> > 
> > It seems you reduced the code; very good!
> 
> Christian David wrote:
>     It compiles on my system.

Sorry for the delayed reply.  I encountered a problem during testing, when I used a particular test file.
I spent some time trying to find the root cause, without success, but I could see what was going wrong and
had to do some rewriting.

Thanks also, for your time and expertise.

> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment316>
> >
> >     I do not understand this: "…if only one value column…"

Different banks format their files in different ways.  One such difference is in how they display amounts.
Some have just a single column/field to show the amount with its sign.  Others have a debit column and a credit column.
Users will be aware of which they need to use, as they have their file to show them.
The string is a hint to click the Amount radio button if they have just one such field in their file.

> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment317>
> >
> >     I do not understand this: "…if only one value column…"

As above, but for the 'What's this'

> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment318>
> >
> >     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.

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.

> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment319>
> >
> >     Is this used anywhere?

This code was removed fairly recently, following another bug.  I put it back in (temporarily, and perhaps mistakenly), in expectation that it would cause a conflict and need to be removed on update/merging.

> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment320>
> >
> >     Is this used anywhere?

As above.

> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment321>
> >
> >     This should be done in a more modular fashion (general annotation).

Sorry, can you explain.

> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment322>
> >
> >     ```Qt::WindowFlags eFlags = windowFlags();``` (space removed)

I know it looks odd, but I copied this from a google search.  If I remember, the point was to avoid the window in question staying on top.
Perhaps I misunderstood?  I didn't actually try without it.

> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment323>
> >
> >     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?).

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.

> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment324>
> >
> >     Why do you need the ui to change?

As above, really.

> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment325>
> >
> >     Hehe, such annoying calculations appear if you do not trust the library ;)

I took a lot of time and effort on this, as I wanted the tables to be the exact size needed, and I
think it's frowned uppon to use any apparently arbitrary values, so I showed the derivation.  I tried to get the window to shrink
in line with its content, but wasn't really successful, without finishing up with them not resizeable.

> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment326>
> >
> >     ```+ 2 + 1``` ?

Ah!!!  Drat.  I had to do some tweaking to get the scrollbars not to have minor gaps after scrolling to the foot.
Fixed.

> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment327>
> >
> >     You have the method ```clearColumnNumbers()``` for this already.

This one removes all the items, whereas clearColumnNumbers() just clears the numbers but leaves the items in place.

> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment328>
> >
> >     Here ```show()``` is called twice and setting the flag has no effect as it is removed again.

As for line 1114 above.

> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment329>
> >
> >     ```if (m_wiz->m_pageIntro->isVisible() || m_wiz->m_pageLinesDate->isVisible())```

Done.  I have been trying to watch for that.

> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment330>
> >
> >     Same as before.

For me too.

> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment331>
> >
> >     Why is this disconnected? It is connected again later. This may save some of your time: http://qt-project.org/doc/qt-4.8/qt.html#ConnectionType-enum

Oh, that part of the code is redundant now, as a while back I made a change, determining the field delimiter actually present in the file.
I need to do a bit more testing, to see if any part is needed, but I suspect not.  I'll need to check also in the investment code.
The patch grows some more!
I also need to digest the content of that link, as I've never used the ConnectionType.

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

As above.

> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment333>
> >
> >     Why do you disconnect and reconnect later? Also this connection should be done in CSVWizard.

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.

> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment334>
> >
> >     "reset this combobox" - the name ```resetComboBox()``` is descriptive already.
> >     
> >     I noticed this at several places: You are using a lot of whitespace. This makes it harder for me to read the code.

Removed.  It was just there to show quickly what happened where.
OK, sorry about the white space.  I wanted to separate it clearly from the code.
Again, that predates, so I'll do it gradually.

> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment335>
> >
> >     ```m_wiz->ui->label_intro->setText(QLatin1String("<b>") + str + QLatin1String("</b>"));```

A while back, I did get to know about this, but there may be one or two still lurking.

> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment336>
> >
> >     This method should be ```const```.

Also, several others changed, too.

> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment338>
> >
> >     Usually you want member variables to be private.

A relic from my earlier days, I'm afraid.  I'll start to look at these as I get the chance.

> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment339>
> >
> >     ```QStringList    m_columnTypeList;  //  holds field types - date, payee, etc.```
> >     
> >     to
> >     
> >     ```/** holds field types - date, payee, etc. */
> >     QStringList    m_columnTypeList;```
> >     
> >     then doxygen can help you

Oh, I hadn't come across that.  If I've used it, it's by accident.

> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment340>
> >
> >     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.

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.

> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment341>
> >
> >     Library includes should be above project includes.

I think sometimes I've done that, without knowing it was a requirement/style issue.  What about the header #include in a cpp file?

> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment342>
> >
> >     else if

Of course.

> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment343>
> >
> >     else if

etc. for the others.

> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment344>
> >
> >     This method looks very similar to the following ```…ColumnSelected()``` methods. They could be compacted with some template magic.

Hmmm. I'll have a play with that later.

> On Feb. 12, 2015, 9:24 p.m., Christian David wrote:
> > File Attachment: Updated patch - 0002-BUG-340656.patch
> > <https://git.reviewboard.kde.org/r/122364/#fcomment345>
> >
> >     Can this ```<center>``` work? There is no new paragraph or line break.

Yes, it does work the way I expected.  The second sentence is centered.

- Allan

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

On Feb. 3, 2015, 12:27 a.m., Allan Anderson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122364/
> -----------------------------------------------------------
> 
> (Updated Feb. 3, 2015, 12:27 a.m.)
> 
> 
> 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/20150217/181f4dcf/attachment-0001.html>