[Kmymoney-devel] KMyMoney: proposal to replace hard coded utf-8 in KReportsView::KReportTab

[Alvaro Soliverez]

Hello Bernd,
the code itself looks fine, but there are some adjustments to be made.


On Sun, Apr 11, 2010 at 12:10 PM, Bernd Gonsior <bgo at freeplexx.de> wrote:
> Hello Alvaro,
>
> the new class is ready for review.
>
> Changes at a glance:
> - html generation is moved from KReportsView to KReportsViewHtml

there is no need to create a new class. the code to create the new
class should be in ReportTable.
A new file reporttable.cpp could hold that code, since it's used by
all child classes.

In fact, the current renderHTML() method of the child classes could be
renamed to renderBody(), and be called from the ReportTable. So,
KReportView should only call createTable of the child classes, and it
would execute all what's needed.
Ask me if you need clarification on this.

> - added a simple unit test to check the html encoding
Good.

> - KFileDialog in KReportsView::slotSaveView did not show the includeCss
>  checkbox, fixed
> - state of file filter for KFileDialog in KReportsView::slotSaveView was not
>  kept between successive calls, fixed
>
You deleted the Private class. The FileSaveProperties class should
actually be within that Private class and not instead of it. That aids
in library compatibility.

Next time, please try to separate the patches when they fix more than
one stuff to make it easier for review.

> All sources and the svn-diff are in the attached archive reports-html.tgz.
>

Overall it's good, just a few changes here and there that avoid new
classes and help maintainability.

Thanks!

Regards,
Alvaro