-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Salvage #13772 #13935
base: main
Are you sure you want to change the base?
Salvage #13772 #13935
Changes from 4 commits
58d8b98
d131d1c
57007c8
01b80a5
f876724
e2c74a9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,10 @@ | ||
#include "controllerscriptinterfacelegacy.h" | ||
|
||
#if QT_VERSION < QT_VERSION_CHECK(6, 4, 0) | ||
#include <QTextCodec> | ||
#else | ||
#include <QStringEncoder> | ||
#endif | ||
#include <gsl/pointers> | ||
|
||
#include "control/controlobject.h" | ||
|
@@ -1052,3 +1057,55 @@ void ControllerScriptInterfaceLegacy::softStart(int deck, bool activate, double | |
// activate the ramping in scratchProcess() | ||
m_ramp[deck] = true; | ||
} | ||
|
||
QByteArray ControllerScriptInterfaceLegacy::convertCharset( | ||
const ControllerScriptInterfaceLegacy::WellKnownCharsets targetCharset, | ||
const QString& value) { | ||
switch (targetCharset) { | ||
case WellKnownCharsets::US_ASCII: | ||
return convertCharset(QStringLiteral("US-ASCII"), value); | ||
case WellKnownCharsets::Latin1: | ||
case WellKnownCharsets::ISO_8859_1: | ||
return convertCharset(QStringLiteral("ISO-8859-1"), value); | ||
case WellKnownCharsets::Latin9: | ||
case WellKnownCharsets::ISO_8859_15: | ||
return convertCharset(QStringLiteral("ISO-8859-15"), value); | ||
case WellKnownCharsets::UCS2: | ||
case WellKnownCharsets::ISO_10646_UCS_2: | ||
return convertCharset(QStringLiteral("ISO-10646-UCS-2"), value); | ||
case WellKnownCharsets::UTF_8: | ||
return convertCharset(QStringLiteral("UTF-8"), value); | ||
case WellKnownCharsets::UTF_16BE: | ||
return convertCharset(QStringLiteral("UTF-16BE"), value); | ||
case WellKnownCharsets::UTF_16LE: | ||
return convertCharset(QStringLiteral("UTF-16LE"), value); | ||
} | ||
m_pScriptEngineLegacy->logOrThrowError(QStringLiteral("Unknown charset specified")); | ||
return QByteArray(); | ||
} | ||
|
||
QByteArray ControllerScriptInterfaceLegacy::convertCharset( | ||
const QString& targetCharset, const QString& value) { | ||
#if QT_VERSION < QT_VERSION_CHECK(6, 4, 0) | ||
QByteArray encoderNameArray = targetCharset.toUtf8(); | ||
auto* pCodec = QTextCodec::codecForName(encoderNameArray); | ||
if (!pCodec) { | ||
qCWarning(m_logger) << "Unable to open encoder"; | ||
return QByteArray(); | ||
} | ||
return std::unique_ptr(pCodec->makeEncoder())->fromUnicode(value); | ||
#else | ||
#if QT_VERSION >= QT_VERSION_CHECK(6, 8, 0) | ||
QAnyStringView encoderName = QAnyStringView(targetCharset); | ||
#else | ||
QByteArray encoderNameArray = targetCharset.toUtf8(); | ||
const char* encoderName = encoderNameArray.constData(); | ||
#endif | ||
QStringEncoder fromUtf16 = QStringEncoder(encoderName); | ||
if (!fromUtf16.isValid()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why you removed the flags here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see the commit message, writing the replacement char is better than replacing with null bytes IMO. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you sure? I think it depends on the encoding. The Qt docs say they use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
qCWarning(m_logger) << "Unable to open encoder"; | ||
return QByteArray(); | ||
} | ||
return fromUtf16(value); | ||
#endif | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,8 @@ | |
#include <gmock/gmock.h> | ||
#include <gtest/gtest.h> | ||
|
||
#include <QByteArrayView> | ||
#include <QMetaEnum> | ||
#include <QScopedPointer> | ||
#include <QTemporaryFile> | ||
#include <QThread> | ||
|
@@ -12,6 +14,7 @@ | |
|
||
#include "control/controlobject.h" | ||
#include "control/controlpotmeter.h" | ||
#include "controllers/scripting/legacy/controllerscriptinterfacelegacy.h" | ||
#ifdef MIXXX_USE_QML | ||
#include <QQuickItem> | ||
|
||
|
@@ -658,6 +661,84 @@ TEST_F(ControllerScriptEngineLegacyTest, connectionExecutesWithCorrectThisObject | |
EXPECT_DOUBLE_EQ(1.0, pass->get()); | ||
} | ||
|
||
TEST_F(ControllerScriptEngineLegacyTest, convertCharsetUndefinedOnUnknownCharset) { | ||
const auto result = evaluate("engine.convertCharset('NULL', 'Hello!')"); | ||
|
||
EXPECT_EQ(qjsvalue_cast<QByteArray>(result), QByteArrayView("")); | ||
} | ||
Comment on lines
+664
to
+668
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
TEST_F(ControllerScriptEngineLegacyTest, convertCharsetCorrectValueWellKnown) { | ||
const auto result = evaluate( | ||
"engine.convertCharset(engine.WellKnownCharsets.Latin9, 'Hello!')"); | ||
|
||
// ISO-8859-15 ecoded 'Hello!' | ||
EXPECT_EQ(qjsvalue_cast<QByteArray>(result), | ||
QByteArrayView::fromArray({'\x48', '\x65', '\x6c', '\x6c', '\x6f', '\x21'})); | ||
} | ||
|
||
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', 'مايأ نامز')")); | ||
|
||
EXPECT_EQ(result, | ||
QByteArrayView::fromArray( | ||
{'\x00', '\x00', '\x00', '\x00', '\x20', '\x00', '\x00', '\x00', '\x00'})); | ||
} | ||
|
||
#define COMPLICATEDSTRINGLITERAL "Hello, 世界! שלום! こんにちは! 안녕하세요! 😊" | ||
|
||
static int convertedCharsetForString(ControllerScriptInterfaceLegacy::WellKnownCharsets charset) { | ||
// the expected length after conversion of COMPLICATEDSTRINGLITERAL | ||
using enum ControllerScriptInterfaceLegacy::WellKnownCharsets; | ||
switch (charset) { | ||
case US_ASCII: | ||
case Latin9: | ||
case ISO_8859_15: | ||
return 32; | ||
case Latin1: | ||
case ISO_8859_1: | ||
return 33; | ||
case UTF_8: | ||
return 63; | ||
case UTF_16BE: | ||
case UTF_16LE: | ||
return 66; | ||
case UCS2: | ||
case ISO_10646_UCS_2: | ||
return 68; | ||
} | ||
// unreachable (TODO assert false?) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That assert would make sense IMO There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, still TODO |
||
return 0; | ||
} | ||
|
||
TEST_F(ControllerScriptEngineLegacyTest, convertCharsetAllWellKnownCharsets) { | ||
QMetaEnum charsetEnumEntry = QMetaEnum::fromType< | ||
ControllerScriptInterfaceLegacy::WellKnownCharsets>(); | ||
|
||
for (int i = 0; i < charsetEnumEntry.keyCount(); ++i) { | ||
QString key = charsetEnumEntry.key(i); | ||
auto enumValue = | ||
static_cast<ControllerScriptInterfaceLegacy::WellKnownCharsets>( | ||
charsetEnumEntry.value(i)); | ||
QString source = QStringLiteral( | ||
"engine.convertCharset(engine.WellKnownCharsets.%1, " | ||
"'" COMPLICATEDSTRINGLITERAL "')") | ||
.arg(key); | ||
auto result = qjsvalue_cast<QByteArray>(evaluate(source)); | ||
EXPECT_EQ(result.size(), convertedCharsetForString(enumValue)) | ||
<< "Unexpected length of converted string for encoding: '" | ||
<< key.toStdString() << "'"; | ||
} | ||
} | ||
|
||
#ifdef MIXXX_USE_QML | ||
class MockScreenRender : public ControllerRenderingEngine { | ||
public: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait this is still documented even though it's now an internal API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forgot to remove. thanks for the reminder.