[Kmymoney-devel] Review Request: Allan's csvimporter plugin

Mon Jun 28 12:45:07 CEST 2010

> On 2010-06-28 06:57:25, Thomas Baumgart wrote:
> > trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/convdate.cpp, line 29
> > <http://reviewboard.kde.org/r/4462/diff/1/?file=29801#file29801line29>
> >
> >     use character assignment as in QString slash = '/';  Krazy will warn you anyway.

Yes, it did and I sorted it yesterday!

> On 2010-06-28 06:57:25, Thomas Baumgart wrote:
> > trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/convdate.cpp, line 149
> > <http://reviewboard.kde.org/r/4462/diff/1/?file=29801#file29801line149>
> >
> >     What is so special about -1 to be detected and yet allow -2?  The documentation of the method does not include a hint. Better use a range check.

The values come from the combo box, and I (did originally) initialise to -1.  So, if -1 is returned, the user didn't select a needed parameter.

I've no hang up about changing it, but what will it achieve in these circumstances?  I'll check back in any case as things can get changed without the consequences being appreciated at the time.

> On 2010-06-28 06:57:25, Thomas Baumgart wrote:
> > trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/csvimporterdlg.h, line 44
> > <http://reviewboard.kde.org/r/4462/diff/1/?file=29804#file29804line44>
> >
> >     Indentation: please try to run astyle-kmymoney.sh now and then. (I know, I forget it too sometimes)

Not guilty, m'lord!  Twas OK when I sent it to Cristian.  He did a major prune, and that must have slipped in.

I've been making use of KDevelop tools/align, but on the question of style, I find it better for ensuring parentheses are balanced if they are vertically aligned, but Cristian prefers otherwise.  I know there may be flame-wars about these things, but are there agreed standards?  I have seen both in use in the same file in KMM, although I've not been looking at this recently.

> On 2010-06-28 06:57:25, Thomas Baumgart wrote:
> > trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/csvimporterdlg.cpp, line 507
> > <http://reviewboard.kde.org/r/4462/diff/1/?file=29805#file29805line507>
> >
> >     This whole line is a NOOP. inFile is just contructed in the source line above and thus cannot be open.

Hmmm...  That's true...

> On 2010-06-28 06:57:25, Thomas Baumgart wrote:
> > trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/csvimporterdlg.cpp, line 570
> > <http://reviewboard.kde.org/r/4462/diff/1/?file=29805#file29805line570>
> >
> >     Please try to use layouts instead of fixed sizes.

Yes, Alvaro made this point.  I've asked a related question of him, and would appreciate comments on that.

> On 2010-06-28 06:57:25, Thomas Baumgart wrote:
> > trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/investmentdlg.cpp, line 50
> > <http://reviewboard.kde.org/r/4462/diff/1/?file=29810#file29810line50>
> >
> >     I spotted this also in another place: why do you have a fixed (limited) list of codecs? There's an example at http://doc.trolltech.com/4.4/tools-codecs-mainwindow-cpp.html in the method MainWindow::findCodecs()
> >      on how to extract them from Qt itself.

Ah, that's good.  I've had a question in my mind about this for a while.  The reason I put them in in the first place was that I received a few non-UK files examples and wanted to remove currency symbols in case they caused trouble (although, subsequently I've found they don't seem to).  For the user, that's not something he is likely to face as he's working in his own locale.  So, I wondered whether they served any purpose, except, perhaps, for someone dealing in multi-currencies.

Anyway, they're there, and I'll check on that link to do it properly.  I was sort of expecting that I'd need to revisit at some stage.

> On 2010-06-28 06:57:25, Thomas Baumgart wrote:
> > trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/investmentdlg.cpp, line 603
> > <http://reviewboard.kde.org/r/4462/diff/1/?file=29810#file29810line603>
> >
> >     Use the QString::clear() as in trInvData.memo.clear() to clear strings.

I've recently seen similar points made, although I've not seen why it's better.

But, if it's better, I'd better do it!

> On 2010-06-28 06:57:25, Thomas Baumgart wrote:
> > trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/csvimporterdlg.h, line 3
> > <http://reviewboard.kde.org/r/4462/diff/1/?file=29804#file29804line3>
> >
> >     No need to keep a version for each file. SVN will take good care about that for you. If you want to keep a version for the whole package, add it as a #define in a header. (This might apply to the other files as well but I just comment on it here).

What was at the back of my mind, although not something a comment in the source would help with, is If a future user highlights a problem, how will he/I know his version?  Originally, I had a help/About menu which gave the rev., but that went with the change to plugin.  So, I've stuck a label on the ui with the version.  Is there a better way for a plugin, or do I just go by his KMM rev.?

> On 2010-06-28 06:57:25, Thomas Baumgart wrote:
> > trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/csvimporterdlg.cpp, line 258
> > <http://reviewboard.kde.org/r/4462/diff/1/?file=29805#file29805line258>
> >
> >     This would not work on my box as I don't have "/home/aga". You might want to take a look at KStandardDirs. It has plenty of methods to get the right name for your file.

Yes, Cristian noticed, too.  It was a temporary hack while I got it working under KDE, as it had worked in QCreator, and I knew I needed to come back to it, but my attention drifted elsewhere.  Apologies.  Definitely not my intention.

- Allan

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

On 2010-06-27 03:49:22, Cristian Onet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4462/
> -----------------------------------------------------------
> 
> (Updated 2010-06-27 03:49:22)
> 
> 
> Review request for kmymoney.
> 
> 
> Summary
> -------
> 
> Allan's csvimporter plugin
> 
> 
> Diffs
> -----
> 
>   trunk/extragear/office/kmymoney/kmymoney/plugins/CMakeLists.txt 1143143 
>   trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/CMakeLists.txt PRE-CREATION 
>   trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/convdate.h PRE-CREATION 
>   trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/convdate.cpp PRE-CREATION 
>   trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/csvdatetest.h PRE-CREATION 
>   trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/csvdatetest.cpp PRE-CREATION 
>   trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/csvimporterdlg.h PRE-CREATION 
>   trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/csvimporterdlg.cpp PRE-CREATION 
>   trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/csvimporterdlg.ui PRE-CREATION 
>   trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/csvimporterplugin.h PRE-CREATION 
>   trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/csvimporterplugin.cpp PRE-CREATION 
>   trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/investmentdlg.h PRE-CREATION 
>   trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/investmentdlg.cpp PRE-CREATION 
>   trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/investmentdlg.ui PRE-CREATION 
>   trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/kmm_csvimport.desktop PRE-CREATION 
>   trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/kmm_csvimport.rc PRE-CREATION 
>   trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/redefinedlg.h PRE-CREATION 
>   trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/redefinedlg.cpp PRE-CREATION 
>   trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/redefinedlg.ui PRE-CREATION 
> 
> Diff: http://reviewboard.kde.org/r/4462/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Cristian
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kmymoney-devel/attachments/20100628/2e03db86/attachment-0001.htm 