Fix caluculation of m_resourcePath in the testing case #14110
Conversation
|
The reason for introducing this is to allow consistency with the QML code. As you can see with the S4 Mk3 PR, advanced mapping will likely consist of multiple QML file at different location, so I would rather allow some consistency on how you can import and reuse Mixxx component. That being said, I agree that the current implementation is flawdd, and we should likely publish a QML module instead, which can be imported, along the lines of |
|
Filed a bug for the pass issue #14151 Ready for merge. |
|
Can we merge this now? |
|
@acolombier What do you think? Merge? |
| @@ -125,7 +125,7 @@ class ControllerScriptEngineLegacy : public ControllerScriptEngineBase { | |||
| std::unordered_map<QString, std::unique_ptr<mixxx::qml::QmlMixxxControllerScreen>> m_rootItems; | |||
| QList<LegacyControllerMapping::QMLModuleInfo> m_modules; | |||
| QList<LegacyControllerMapping::ScreenInfo> m_infoScreens; | |||
| QString m_resourcePath{QStringLiteral(".")}; | |||
There was a problem hiding this comment.
How do you feel about keeping this default till we implement the proper module publishing?
There was a problem hiding this comment.
Here we have a different topic. I have removed the default . because is never used. It is set via setResourcePath() to the correct value. Appart from that . would require to start Mixxx from the res directory
There was a problem hiding this comment.
Yeah, actually this should not break the current behaviour, but I thought it would be best to default the QML path to be ./qml rather than /qml, but since we are calling setResourcePath before loading the scene, I'm happy to ignore that
| @@ -125,7 +125,7 @@ class ControllerScriptEngineLegacy : public ControllerScriptEngineBase { | |||
| std::unordered_map<QString, std::unique_ptr<mixxx::qml::QmlMixxxControllerScreen>> m_rootItems; | |||
| QList<LegacyControllerMapping::QMLModuleInfo> m_modules; | |||
| QList<LegacyControllerMapping::ScreenInfo> m_infoScreens; | |||
| QString m_resourcePath{QStringLiteral(".")}; | |||
There was a problem hiding this comment.
Yeah, actually this should not break the current behaviour, but I thought it would be best to default the QML path to be ./qml rather than /qml, but since we are calling setResourcePath before loading the scene, I'm happy to ignore that
It took me quite a while to figure that out. A related issue is that the path/line reported in the error message is wrong, because of a QML hack:
https://github.com/mixxxdj/mixxx/blob/main/src/controllers/scripting/legacy/controllerscriptenginelegacy.cpp#L763-L765
The hack allows statements like this:
import "." as Skininstead of
import "../qml" as SkinI would prefer to remove the hack and use the later, which feels correct for me.