Salvage #13772 #13935
Salvage #13772 #13935
Conversation
Allow to convert from UTF-8 to whatever encoding the device supports
Swiftb0y
force-pushed
the
review/christophehenry/gh13772-charset-encoding-salvage
branch
4 times, most recently
from
f4219e9
to
bd5996c
Compare
Swiftb0y
force-pushed
the
review/christophehenry/gh13772-charset-encoding-salvage
branch
from
bd5996c
to
d131d1c
Compare
| case ISO_10646_UCS_2: | ||
| return 68; | ||
| } | ||
| // unreachable (TODO assert false?) |
There was a problem hiding this comment.
That assert would make sense IMO
Returning part of a usable result instead of nullbytes since those likely terminate the string early or even corrupt the underlying binary message format the buffer is embedded in.
src/controllers/scripting/legacy/controllerscriptinterfacelegacy.h
Outdated
Show resolved
Hide resolved
| * @param value The string to encode | ||
| * @returns The converted String as an array of bytes. Will return an empty buffer on conversion error or unavailable charset. | ||
| */ | ||
| function convertCharset(targetCharset: string, value: string): ArrayBuffer |
There was a problem hiding this comment.
Wait this is still documented even though it's now an internal API?
There was a problem hiding this comment.
forgot to remove. thanks for the reminder.
| const char* encoderName = encoderNameArray.constData(); | ||
| #endif | ||
| QStringEncoder fromUtf16 = QStringEncoder(encoderName); | ||
| if (!fromUtf16.isValid()) { |
There was a problem hiding this comment.
Why you removed the flags here?
There was a problem hiding this comment.
see the commit message, writing the replacement char is better than replacing with null bytes IMO.
There was a problem hiding this comment.
This may be a problem. During the tests I noticed that replacement char varies between Ubuntu and Fedora (maybe between Qt versions). Replace invalid chars with \0x00 is the most predictable option.
There was a problem hiding this comment.
Are you sure? I think it depends on the encoding. The Qt docs say they use QChar::ReplacementCharacter or a question mark.
There was a problem hiding this comment.
This is what I noticed here. The relevant commit is probably this one. Although I tested your branch and the tests pass here too so I'm not sure anymore.
| TEST_F(ControllerScriptEngineLegacyTest, convertCharsetCorrectValueStringCharset) { | ||
| const auto result = evaluate("engine.convertCharset('ISO-8859-15', 'Hello!')"); | ||
|
|
||
| // ISO-8859-15 ecoded 'Hello!' | ||
| EXPECT_EQ(qjsvalue_cast<QByteArray>(result), | ||
| QByteArrayView::fromArray({'\x48', '\x65', '\x6c', '\x6c', '\x6f', '\x21'})); | ||
| } | ||
|
|
||
| TEST_F(ControllerScriptEngineLegacyTest, convertCharsetUnsupportedChars) { | ||
| auto result = qjsvalue_cast<QByteArray>( | ||
| evaluate("engine.convertCharset('ISO-8859-15', 'مايأ نامز')")); | ||
| char sub = '\x1A'; // ASCII/Latin9 SUB character | ||
| EXPECT_EQ(result, | ||
| QByteArrayView::fromArray( | ||
| {sub, sub, sub, sub, '\x20', sub, sub, sub, sub})); | ||
| } |
There was a problem hiding this comment.
Those tests aren't relevant anymore since it's an internal API now, are they?
There was a problem hiding this comment.
mostly yes. I should translate them to their enum equivalent so we can at least be reasonably sure about the output data.
| TEST_F(ControllerScriptEngineLegacyTest, convertCharsetUndefinedOnUnknownCharset) { | ||
| const auto result = evaluate("engine.convertCharset('NULL', 'Hello!')"); | ||
|
|
||
| EXPECT_EQ(qjsvalue_cast<QByteArray>(result), QByteArrayView("")); | ||
| } |
There was a problem hiding this comment.
yeah... Not quite sure what to do here honestly. It seems that there are implicit conversions to enums happening, which isn't great if it just silently succeeds. I wonder what value plain undefined results in? I'd guess undefined->0->US_ASCII?
Simplified alternative of #13772 which implements the original scope I proposed.
CC @christophehenry @JoergAtGithub