Expose track info to JS controllers #14175
Conversation
res/controllers/engine-api.d.ts
Outdated
| declare interface PlayerManager { | ||
| isLoaded(): boolean; | ||
| getTrack(): string; | ||
| getTitle(): string; |
There was a problem hiding this comment.
I don't think these are accessible via JS because those functions are not Q_INVOKABLE. You should however be able to directly access the declared Q_PROPERTYs. So just console.log(player.title) for example.
Also slight misnaming, this is not the playermanager:
| declare interface PlayerManager { | |
| isLoaded(): boolean; | |
| getTrack(): string; | |
| getTitle(): string; | |
| declare interface PlayerInfo { | |
| isLoaded(): boolean; | |
| getTrack(): string; | |
| getTitle(): string; |
There was a problem hiding this comment.
I think the proposed getter is a much safer solution, than direct access to properties.
Please have a look on the other engine functions, which have a Proxy class, that exposes such functions as Q_INVOKABLE.
There was a problem hiding this comment.
No!
afaict this would remove the .connect(handler)! That is a deal breaker. Moreover, we're now unnecessarily duplicating APIs again!
There was a problem hiding this comment.
We need proper abstraction of the stable API from the experimental QML code. How to ensure this without a proper wrapper?
The other issue is that we need to prevent writing to internal strings in Mixxx.
There was a problem hiding this comment.
How to ensure this without a proper wrapper?
QmlPlayerProxy is the wrapper.
The other issue is that we need to prevent writing to internal strings in Mixxx.
Again, what is "internal"? Everything exposed via QmlPlayerProxy should be freely readable and writable. There is no need for a second QmlPlayerProxy copy IMO.
There was a problem hiding this comment.
We need proper abstraction of the stable API from the experimental QML code.
Well, the code has been around long enough. Lets make it stable then... The QML controller screen feature already assumes its stable.
| @@ -31,7 +31,9 @@ constexpr double kBrakeRampToRate = 0.01; | |||
| ControllerScriptInterfaceLegacy::ControllerScriptInterfaceLegacy( | |||
| ControllerScriptEngineLegacy* m_pEngine, const RuntimeLoggingCategory& logger) | |||
| : m_pScriptEngineLegacy(m_pEngine), | |||
| m_logger(logger) { | |||
| m_logger(logger), | |||
| m_pPlayerManagerProxy(mixxx::qml::QmlPlayerManagerProxy::create(m_pEngine->jsEngine().get(), m_pEngine)) | |||
There was a problem hiding this comment.
Still struggling with C++'s pointers. Not sure about m_pEngine->jsEngine().get() here. I feel like ideally I should be using .get() which seem to defeat std::shared_ptr. I though I could use C++ smart pointers (shared_ptr and unique_ptr) as if they were raw pointers but no.
There was a problem hiding this comment.
yeah, I see how thats confusing. In this case its fine though because create does not take the pointer for storing (in fact, its unused, we should double check that). So reaching into the shared_ptr and taking the raw one is completely fine in this case...
| @@ -60,7 +60,7 @@ void QmlPlayerManagerProxy::loadLocationToPlayer( | |||
| } | |||
|
|
|||
| // static | |||
| QmlPlayerManagerProxy* QmlPlayerManagerProxy::create(QQmlEngine* pQmlEngine, QJSEngine* pJsEngine) { | |||
| QmlPlayerManagerProxy* QmlPlayerManagerProxy::create(QJSEngine* pJsEngine, QObject* parent) { | |||
There was a problem hiding this comment.
Changing the order of arguments here to closer match the signature of QmlPlayerManagerProxy.
There was a problem hiding this comment.
Sure just make sure to double check all invocation sites. The compiler may not catch calls with the arguments mistakenly swapped due to the class hierarchy (since QQmlEngine converts implicitly to QJSEngine which also implicitly converts to QObject).
There was a problem hiding this comment.
Also note that the ordering was likely chosen deliberately because it matches the Qt example here: https://doc.qt.io/qt-6/qqmlengine.html#QML_SINGLETON
There was a problem hiding this comment.
Hmm… Wait, it's supposed to be a singleton instance but it doesn't implement the singleton pattern. The create static method doesn't check the existance of a static field like it's done on https://doc.qt.io/qt-6/qqmlengine.html#QML_SINGLETON.
christophehenry
force-pushed
the
expose-track-info-to-mappings
branch
from
dcae1c3 to
330cdc8
Compare
| isLoaded(): boolean; | ||
| track(): string; | ||
| title(): string; | ||
| artist(): string; | ||
| album(): string; | ||
| albumArtist(): string; | ||
| genre(): string; | ||
| composer(): string; | ||
| grouping(): string; | ||
| year(): string; | ||
| trackNumber(): string; | ||
| trackTotal(): string; | ||
| comment(): string; | ||
| keyText(): string; |
There was a problem hiding this comment.
Please add unit tests that:
-Test each function
-Ensure that the test case fails if somebody add a field to PlayerInfo. This is to ensure that nobody expose unintentional objects without implementing test and documentation
There was a problem hiding this comment.
Thats quite a lot of work and repeated code for not a lot of gain IMO...
| /** QmlPlayerProxy */ | ||
| declare interface PlayerInfo { | ||
| isLoaded(): boolean; | ||
| track(): string; |
There was a problem hiding this comment.
What does the string contain returened here? track() is too common to be self explaining.
| grouping(): string; | ||
| year(): string; | ||
| trackNumber(): string; | ||
| trackTotal(): string; |
There was a problem hiding this comment.
trackTotal() is also too common. Is it number of Tracks in the Album?
Following #13935, my proposed solution to expose track infos to JS mappings based on @Swiftb0y's idea.