[Kmymoney-devel] Review Request 109803: Add CSV export capability.

Sun Apr 21 23:55:37 UTC 2013

> On April 21, 2013, 1:39 p.m., Thomas Baumgart wrote:
> > kmymoney/plugins/csvexport/csvexportdlg.h, line 55
> > <http://git.reviewboard.kde.org/r/109803/diff/3/?file=139975#file139975line55>
> >
> >     Using the xxxDecl classes is somewhat old fashioned. No need for that in this case. You can directly apply the multi inheritance to CsvExportDialog and don't need CsvExportDlgDecl at all. See http://apidocs.meego.com/1.2/qt4/designer-using-a-ui-file.html for a good tutorial. Personally, I am currently using the single inheritance method in another project, but that does not mean anything.
> 
> Allan Anderson wrote:
>     And I only started to use that to be more compatible with the project!

Done.

> On April 21, 2013, 1:39 p.m., Thomas Baumgart wrote:
> > kmymoney/plugins/csvexport/csvexportdlg.h, line 87
> > <http://git.reviewboard.kde.org/r/109803/diff/3/?file=139975#file139975line87>
> >
> >     make that "const QString& getAccountid(const QString& str)"

Done.

> On April 21, 2013, 1:39 p.m., Thomas Baumgart wrote:
> > kmymoney/plugins/csvexport/csvexportdlg.h, line 92
> > <http://git.reviewboard.kde.org/r/109803/diff/3/?file=139975#file139975line92>
> >
> >     In case you don't return a reference to the QDate object, there's no need for the return value to be const.

Done.

> On April 21, 2013, 1:39 p.m., Thomas Baumgart wrote:
> > kmymoney/plugins/csvexport/csvexportdlg.h, line 99
> > <http://git.reviewboard.kde.org/r/109803/diff/3/?file=139975#file139975line99>
> >
> >     Same here with the const return value.

Done.

> On April 21, 2013, 1:39 p.m., Thomas Baumgart wrote:
> > kmymoney/plugins/csvexport/csvexportdlg.h, line 140
> > <http://git.reviewboard.kde.org/r/109803/diff/3/?file=139975#file139975line140>
> >
> >     Not sure if using a default argument for a slot is a good idea. Please see http://harmattan-dev.nokia.com/docs/library/html/qt4/signalsandslots.html#signals-and-slots-with-default-arguments

The way I read it is that it is OK, as long as "the signature passed to the SIGNAL() macro must not have fewer arguments than the signature passed to the SLOT() macro".  So, it would appear to be OK here.  In fact, removing it results in three "Object::connect: No such slot CsvExportDlg::checkData()
Object::connect:  (sender name:   'm_qlineeditFile')" messages, from where the SLOT argument is empty.

I would say it is definitely necessary, because of the connects with bool SIGNAL() arguments, the slot expecting QString (I think).

> On April 21, 2013, 1:39 p.m., Thomas Baumgart wrote:
> > kmymoney/plugins/csvexport/csvexportdlg.cpp, line 54
> > <http://git.reviewboard.kde.org/r/109803/diff/3/?file=139976#file139976line54>
> >
> >     In case you eliminate the xxxxDecl class, you would place your call to "setupUi(this)" here.

Already done.

> On April 21, 2013, 1:39 p.m., Thomas Baumgart wrote:
> > kmymoney/plugins/csvexport/csvexportdlg.cpp, line 164
> > <http://git.reviewboard.kde.org/r/109803/diff/3/?file=139976#file139976line164>
> >
> >     I would use "!listTrans.isEmpty()" here and avoid the continue stuff and embrace the earliest date condition and latestDate assignment with it.

Of course.

> On April 21, 2013, 1:39 p.m., Thomas Baumgart wrote:
> > kmymoney/plugins/csvexport/csvexportdlg.cpp, line 224
> > <http://git.reviewboard.kde.org/r/109803/diff/3/?file=139976#file139976line224>
> >
> >     Do you really need to sort the list here or can we improve performance by sorting it once at the end of this method?

Doh!  OK.

> On April 21, 2013, 1:39 p.m., Thomas Baumgart wrote:
> > kmymoney/plugins/csvexport/csvexportdlg.cpp, line 235
> > <http://git.reviewboard.kde.org/r/109803/diff/3/?file=139976#file139976line235>
> >
> >     Can't you use MyMoneyFile::accountByName()? It seems to have the same functionality.

Similar, anyway.  It returns a MyMoneyAccount, rather than the id.  I knew there must be a suitable KMM method, but missed this.

> On April 21, 2013, 1:39 p.m., Thomas Baumgart wrote:
> > kmymoney/plugins/csvexport/csvwriter.cpp, line 82
> > <http://git.reviewboard.kde.org/r/109803/diff/3/?file=139981#file139981line82>
> >
> >     Can you use "m_plugin->m_dlg->deleteLater()" here?

Would that make any difference?  Perhaps the comment is misleading.  I based the method loosely on the QIF importer, but that deleted too soon, and the progressbar I'd added wasn't visible, so I positioned the delete after the export completed.

Breaking off here to continue tomorrow /today.

- Allan

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109803/#review31365
-----------------------------------------------------------

On April 19, 2013, 5:52 p.m., Allan Anderson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109803/
> -----------------------------------------------------------
> 
> (Updated April 19, 2013, 5:52 p.m.)
> 
> 
> Review request for KMymoney.
> 
> 
> Description
> -------
> 
> Add CSV export capability.  Modify existing KMyMoney File menu in order to make menu item positions more logical.
> 
> 
> This addresses bug 317614.
>     http://bugs.kde.org/show_bug.cgi?id=317614
> 
> 
> Diffs
> -----
> 
>   kmymoney/plugins/CMakeLists.txt 81ca458 
>   kmymoney/plugins/csvexport/CMakeLists.txt PRE-CREATION 
>   kmymoney/plugins/csvexport/csvexportdlg.h PRE-CREATION 
>   kmymoney/plugins/csvexport/csvexportdlg.cpp PRE-CREATION 
>   kmymoney/plugins/csvexport/csvexportdlgdecl.ui PRE-CREATION 
>   kmymoney/plugins/csvexport/csvexporterplugin.h PRE-CREATION 
>   kmymoney/plugins/csvexport/csvexporterplugin.cpp PRE-CREATION 
>   kmymoney/plugins/csvexport/csvwriter.h PRE-CREATION 
>   kmymoney/plugins/csvexport/csvwriter.cpp PRE-CREATION 
>   kmymoney/plugins/csvexport/kmm_csvexport.desktop PRE-CREATION 
>   kmymoney/plugins/csvexport/kmm_csvexport.rc PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/109803/diff/
> 
> 
> Testing
> -------
> 
> Exported numerous checking and investment CSV files, and then imported into KMyMoney via CSV import (discovering a few minor issues in the existing KMyMoney accounts in the process.)
> Ran astyle and Krazy2.
> 
> 
> Thanks,
> 
> Allan Anderson
> 
>

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