On Sunday 08 April 2007 04:56, Olivier Goffart wrote:
> Hi, thanks for the patch.
> Some note:
>
>
> There is no reason to use QPointer. (or is it?)
I used it because the memory management of the thing it points to is dicey, so 
I wanted the most protection from mistakes as I could get.

>
> QString::null is deprecated in QT4, use QString().
Ok

> Why did you change some QLatin1String to QString::fromLatin1 ?
I didn't do that, someone else did between the 3rd developer snapshot and now

>
> in setBody( QTextDocument , MessageFormat ) ,  the default argument of
> MessageFormat should be richtext.  i would say it is not even required to
> add the MessageFormat argument.
>
Ok for the default suggestion

I think the parameter should be left in because if someone wants to specify 
the body and the format, how else would they do it? (unless we add a 
setMessageFormat() method)

> Also, the MessageFormat sementics should probably be changed.
> (using Qt::TextFormat in constructor, but using setPlainBody and
> setHtmlBody in favor of setBody, which lead to lots of errors)
I have thought about the use of MessageFormat and I'm leaving it how it is. 
Qt::TextFormat seems attractive, but Message defines more types than 
TextFormat offers (like ParsedHTML and Crypted). For the same reason, 
setPlainBody and setHtmlBody leave out some types and make expanding for 
future types more difficult. I don't really see how setHtmlBody and 
setPlainBody would lead to any fewer errors, since it's really the same as 
setBody in that the caller still has to know what kind of string they are 
passing.

>
>
> about removing \n, this is correct to do that, at least for jabber.
Ok

Thanks for all the feedback


-- 
~Charles Connell
_______________________________________________
kopete-devel mailing list
[email protected]
https://mail.kde.org/mailman/listinfo/kopete-devel

Reply via email to