----------------------------------------------------------- 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 > >
_______________________________________________ KMyMoney-devel mailing list KMyMoney-devel@kde.org https://mail.kde.org/mailman/listinfo/kmymoney-devel
