[Kde-pim] Re: Review Request: Fix for Bug 207779 - Kmail removes empty lines in HTML

[Frank Vanderham]

> On 2011-01-03 11:06:23, Torgny Nyblom wrote:
> > Why are you using regexps at all? You are only replacing a fixed string with another fixed string.
> > 
> > http://doc.trolltech.com/4.7/qstring.html#replace-8

I'm using Wildcard matching for the <ol> and <ul> fixes. Since there is no measurable performance difference with exact string matches, I felt that taking the same approach for <p> fixes was better. It would also allow for future changes in case Qt changes the markup yet again. 


> On 2011-01-03 11:06:23, Torgny Nyblom wrote:
> > messagecomposer/kmeditor.cpp, line 732
> > <http://git.reviewboard.kde.org/r/100281/diff/3/?file=6005#file6005line732>
> >
> >     Could this be rewritten into something like:
> >     
> >     int offset = emptyLineFinder.indexIn(..);
> >     while ( offset != -1 ) {
> >       result.replace(..);
> >       offset = ...;
> >     }
> >     
> >     In my opinion that improves readability, but it's not a big deal.

The diff messed this up - I'm not following; do you mean setting the initial offset before the while loop? I actually borrowed this code from a different patch for a similar issue, so I have no problems changing it if needed.


> On 2011-01-03 11:06:23, Torgny Nyblom wrote:
> > messagecomposer/kmeditor.cpp, line 746
> > <http://git.reviewboard.kde.org/r/100281/diff/3/?file=6005#file6005line746>
> >
> >     Since these are of static length and the same why the test?
> >     
> >     And why not use matchedLenght() both here and above?


* Perform the test for static length: I wanted the code to be idiot proof in case future changes are only made to the "static const" strings, which could then be different length and we need to advance to the lesser length of the two
* why not use matchedLength: if we use matchedLenght, that would be a bug since the replacement has a smaller length than the original, in which case a second occurance in a row could be missed. We need to advance the search up by the replaced string length only.


- Frank


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/100281/#review708
-----------------------------------------------------------


On 2011-01-03 02:24:52, Frank Vanderham wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100281/
> -----------------------------------------------------------
> 
> (Updated 2011-01-03 02:24:52)
> 
> 
> Review request for KDEPIM.
> 
> 
> Summary
> -------
> 
> The KMail message editor uses Qt's rich text editor functionality. The HTML markup generated by this control is not compatible with email readers such as MS Outlook. This fix intercepts the HTML markup generated by the editor and fixes the markup before sending it out.
> Contrary to the previous approach, I now use good ol' regular expressions instead of the QWebKit to parse and change the HTML markup, in order to remove the QWebKit dependency. This should make the fix applicable to WinCE and also backwards compatible to 4.4 (which I hope happens, since that is my purpose here).
> 
> [Personal note: I need to stress that as end-user of KDE for business purposes myself, this is a huge problem with KMail today: my customers complain my KMail generated email looks terrible when received, which is what prompted me to roll up my sleeves and fix it myself.]
> 
> 
> This addresses bug 207779.
>     http://bugs.kde.org/show_bug.cgi?id=207779
> 
> 
> Diffs
> -----
> 
>   messagecomposer/CMakeLists.txt ae2c106 
>   messagecomposer/kmeditor.cpp 86bae8a 
> 
> Diff: http://git.reviewboard.kde.org/r/100281/diff
> 
> 
> Testing
> -------
> 
> I stood up a MS system with Outlook Express and send through a number of HTML formatted emails, especially with "empty lines", bullet lists and numbered lists (which all show up mangled prior to the fix). All those emails arrived as desired.
> 
> After Torgny's comment, I also added a test case for Thunderbird: email arrived as desired. Of course, the mail viewed in KMail also looks as required.
> 
> 
> Thanks,
> 
> Frank
> 
>

_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/