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

Thu Feb 12 21:24:55 UTC 2015

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

That is a really huge diff. Here are some things I noticed while skimed through it.

It seems you reduced the code; very good!

File Attachment: Updated patch - 0002-BUG-340656.patch
<https://git.reviewboard.kde.org//r/122364/#fcomment286>

    I do not understand this: "…if only one value column…"

File Attachment: Updated patch - 0002-BUG-340656.patch
<https://git.reviewboard.kde.org//r/122364/#fcomment287>

    I do not understand this: "…if only one value column…"

File Attachment: Updated patch - 0002-BUG-340656.patch
<https://git.reviewboard.kde.org//r/122364/#fcomment288>

    This should be ```#include "…"```

File Attachment: Updated patch - 0002-BUG-340656.patch
<https://git.reviewboard.kde.org//r/122364/#fcomment289>

    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.

File Attachment: Updated patch - 0002-BUG-340656.patch
<https://git.reviewboard.kde.org//r/122364/#fcomment290>

    Is this used anywhere?

File Attachment: Updated patch - 0002-BUG-340656.patch
<https://git.reviewboard.kde.org//r/122364/#fcomment291>

    Is this used anywhere?

File Attachment: Updated patch - 0002-BUG-340656.patch
<https://git.reviewboard.kde.org//r/122364/#fcomment292>

    This should be done in a more modular fashion (general annotation).

File Attachment: Updated patch - 0002-BUG-340656.patch
<https://git.reviewboard.kde.org//r/122364/#fcomment293>

    ```Qt::WindowFlags eFlags = windowFlags();``` (space removed)

File Attachment: Updated patch - 0002-BUG-340656.patch
<https://git.reviewboard.kde.org//r/122364/#fcomment294>

    This part can be removed, the flag is set and removed again. Also you call show() twice.

File Attachment: Updated patch - 0002-BUG-340656.patch
<https://git.reviewboard.kde.org//r/122364/#fcomment295>

    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?).

File Attachment: Updated patch - 0002-BUG-340656.patch
<https://git.reviewboard.kde.org//r/122364/#fcomment296>

    Why do you need the ui to change?

File Attachment: Updated patch - 0002-BUG-340656.patch
<https://git.reviewboard.kde.org//r/122364/#fcomment297>

    Why do you disconnect and reconnect later? Also this connection should be done in CSVWizard.

File Attachment: Updated patch - 0002-BUG-340656.patch
<https://git.reviewboard.kde.org//r/122364/#fcomment298>

    Hehe, such annoying calculations appear if you do not trust the library ;)

File Attachment: Updated patch - 0002-BUG-340656.patch
<https://git.reviewboard.kde.org//r/122364/#fcomment299>

    ```+ 2 + 1``` ?

File Attachment: Updated patch - 0002-BUG-340656.patch
<https://git.reviewboard.kde.org//r/122364/#fcomment300>

    You have the method ```clearColumnNumbers()``` for this already.

File Attachment: Updated patch - 0002-BUG-340656.patch
<https://git.reviewboard.kde.org//r/122364/#fcomment301>

    "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.

File Attachment: Updated patch - 0002-BUG-340656.patch
<https://git.reviewboard.kde.org//r/122364/#fcomment302>

    Here ```show()``` is called twice and setting the flag has no effect as it is removed again.

File Attachment: Updated patch - 0002-BUG-340656.patch
<https://git.reviewboard.kde.org//r/122364/#fcomment303>

    ```if (m_wiz->m_pageIntro->isVisible() || m_wiz->m_pageLinesDate->isVisible())```

File Attachment: Updated patch - 0002-BUG-340656.patch
<https://git.reviewboard.kde.org//r/122364/#fcomment304>

    Same as before.

File Attachment: Updated patch - 0002-BUG-340656.patch
<https://git.reviewboard.kde.org//r/122364/#fcomment305>

    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

File Attachment: Updated patch - 0002-BUG-340656.patch
<https://git.reviewboard.kde.org//r/122364/#fcomment306>

    ```m_wiz->ui->label_intro->setText(QLatin1String("<b>") + str + QLatin1String("</b>"));```

File Attachment: Updated patch - 0002-BUG-340656.patch
<https://git.reviewboard.kde.org//r/122364/#fcomment307>

    This method should be ```const```.

File Attachment: Updated patch - 0002-BUG-340656.patch
<https://git.reviewboard.kde.org//r/122364/#fcomment308>

    Usually you want member variables to be private.

File Attachment: Updated patch - 0002-BUG-340656.patch
<https://git.reviewboard.kde.org//r/122364/#fcomment309>

    ```QStringList    m_columnTypeList;  //  holds field types - date, payee, etc.```

    to

    ```/** holds field types - date, payee, etc. */
    QStringList    m_columnTypeList;```

    then doxygen can help you

File Attachment: Updated patch - 0002-BUG-340656.patch
<https://git.reviewboard.kde.org//r/122364/#fcomment310>

    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.

File Attachment: Updated patch - 0002-BUG-340656.patch
<https://git.reviewboard.kde.org//r/122364/#fcomment311>

    Library includes should be above project includes.

File Attachment: Updated patch - 0002-BUG-340656.patch
<https://git.reviewboard.kde.org//r/122364/#fcomment312>

    else if

File Attachment: Updated patch - 0002-BUG-340656.patch
<https://git.reviewboard.kde.org//r/122364/#fcomment313>

    else if

File Attachment: Updated patch - 0002-BUG-340656.patch
<https://git.reviewboard.kde.org//r/122364/#fcomment314>

    This method looks very similar to the following ```…ColumnSelected()``` methods. They could be compacted with some template magic.

File Attachment: Updated patch - 0002-BUG-340656.patch
<https://git.reviewboard.kde.org//r/122364/#fcomment315>

    Can this ```<center>``` work? There is no new paragraph or line break.

- Christian David

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/20150212/49d5b4a6/attachment-0001.html>