Bug 5232

Created attachment 4938 [details] test results for yen, won and tilde Well, verifying the handling of the questionable characters appears troublesome - when rendering, MSIE, FF and Safari give very different results, and even different versions of Firefox do (tested with 1.5/Mac and 1.0.4/Linux). I think I'll just try to keep their encoding unchanged, as much as possible.

Created attachment 5042 [details] proposed patch - Rewrote fromUnicode to use ICU - ...which nicely fixed surrogate pair handling - ... and changed the results for Japanese encodings. Please see the comments in layout tests for test matrices. The change in ISO-2022-JP looks especially suspicious. Please see <http://dev.icu-project.org/cgi-bin/icu-bugs/returned?id=1252> and <http://dev.icu-project.org/cgi-bin/icu-bugs/returned?id=1287>. I cannot determine which behavior is correct. I decided not to do the conversion to precomposed Unicode here. It's unclear if it should be done at this level at all, and the scope of the proposed patch is already fairly large.

Comment on attachment 5042 [details] proposed patch Looks OK. It's unfortunate that the loop uses QCString::append because that makes the loop n^2 in the size of the string. Each conversion buffer has its size recalculated using strlen() and worse, the base QCString has its size recalculated using strlen() each time QCString::append is called. A more efficient version would use QCString::resize and keep track of the length of the result. The original CFString version had this problem too, so I won't do a review- on this just because of it, but it's worth fixing. I'm concerned about the handling of U+005C in ISO-2022-JP, but it looks like it's no worse after this change than before. What are we going to do about character sets not yet supported by ICU, such as KOI8-U (see bug 4195)? Setting review- until I have the answer to at least this last question.

Created attachment 5047 [details] revised patch Uses QByteArray::resize() to avoid multiple strlens. Not sure if it's practical, because the buffer size is 16K, but it makes the code only marginally more complex, so sounds like a good change. As for KOI8-U, one solution is to keep ICU changes out of mainline until Leopard. Alternatively, it's possible to add support for this encoding via ucnv_openPackage(). Depends on whether there are more missing encodings, I guess.

Comment on attachment 5047 [details] revised patch Looking great! If we want to keep this out of "mainline" then we can't check into tip of tree. We need TOT to be something suitable for shipping before Leopard -- it's not OK to knowingly regress something in a way that's only fixed by Leopard. There's no need to call QByteArray::resize directly -- QCString::resize doesn't do any call to strlen, and in fact it sets the terminating 0 byte correctly, so this: static_cast<QByteArray&>(result).resize(resultLength + chunkLength + 1); memcpy(result.data() + resultLength, buffer, chunkLength); result.data()[resultLength + chunkLength] = '\0'; can just be: result.resize(resultLength + chunkLength + 1); memcpy(result.data() + resultLength, buffer, chunkLength); I think we really need to consider what to do about the character sets that ICU can't handle and CF can. First step I suppose would be to inventory which character sets those are. I think that the call to ucnv_setSubstChars needs its own ASSERT about the error code returned.

QCString::resize() is private, this is why I had to use QByteArray::resize(). Actually, if ucnv_setSubstChars() returns an error (which is hardly realistic), it will be reported by an existing assert() below, because of ICU's error handling mechanism. Going to investigate what other encodings besides KOI8-U are broken...

Similar code has been just landed by Maciej for Win32. FWIW, it uses QCString::append(), not resize(). Japanese backslash handling is also different. Since KOI8-U is by far not the only encoding missing from ICU, further architectural fixes may be needed to use both ICU and CFString on Mac OS X.

Everything here has been addressed over time (with a possible exception of reverse solidus and tilde handling, which will need to be revisited if any bugs are reported).