Re: [Libreoffice] [PUSHED] [PATCH] Fix Bug 34666

Am 18.03.2011 14:08, schrieb Petr Mladek:
> Thorsten Behrens píše v Pá 18. 03. 2011 v 13:29 +0100:
> > Christina Roßmanith wrote:
> > > first bug fix. Built successfully and could open testdoc.html correctly.
> Congratulations! I am looking forward to see more fixes from you.
>
> > Very cool, checked that - it's pushed! :)
> Heh, I have tried the patch yesterday and did not see any difference in
> the HTML import. I was not sure what it actually fixed, so I asked
> Cedric for more details (he created the simplified test case). I havn't
> got the response yet.
The testdoc.html has a unknown tag which is closed inside the tag 
<o:bla........../>. I tried to add a separate closing tag 
<o:bla......></o:bla> and that worked. (*) But I thought, the goal is to 
open testdoc.html properly not to change it that it can be opened. Then 
I saw that the closing '/>' was eaten by ScanText('>') which eats 
everything up to '>'. But if you inspect the last character of aToken 
you can decide that the tag was closed immediately. What I didn't check 
is whether testdoc.html is HTML-conformant. But see above (*)...

> Note that you did the hard job with finding the right place to fix, ...
> Just please excuse my nitpicking. I would suggest 3 changes:
>
> 1. IMHO, it should check for the size of aToken to avoid a potential
> crash:
>
>    - if ('/' == aToken.GetChar(aToken.Len()-1)) {
>    + if (aToken.Len()>= 1&&  '/' == aToken.GetChar(aToken.Len()-1)) {
Accepted  :-)
> 2. #i34666# associates OOo isusezilla. The bug is evidently from Free
> Desktop bugzilla, so there should be fdo#34666 instead of #i34666# in
> the comment.
Accepted :-)
> 3. I prefer commit messages that gives a rough idea about what has been
> fixed. The bug number is important but it is not that helpful when you
> look a fix in a specific area. Next time, It would suggest somethig
> like:
>
>    --- cut ---
>    comment import problems with unsupported HTML tag (fdo#34666)
>    --- cut ---
>
> I have done the first two suggested changes by
> http://cgit.freedesktop.org/libreoffice/libs-gui/commit/?id=95e301d0a55b8145ce0cdb037e438bbcfcd4a4eb
Thanks. I expected some improving suggestions, so I'm not offended.

Christina