From bec52b46b33a99e458bb852ded8f2c21a836eb02 Mon Sep 17 00:00:00 2001 From: Mike Solar Date: Fri, 17 Jul 2026 08:38:25 +0800 Subject: [PATCH] nodes: fix serialized ID typos with backward compatibility - ChromaKey tolerance inputs are now spelled "upper/lower_tolerance_in"; old projects with the misspelled "tolerence" IDs keep loading (values and connections) via a new Node::GetInputIDForLegacyID() hook applied in LoadInput and connection deserialization - flip/ripple/swirl/tile/wave node IDs moved to the consistent org.olivevideoeditor.Olive.* domain; NodeFactory::CreateFromID maps the old org.oliveeditor.* IDs so old projects still resolve - Regression tests: legacy ChromaKey IDs (values + connections) and legacy factory IDs --- app/node/distort/flip/flipdistortnode.cpp | 2 +- app/node/distort/ripple/rippledistortnode.cpp | 2 +- app/node/distort/swirl/swirldistortnode.cpp | 2 +- app/node/distort/tile/tiledistortnode.cpp | 2 +- app/node/distort/wave/wavedistortnode.cpp | 2 +- app/node/factory.cpp | 20 +++++++++- app/node/keying/chromakey/chromakey.cpp | 17 ++++++++- app/node/keying/chromakey/chromakey.h | 4 ++ app/node/node.cpp | 12 ++++++ app/node/node.h | 8 ++++ tests/gtest/node_distort_test.cpp | 15 +++----- tests/gtest/node_save_load_test.cpp | 38 +++++++++++++++++++ tests/gtest/project_factory_test.cpp | 33 ++++++++++++++++ 13 files changed, 139 insertions(+), 18 deletions(-) diff --git a/app/node/distort/flip/flipdistortnode.cpp b/app/node/distort/flip/flipdistortnode.cpp index c666c63d8..0711fe77b 100644 --- a/app/node/distort/flip/flipdistortnode.cpp +++ b/app/node/distort/flip/flipdistortnode.cpp @@ -50,7 +50,7 @@ QString FlipDistortNode::Name() const QString FlipDistortNode::id() const { - return QStringLiteral("org.oliveeditor.Olive.flip"); + return QStringLiteral("org.olivevideoeditor.Olive.flip"); } QVector FlipDistortNode::Category() const diff --git a/app/node/distort/ripple/rippledistortnode.cpp b/app/node/distort/ripple/rippledistortnode.cpp index 4a0244037..e7e3065dc 100644 --- a/app/node/distort/ripple/rippledistortnode.cpp +++ b/app/node/distort/ripple/rippledistortnode.cpp @@ -67,7 +67,7 @@ QString RippleDistortNode::Name() const QString RippleDistortNode::id() const { - return QStringLiteral("org.oliveeditor.Olive.ripple"); + return QStringLiteral("org.olivevideoeditor.Olive.ripple"); } QVector RippleDistortNode::Category() const diff --git a/app/node/distort/swirl/swirldistortnode.cpp b/app/node/distort/swirl/swirldistortnode.cpp index 6c5d10120..6fb0d3d94 100644 --- a/app/node/distort/swirl/swirldistortnode.cpp +++ b/app/node/distort/swirl/swirldistortnode.cpp @@ -61,7 +61,7 @@ QString SwirlDistortNode::Name() const QString SwirlDistortNode::id() const { - return QStringLiteral("org.oliveeditor.Olive.swirl"); + return QStringLiteral("org.olivevideoeditor.Olive.swirl"); } QVector SwirlDistortNode::Category() const diff --git a/app/node/distort/tile/tiledistortnode.cpp b/app/node/distort/tile/tiledistortnode.cpp index 880a88b28..f535a2c02 100644 --- a/app/node/distort/tile/tiledistortnode.cpp +++ b/app/node/distort/tile/tiledistortnode.cpp @@ -69,7 +69,7 @@ QString TileDistortNode::Name() const QString TileDistortNode::id() const { - return QStringLiteral("org.oliveeditor.Olive.tile"); + return QStringLiteral("org.olivevideoeditor.Olive.tile"); } QVector TileDistortNode::Category() const diff --git a/app/node/distort/wave/wavedistortnode.cpp b/app/node/distort/wave/wavedistortnode.cpp index f5e76dfdf..1ba41e722 100644 --- a/app/node/distort/wave/wavedistortnode.cpp +++ b/app/node/distort/wave/wavedistortnode.cpp @@ -54,7 +54,7 @@ QString WaveDistortNode::Name() const QString WaveDistortNode::id() const { - return QStringLiteral("org.oliveeditor.Olive.wave"); + return QStringLiteral("org.olivevideoeditor.Olive.wave"); } QVector WaveDistortNode::Category() const diff --git a/app/node/factory.cpp b/app/node/factory.cpp index 0cd4c5caa..c91a9303d 100644 --- a/app/node/factory.cpp +++ b/app/node/factory.cpp @@ -22,6 +22,7 @@ #include "factory.h" #include +#include #include "audio/pan/pan.h" #include "audio/volume/volume.h" @@ -224,8 +225,25 @@ QString NodeFactory::GetNameFromID(const QString &id) Node *NodeFactory::CreateFromID(const QString &id) { + QString resolved_id = id; + + // Node IDs renamed after older project files were written + static const QHash kLegacyIDs = { + { QStringLiteral("org.oliveeditor.Olive.flip"), + QStringLiteral("org.olivevideoeditor.Olive.flip") }, + { QStringLiteral("org.oliveeditor.Olive.ripple"), + QStringLiteral("org.olivevideoeditor.Olive.ripple") }, + { QStringLiteral("org.oliveeditor.Olive.swirl"), + QStringLiteral("org.olivevideoeditor.Olive.swirl") }, + { QStringLiteral("org.oliveeditor.Olive.tile"), + QStringLiteral("org.olivevideoeditor.Olive.tile") }, + { QStringLiteral("org.oliveeditor.Olive.wave"), + QStringLiteral("org.olivevideoeditor.Olive.wave") }, + }; + resolved_id = kLegacyIDs.value(id, id); + foreach (Node *n, library_) { - if (n->id() == id) { + if (n->id() == resolved_id) { return n->copy(); } } diff --git a/app/node/keying/chromakey/chromakey.cpp b/app/node/keying/chromakey/chromakey.cpp index eaf5bf9fd..dcf0cdc24 100644 --- a/app/node/keying/chromakey/chromakey.cpp +++ b/app/node/keying/chromakey/chromakey.cpp @@ -28,9 +28,9 @@ const QString ChromaKeyNode::kColorInput = QStringLiteral("color_key"); const QString ChromaKeyNode::kMaskOnlyInput = QStringLiteral("mask_only_in"); const QString ChromaKeyNode::kInvertInput = QStringLiteral("invert_in"); const QString ChromaKeyNode::kUpperToleranceInput = - QStringLiteral("upper_tolerence_in"); + QStringLiteral("upper_tolerance_in"); const QString ChromaKeyNode::kLowerToleranceInput = - QStringLiteral("lower_tolerence_in"); + QStringLiteral("lower_tolerance_in"); const QString ChromaKeyNode::kGarbageMatteInput = QStringLiteral("garbage_in"); const QString ChromaKeyNode::kCoreMatteInput = QStringLiteral("core_in"); const QString ChromaKeyNode::kShadowsInput = QStringLiteral("shadows_in"); @@ -163,4 +163,17 @@ void ChromaKeyNode::ConfigChanged() GenerateProcessor(); } +QString ChromaKeyNode::GetInputIDForLegacyID(const QString &id) const +{ + // Older project files used the misspelled "tolerence" input IDs + if (id == QStringLiteral("upper_tolerence_in")) { + return kUpperToleranceInput; + } + if (id == QStringLiteral("lower_tolerence_in")) { + return kLowerToleranceInput; + } + + return super::GetInputIDForLegacyID(id); +} + } // namespace olive diff --git a/app/node/keying/chromakey/chromakey.h b/app/node/keying/chromakey/chromakey.h index 3b163a55d..bdfa6e8c8 100644 --- a/app/node/keying/chromakey/chromakey.h +++ b/app/node/keying/chromakey/chromakey.h @@ -46,6 +46,10 @@ public: virtual void ConfigChanged() override; + // Maps the misspelled tolerance input IDs from old project files onto the + // corrected ones + virtual QString GetInputIDForLegacyID(const QString &id) const override; + static const QString kColorInput; static const QString kInvertInput; static const QString kMaskOnlyInput; diff --git a/app/node/node.cpp b/app/node/node.cpp index 5164ffd03..8056acfda 100644 --- a/app/node/node.cpp +++ b/app/node/node.cpp @@ -1351,6 +1351,10 @@ bool Node::Load(QXmlStreamReader *reader, SerializedData *data) } } + // Translate IDs renamed after older project files were + // written + param_id = GetInputIDForLegacyID(param_id); + QString output_node_id; while (XMLReadNextStartElement(reader)) { @@ -1573,6 +1577,11 @@ void Node::PostLoadEvent(SerializedData *data) } } +QString Node::GetInputIDForLegacyID(const QString &id) const +{ + return id; +} + bool Node::LoadInput(QXmlStreamReader *reader, SerializedData *data) { if (dynamic_cast(this)) { @@ -1598,6 +1607,9 @@ bool Node::LoadInput(QXmlStreamReader *reader, SerializedData *data) return false; } + // Translate IDs renamed after older project files were written + param_id = GetInputIDForLegacyID(param_id); + if (!this->HasInputWithID(param_id)) { qWarning() << "Failed to load parameter that didn't exist:" << param_id; reader->skipCurrentElement(); diff --git a/app/node/node.h b/app/node/node.h index 1b15afdc5..697a2b0bb 100644 --- a/app/node/node.h +++ b/app/node/node.h @@ -1094,6 +1094,14 @@ public: bool LoadInput(QXmlStreamReader *reader, SerializedData *data); void SaveInput(QXmlStreamWriter *writer, const QString &id) const; + /** + * @brief Maps an input ID read from an old project file to its current ID + * + * Nodes whose input IDs have been renamed override this so old projects + * keep loading. The default implementation returns the ID unchanged. + */ + virtual QString GetInputIDForLegacyID(const QString &id) const; + bool LoadImmediate(QXmlStreamReader *reader, const QString &input, int element, SerializedData *data); void SaveImmediate(QXmlStreamWriter *writer, const QString &input, diff --git a/tests/gtest/node_distort_test.cpp b/tests/gtest/node_distort_test.cpp index 5a6395c3e..d1029d7a0 100644 --- a/tests/gtest/node_distort_test.cpp +++ b/tests/gtest/node_distort_test.cpp @@ -660,9 +660,8 @@ TEST(FlipDistortNode, MetadataIsCorrect) { olive::FlipDistortNode node; // NOTE: unlike most Olive nodes ("org.olivevideoeditor.Olive.*"), the - // flip/ripple/swirl/tile/wave nodes use the "org.oliveeditor.Olive.*" // domain (inconsistent ID, documented here as a suspected bug) - EXPECT_EQ(node.id(), QStringLiteral("org.oliveeditor.Olive.flip")); + EXPECT_EQ(node.id(), QStringLiteral("org.olivevideoeditor.Olive.flip")); EXPECT_EQ(node.Name(), QStringLiteral("Flip")); EXPECT_FALSE(node.Description().isEmpty()); EXPECT_TRUE(node.Category().contains(olive::Node::kCategoryDistort)); @@ -1226,8 +1225,7 @@ TEST(MaskDistortNode, ValueWithFeatherNestsBlurJob) TEST(RippleDistortNode, MetadataIsCorrect) { olive::RippleDistortNode node; - // NOTE: "org.oliveeditor.*" domain, inconsistent with most Olive nodes - EXPECT_EQ(node.id(), QStringLiteral("org.oliveeditor.Olive.ripple")); + EXPECT_EQ(node.id(), QStringLiteral("org.olivevideoeditor.Olive.ripple")); EXPECT_EQ(node.Name(), QStringLiteral("Ripple")); EXPECT_FALSE(node.Description().isEmpty()); EXPECT_TRUE(node.Category().contains(olive::Node::kCategoryDistort)); @@ -1359,8 +1357,7 @@ TEST(RippleDistortNode, ValueWithIntensityPushesShaderJob) TEST(SwirlDistortNode, MetadataIsCorrect) { olive::SwirlDistortNode node; - // NOTE: "org.oliveeditor.*" domain, inconsistent with most Olive nodes - EXPECT_EQ(node.id(), QStringLiteral("org.oliveeditor.Olive.swirl")); + EXPECT_EQ(node.id(), QStringLiteral("org.olivevideoeditor.Olive.swirl")); EXPECT_EQ(node.Name(), QStringLiteral("Swirl")); EXPECT_EQ(node.Description(), QStringLiteral("Distorts an image by swirling it around a center point.")); @@ -1497,8 +1494,7 @@ TEST(SwirlDistortNode, ValueWithAngleAndRadiusPushesShaderJob) TEST(TileDistortNode, MetadataIsCorrect) { olive::TileDistortNode node; - // NOTE: "org.oliveeditor.*" domain, inconsistent with most Olive nodes - EXPECT_EQ(node.id(), QStringLiteral("org.oliveeditor.Olive.tile")); + EXPECT_EQ(node.id(), QStringLiteral("org.olivevideoeditor.Olive.tile")); EXPECT_EQ(node.Name(), QStringLiteral("Tile")); EXPECT_FALSE(node.Description().isEmpty()); EXPECT_TRUE(node.Category().contains(olive::Node::kCategoryDistort)); @@ -1649,8 +1645,7 @@ TEST(TileDistortNode, ValueWithNonUnitScalePushesShaderJob) TEST(WaveDistortNode, MetadataIsCorrect) { olive::WaveDistortNode node; - // NOTE: "org.oliveeditor.*" domain, inconsistent with most Olive nodes - EXPECT_EQ(node.id(), QStringLiteral("org.oliveeditor.Olive.wave")); + EXPECT_EQ(node.id(), QStringLiteral("org.olivevideoeditor.Olive.wave")); EXPECT_EQ(node.Name(), QStringLiteral("Wave")); EXPECT_FALSE(node.Description().isEmpty()); EXPECT_TRUE(node.Category().contains(olive::Node::kCategoryDistort)); diff --git a/tests/gtest/node_save_load_test.cpp b/tests/gtest/node_save_load_test.cpp index 262520f05..271501c02 100644 --- a/tests/gtest/node_save_load_test.cpp +++ b/tests/gtest/node_save_load_test.cpp @@ -16,6 +16,7 @@ #include "node/generator/solid/solid.h" #include "node/generator/text/textv3.h" #include "node/keyframe.h" +#include "node/keying/chromakey/chromakey.h" #include "node/math/math/math.h" #include "node/node.h" #include "node/project.h" @@ -617,3 +618,40 @@ TEST_F(NodeSaveLoadTest, ConnectionsLinksAndPositionsResolveAfterProjectLoad) olive::NodeFactory::Destroy(); } + +TEST_F(NodeSaveLoadTest, LegacyMisspelledChromaKeyIDsAreMapped) +{ + auto *src = AddNode(); + src->SetStandardValue(olive::ChromaKeyNode::kUpperToleranceInput, 42.0); + src->SetStandardValue(olive::ChromaKeyNode::kLowerToleranceInput, 7.0); + + auto *math = AddNode(); + olive::Node::ConnectEdge( + math, olive::NodeInput(src, olive::ChromaKeyNode::kUpperToleranceInput)); + + QString xml = SaveNodeXml(src); + + // Simulate an old project file written with the misspelled "tolerence" IDs + xml.replace(QStringLiteral("upper_tolerance_in"), + QStringLiteral("upper_tolerence_in")); + xml.replace(QStringLiteral("lower_tolerance_in"), + QStringLiteral("lower_tolerence_in")); + + olive::ChromaKeyNode loaded; + olive::SerializedData data; + ASSERT_TRUE(LoadNodeXml(&loaded, xml, &data)); + + EXPECT_DOUBLE_EQ( + loaded.GetStandardValue(olive::ChromaKeyNode::kUpperToleranceInput) + .toDouble(), + 42.0); + EXPECT_DOUBLE_EQ( + loaded.GetStandardValue(olive::ChromaKeyNode::kLowerToleranceInput) + .toDouble(), + 7.0); + + // Connections to the renamed inputs are remapped too + ASSERT_EQ(data.desired_connections.size(), 1); + EXPECT_EQ(data.desired_connections.first().input.input(), + olive::ChromaKeyNode::kUpperToleranceInput); +} diff --git a/tests/gtest/project_factory_test.cpp b/tests/gtest/project_factory_test.cpp index cf22f4d27..395426048 100644 --- a/tests/gtest/project_factory_test.cpp +++ b/tests/gtest/project_factory_test.cpp @@ -587,3 +587,36 @@ TEST(NodeFactory, CreateMenuRestrictedToCategory) olive::NodeFactory::Destroy(); } + +TEST(NodeFactory, LegacyDistortIdsResolveToRenamedNodes) +{ + olive::NodeFactory::Initialize(); + + const QList> legacy_ids = { + { QStringLiteral("org.oliveeditor.Olive.flip"), + QStringLiteral("org.olivevideoeditor.Olive.flip") }, + { QStringLiteral("org.oliveeditor.Olive.ripple"), + QStringLiteral("org.olivevideoeditor.Olive.ripple") }, + { QStringLiteral("org.oliveeditor.Olive.swirl"), + QStringLiteral("org.olivevideoeditor.Olive.swirl") }, + { QStringLiteral("org.oliveeditor.Olive.tile"), + QStringLiteral("org.olivevideoeditor.Olive.tile") }, + { QStringLiteral("org.oliveeditor.Olive.wave"), + QStringLiteral("org.olivevideoeditor.Olive.wave") }, + }; + + for (const auto &pair : legacy_ids) { + std::unique_ptr node( + olive::NodeFactory::CreateFromID(pair.first)); + ASSERT_NE(node, nullptr) << pair.first.toStdString(); + EXPECT_EQ(node->id(), pair.second); + + // The current id resolves directly as well + std::unique_ptr current( + olive::NodeFactory::CreateFromID(pair.second)); + ASSERT_NE(current, nullptr) << pair.second.toStdString(); + EXPECT_EQ(current->id(), pair.second); + } + + olive::NodeFactory::Destroy(); +}