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

List:       kde-bugs-dist
Subject:    [Bug 81989] kmail need to support HTML signature
From:       Thomas McGuire <Thomas.McGuire () gmx ! net>
Date:       2008-01-26 21:35:49
Message-ID: 20080126213549.5341.qmail () ktown ! kde ! org
[Download RAW message or body]

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.
         
http://bugs.kde.org/show_bug.cgi?id=81989         




------- Additional Comments From Thomas.McGuire gmx net  2008-01-26 22:35 -------
Thanks for the patch.
I tested it, but it seems that it doesn't work. When I enable HTML signatures, KMail \
adds the signature as HTML code instead of actually formatting it as HTML. It seems \
that this is because of KMeditor::insertSignature(), which always calls \
insertPlainText(). You probably need to change that so it calls insertHtml() instead, \
but only if the signature is a HTML signature.


> +    QString _InlinedHtml = config.readEntry( sigTypeInlinedHtmlKey );
> +    if ( _InlinedHtml == "true")
> +      mInlinedHtml = true;
No need to take the detour to read a QString and changing it to a bool then, \
readEntry supports reading bools directly. Change that to 
> mInlinedHtml = config.readEntry( sigTypeInlinedHtmlKey, false );
Otherwise, mInlinedHtml is even undefined if _InlinedHtml is something else than \
"true", which leads to the signature being always HTML, regardless of the config \
value.

A couple of minor nitpicks follow now:

> +      /**  return true if the inlined signature is html formatted */
> +      bool SignatureIsInlinedHtml() const;
You need to add " since 4.1" into the comment to make clear that this method does not \
exist with the kdepimlibs from 4.0 Additionally, method names should always start \
with a lowercase letter, so change it to signatureIsInlinedHtml.

> +      void setInlinedHtml( bool isHtml );
> +      bool isInlinedHtml() const;
add " since 4.1", and comments for  return and  param isHtml. Generally, new methods \
for libraries should be well documented.


> +    mHtmlCheck = new QCheckBox( i18n("&use HTML"), page );
Should be "&Use HTML"

> +    void setBtnState(ViewMode state);
Change this to the following, as per the KMail code style:
> +    void setBtnState( ViewMode state );

Same with:
> +      setBtnState(ShowHtml);
> +      setBtnState(ShowCode);
> +  void SignatureConfigurator::setBtnState(ViewMode state) {
and maybe others

> +  }
> +  void SignatureConfigurator::setBtnState(ViewMode state) {
Add a newline between the two lines

> +    kDebug(5006) << "SignatureConfigurator::slotSetHtml";
Unnecessary, change it to
> kDebug(5006);


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