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

[Cristian Oneț]

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


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.


kmymoney/plugins/csvimport/investmentwizardpage.ui
<https://git.reviewboard.kde.org/r/120507/#comment47429>

    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.



kmymoney/plugins/csvimport/investprocessing.h
<https://git.reviewboard.kde.org/r/120507/#comment47428>

    Whitespace errors (useless whitespace in empty lines or at the end of line) are pretty well highlighted by reviewboard.


- Cristian Oneț


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/1d006c3c/attachment.html>