diff --git a/dev/ideas/Project-File-Refactoring.md b/dev/ideas/Project-File-Refactoring.md index e1c7b8a6d..3e643eb54 100644 --- a/dev/ideas/Project-File-Refactoring.md +++ b/dev/ideas/Project-File-Refactoring.md @@ -1,212 +1,213 @@ This refactoring proposal can solve the following issues: * Fix serialisation[^1] issues like #78 and countless other issues * Store multiple timelines in one project (#228, #226, #227) * Simplify access to project properties in code and at the same time prevent some mistakes * Should solve project file recovery issue like #705 It touches the following areas: * Design of the Kdenlive project object * File format + schema of Kdenlive project files Things to keep in mind: * Backwards compatibility – load project files of the old format * Kdenlive can add other Kdenlive projects to the project bin (kind of nested timelines) (#226) [^1]: Serialisation = creating a Kdenlive project file; loading = deserialisation Design a new way to save and load projects. It should * allow unit testing (load example project files and check that they are loaded correctly, upgraded correctly, etc.) * have a well-defined format (always `.` as decimal point, for example) * allow updating projects to new versions * allow easy access to all project information (e.g. Kdenlive properties like timeline ruler position, track/timeline configuration, etc.) * allow for other serializers (e.g. for MLT) The current project format (as of 20.04) is an MLT compatible XML file and Kdenlive properties are stored in the kdenlive namespace. When loading the project file, only a few properties are extracted and the rest of the XML DOM is parsed in various places within the application. This leads to difficulties when e.g. dealing with decimal separator issues as it has to be considered in every single place where the DOM is parsed and not just in the deserialiser. Possibly useful links: * Serialisation Proxy Pattern https://blog.codefx.org/design/patterns/serialization-proxy-pattern/ * Serialisation Proxy explained for Java https://www.youtube.com/watch?v=V1vQf4qyMXg&feature=youtu.be&t=56m12s * Random issue with information on Serialisation Proxy and Memento Pattern https://github.com/triplea-game/triplea/issues/814 ## Current Issues * Kdenlive internal properties are stored as metadata of the XML file. This requires two workarounds: 1. to store it as metadata which is ignored by MLT, and 2. to convert the strings to actual values * Some data is coming from MLT directly ([service properties][mlt-props]) and we currently do not control (reliably enough?) what number format is used. Example code accessing the FPS property from MLT directly, which led to an error in #687: ```c++ double ClipController::originalFps() const { // […] QString propertyName = QStringLiteral("meta.media.%1.stream.frame_rate").arg(m_videoIndex); return m_properties->get_double(propertyName.toUtf8().constData()); } ``` ## Now vs. Future The way project files are handled currently is that the serialised form (XML) is the model at the same time. This probably made sense in the early days, but now poses many limitations. ![image](uploads/deca45c6ccf29dc4ae1c61f92f29916e/image.png) -A standard way to load saved projects is to parse it (from whatever format it is) and then create an in-memory representation of it which is very easy to work with for developers. This approach has several advantage besides usability for developers. One is that de-serialisation (loading the project file) is happening in a single place and all issues (like decimal separator, document version updates, validations) are done in that step. Afterwards, the developer does not need to worry about reading XML DOM nodes in order to get a property, but simply access the data model in memory. +A standard way to load saved projects is to parse it (from whatever format it is) and then create an in-memory representation of it which is very easy to work with for developers. Comment from Alcinos: This is essentially what happens today already, in our model files. See https://invent.kde.org/multimedia/kdenlive/-/blob/master/src/timeline2/model/builders/meltBuilder.cpp +This approach has several advantage besides usability for developers. One is that de-serialisation (loading the project file) is happening in a single place and all issues (like decimal separator, document version updates, validations) are done in that step. Afterwards, the developer does not need to worry about reading XML DOM nodes in order to get a property, but simply access the data model in memory. ![image](uploads/fca9607dd572a66c3dfd90304662ed5f/image.png) ## Proposed changes ### Properties Define properties explicitly instead of storing them in a QMap (where every value is converted to a string and back). Currently there is `KdenliveDoc::getDocumentProperty(const QString &name, const QString &defaultValue)` which is abused for everything. Before ```c++ QPoint KdenliveDoc::zoom() const { return QPoint(m_documentProperties.value(QStringLiteral("zoom")).toInt(), m_documentProperties.value(QStringLiteral("verticalzoom")).toInt()); } ``` After ```c++ QPoint KdenliveDoc::zoom() const { return m_zoom; } ``` This can be done separately from the other changes. ### Wrap access to MLT (TBD if that makes sense) When transferring XML data between Kdenlive and MLT, do this via a wrapper function to ensure the locale is always set to LC_NUMERIC so the decimal point is well-defined. The wrapper could also be extended to solve problems like [this one](https://bugs.kde.org/show_bug.cgi?id=419603). ### Do not use MLT XML Change the project file from MLT XML to an own format so we do not need to encode our own settings. The format should be defined in a way that does not require number/string conversion (this is possible with XML, JSON, etc. when used correctly). Before ```c++ const QByteArray KdenliveDoc::getProjectXml() { const QByteArray result = m_document.toString().toUtf8(); // We don't need the xml data anymore, throw away m_document.clear(); return result; } ``` After (±) ```c++ const QByteArray KdenliveDocMltSerialiser::getMltXml(KdenliveDoc doc) { return serialiseForMlt(doc); } ``` ## Feedback round 1 The `.kdenlive` file is deliberately an MLT XML file and not a separate file format which needs to be translated to MLT XML because the translation would add another layer – the largest part of the XML is coming from MLT directly and if there are bugs, they should be fixed in MLT. Next step to fix the decimal separator issue is to run some tests on Linux and Windows with different locale settings to see if the decimal separator can be controlled. * Check if locale settings in Kdenlive affect the locale of libraries when they are called * Test library specific locale settings (e.g. in MLT) → https://invent.kde.org/eugster/decimal-separator ### Notes Typical possibly problematic code → replace the string map ```c++ void ClipPropertiesController::slotAspectValueChanged(int) { auto *spin = findChild(QStringLiteral("force_aspect_num_value")); auto *spin2 = findChild(QStringLiteral("force_aspect_den_value")); if ((spin == nullptr) || (spin2 == nullptr)) { return; } QMap properties; properties.insert(QStringLiteral("force_aspect_den"), QString::number(spin2->value())); properties.insert(QStringLiteral("force_aspect_num"), QString::number(spin->value())); QLocale locale; locale.setNumberOptions(QLocale::OmitGroupSeparator); properties.insert(QStringLiteral("force_aspect_ratio"), locale.toString((double)spin->value() / spin2->value())); emit updateClipProperties(m_id, m_originalProperties, properties); m_originalProperties = properties; } ``` * Probably most occurrences of `QLocale` should be checked. * Check comma occurrences in reported example project files * Test current code for OSs ```c++ #ifndef Q_OS_WIN char *separator = localeconv()->decimal_point; if (QString::fromUtf8(separator) != QChar(systemLocale.decimalPoint())) { // qCDebug(KDENLIVE_LOG)<<"------\n!!! system locale is not similar to Qt's locale... be prepared for bugs!!!\n------"; // HACK: There is a locale conflict, so set locale to C // Make sure to override exported values or it won't work qputenv("LANG", "C"); #ifndef Q_OS_MAC setlocale(LC_NUMERIC, "C"); #else setlocale(LC_NUMERIC_MASK, "C"); #endif systemLocale = QLocale::c(); } #endif ``` Occurrences in .kdenlive files ```text #78 (19.03.90) 29,9692 29,97 00:00:05,000 00:00:05,000 00:00:00,033 , # in 00:11:53,167=0 0 1920 1080 1;00:12:09,267=-904 -621 3840 2160 1 00:11:53,167=0;00:12:09,267=0 #78 (18.12.1) 59,9401 # frei0r.defish0r 0,166 #606 (20.07.70) # frei0r.levels 0,3 # opencv.tracker 00:00:07,240~=6 177 1085 310 0;00:00:07,280~=6 177 1085 310 0;00:00:07,320~=5 […] 0 #406488 (19.04.0) ``` [mlt-props]: https://www.mltframework.org/docs/framework/#service-properties \ No newline at end of file