[prev in list] [next in list] [prev in thread] [next in thread] 

List:       koffice-devel
Subject:    Re: Persistant filter settings - dialog only pops up once
From:       Nicolas Goutte <nicolasg () snafu ! de>
Date:       2003-02-13 14:11:56
[Download RAW message or body]

I am sorry but I do not like your patch.

I have not much time to check your patch, but I have already found a few 
problems.

1. Encodings
You are using the descriptive encoding names (with added texts) but you are 
using your own functions and not those of KCharsets /especially 
KCharsets::encodingForName).

Personnaly, I do not like how these descriptive encoding names are handled in 
KCharsets because it forces the translator of using the characters '(' and 
')'. But do these characters really exists in every languages in whcih KDE is 
translated? (Good, this is more a KCharsets bug than a KOffice bug.)

(Note: if you are using descriptive names, you should perhaps change also the 
encodings added in the plain text filter to be descriptive too.)

2. CRLF
Using real CR and LF as parameters is not good as such parameters can never be 
passed through koconverter, if we change koconverter to support something 
like:
koconverter --patch --parameter=endofline,crlf text.kwd text.txt

3. One shot dialog
As your dialog seems to be "one shot", how the user can make it re-appear. 
(And how is the interaction when a few export filters are used?)

4. KoDocumentPrivate::filterSettings
You should perhaps follow the naming habit of adding a m_ 
So it would be m_filterSettings

Other question: where do you delete it?

(As QMap are implictely shared, would it not be better to a have QMap instead 
of a QMap* ?)

5. Out Of Memory
There is no "Out Of Memory" in operating systems with virtual memories, so you 
do not need to care about them. (The Out Of Memory Killer will trigger before 
you get a null pointer.) (And yes, I know, in the past I too made the error 
to check for NULL pointers.)

6. Code readability:
Instead of
 (*pDoc->filterSettings())["key"]="data";
you should perhasp use
 pDoc->filterSettings()->inseart("key","data");

(And be careful that both will carsh if filterSettings returns NULL.)

Have a nice day/evening/night!

On Thursday 13 February 2003 11:11, Clarence Dang wrote:
> On Wed, 12 Feb 2003 07:43 pm, Clarence Dang wrote:
> > On Sat, 8 Feb 2003 03:36 am, Nicolas Goutte wrote:
> > > However, as all these potential filters seems to have already a dialog,
> > > I think that we should let choose the user in the filter dialog if he
> > > wants the headers or not.
> >
> > No more dialogs please! :) unless the filter already has one as you said.
> > Yes probably that would be the best solution.  Easy so I'll implement it
> > later for ascii/plain text.  Hmm, maybe filter settings should be
> > persistant and stored in KoDocument...
>
> Ok, here's the patch to fix both (option for header/footer output,
> persistant filter settings).  It ends up passing a QMap<QString, QString>*
> around (the filter settings are stored in KoDocument) so ends up changing
> lib/kofficecore.  This means that if you save a text file, you will only
> get prompted with the dialog once (to change options, "Save As" again)
> making text files much, much easier to work with :).  I would imagine that
> this could be used with other more useful filters such as HTML, CSV too... 
> Also fixes a dumb encoding problem I introduced yesterday with asciifilter
> (encoding options ignored).  Comments, objections etc.?
>
> BTW, do we really have to pop up KFileDialog the first time the user is
> trying to File/Save (if s/he opened a foreign document), because it makes
> KWord _very_ hard and awkward to work with...
>
> Thanks,
> Clarence

_______________________________________________
koffice-devel mailing list
koffice-devel@mail.kde.org
http://mail.kde.org/mailman/listinfo/koffice-devel
[prev in list] [next in list] [prev in thread] [next in thread]