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
_______________________________________________
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
