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

[Bernd Gonsior]

Alvaro Soliverez schrieb am 14.04.2010 05:05:
> 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.

Here i need some clarification. Class ReportTable is described as 'interface
definition' and currently contains only virtual members. I could add the
methods of KReportsViewHtml to ReportTable as normal, non-virtual members in
following ways:

1. as simple methods
2. as a new inner class of ReportTable containing all methods
3. as inner class of your standard inner class 'Private'

What would you prefer? Maybe you prefer another way?


> A new file reporttable.cpp could hold that code, since it's used by
> all child classes.

OK

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

OK, this would influence following sources:

views/kreportsview.cpp
reports/pivottable.h
reports/querytabletest.cpp
reports/listtable.h
reports/reporttable.h
reports/pivottable.cpp
reports/reportstestcommon.cpp
reports/listtable.cpp


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

OK, i had a glimpse at your developer-doc but i did not find something about
this (the doc seems to be a bit out of date). Where could i have a look for
things like that?

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

OK, there will be 3 patches:

1. 'make includeCss checkbox appear'

2. 'separate html generation', to check all functionality of this patch no. 1
   is needed

3. 'rename renderHTML() to renderBody()'

Regards,
Bernd