[Kmymoney-devel] Review Request 120507: CSV Plugin - Improve functionality across distros. Improve UI to display the whole of the file being imported, and smarten to avoid displaying split rows. Also, rework some areas of the code to avoid duplication and improve re-usability.

[Allan Anderson]

> On Oct. 7, 2014, 12:47 p.m., Cristian Oneț wrote:
> > I wouldn't wait for a ship it since, as always the patch is huge, I don't want to speak for others but it's really hard for anybody to make a relevant review this way. I know we had our ups and downs so take this advice as a sign that I care enough to not leave this review request unanswered. If you find the patch good just ship it.
> > 
> > Also I support the effort of trying to improve and to avoid duplication but all I see is that 'CSVDialog' keeps getting new members and judging by their names it has become the most complex state machine I've ever seen.
> > 
> > Here are some nitpicks I came up with by looking at the request.

Thanks Cristian.  As you say, the patch is huge, although that mainly comes from several (nearly all) UI files having changes.  Alvaro mentioned recently that he doesn't use QTDesigner because of its tendency to bloat the files, I suspect rather like Windows Office.  If I were more confident, I too would use a text editor.  In addition, some code I wanted to change was also going to need to be reused, so I extracted it into two separate routines, and the original large routine I was able to dispense with.  I don't foresee any further major changes in the plugin!
This patch and the recent associated "BUG:339044 REVIEW:120260 - Fix CSV import wizard not working correctly on Windows", between them removed 431 lines, so I think I've moved in the right direction.


> On Oct. 7, 2014, 12:47 p.m., Cristian Oneț wrote:
> > kmymoney/plugins/csvimport/investmentwizardpage.ui, line 302
> > <https://git.reviewboard.kde.org/r/120507/diff/1/?file=316711#file316711line302>
> >
> >     Do we really need to center all strings? As a translator I found it verry hard to translate the csvimporter because of the many rich text directives. Please take KMyMoney UI as an example.

You did mention this before, but now I understand to what you are referring.  May be this is another example of Designer bloat.  I might be able to get a simpler result from producing these UI labels via code.  I'll keep my eye on that.


> On Oct. 7, 2014, 12:47 p.m., Cristian Oneț wrote:
> > kmymoney/plugins/csvimport/investprocessing.h, line 146
> > <https://git.reviewboard.kde.org/r/120507/diff/1/?file=316712#file316712line146>
> >
> >     Whitespace errors (useless whitespace in empty lines or at the end of line) are pretty well highlighted by reviewboard.

Oops, missed that extra line when lookin through the patch.  Did you say you were going to look into using a more up-to-date version of astyle?


- Allan


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


On Oct. 5, 2014, 6:34 p.m., Allan Anderson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120507/
> -----------------------------------------------------------
> 
> (Updated Oct. 5, 2014, 6:34 p.m.)
> 
> 
> Review request for KMymoney.
> 
> 
> Repository: kmymoney
> 
> 
> Description
> -------
> 
> Improve functionality across distros.
> It was also the intention to save the windows settings, but it was realised that there was a liklihood that the next file to be imported could have quite different column width and number of rows. So, it was decided to  display the whole of the file being imported instead.  
> Also,the UI has been improved to avoid displaying split rows, with all possible rows being displayed, the window height aligning with the rows, and the width displaying complete columns, as far as the screen allowed.  Further, I reworked some areas of the code to avoid duplication and improve re-usability.
> 
> 
> Diffs
> -----
> 
>   kmymoney/plugins/csvimport/lines-datewizardpage.ui 884f3ba 
>   kmymoney/plugins/csvimport/separatorwizardpage.ui b6dba9f 
>   kmymoney/plugins/csvimport/bankingwizardpage.ui 0d6a4a8 
>   kmymoney/plugins/csvimport/csvdialog.h 24abd9a 
>   kmymoney/plugins/csvimport/csvdialog.cpp ab20ed4 
>   kmymoney/plugins/csvimport/csvdialog.ui 61ab9ce 
>   kmymoney/plugins/csvimport/csvimporterplugin.h d2d2f6c 
>   kmymoney/plugins/csvimport/csvimporterplugin.cpp 602b4d9 
>   kmymoney/plugins/csvimport/introwizardpage.ui a597d56 
>   kmymoney/plugins/csvimport/investmentdlg.cpp 5e7b266 
>   kmymoney/plugins/csvimport/investmentwizardpage.ui 846836e 
>   kmymoney/plugins/csvimport/investprocessing.h fa8ffa0 
>   kmymoney/plugins/csvimport/investprocessing.cpp 1629e14 
> 
> Diff: https://git.reviewboard.kde.org/r/120507/diff/
> 
> 
> Testing
> -------
> 
> Tested with numerous banking and investment files from different sources, on Linux Mint KDE and Ubuntu Unity.
> 
> 
> Thanks,
> 
> Allan Anderson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kmymoney-devel/attachments/20141007/bf59682e/attachment.html>