Skip to content

feat: QQmlExtensionPlugin::initialize_engine#331

Open
rubdos wants to merge 3 commits intowoboq:masterfrom
rubdos:qqmlextension-initialize-engine
Open

feat: QQmlExtensionPlugin::initialize_engine#331
rubdos wants to merge 3 commits intowoboq:masterfrom
rubdos:qqmlextension-initialize-engine

Conversation

@rubdos
Copy link
Copy Markdown
Collaborator

@rubdos rubdos commented Apr 12, 2026

This introduces initialize_engine, such that plugins can register e.g. image providers (might file a PR for that too, but I've been using that with cpp! macros for now for a long time). These need access to the engine, and that can be done through this "callback".

This is 100% unsound, and doesn't compile (because Rust QmlEngine != C++ QQmlEngine etc), but filing to start the discussion. The problem is that QmlEngine, the Rust side, assumes ownership of the pointer. We could leak the pointer after using it, but if we then panic in the initializeEngine override, the pointer will get cleaned up. It's not ideal, and would love some suggestions.

GPT 5.4 suggests introducing a QmlEngineRef type, but that gets ugly quite fast because you need to duplicate all the methods (or break interface and call AsRef all the time), and it just doesn't feel right. Maybe we can track an "owned" flag in the struct, but that neither feels great. Any ideas, @ratijas / @ogoffart ?

Copy link
Copy Markdown
Member

@ogoffart ogoffart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it would help to also have example/tests to see how this is meant to be used.

I'd say there could be a ManualyDrop<QQmlEngine> and only pass references to it, or something like that so that the Rust code doesn't take ownership, and only pass `&QQmlEngine from the function

IMHO all the function on QQmlEngine should probably take &self anyway instead of &mut self that was a design mistake as i didn't understand rust when i did that.

@rubdos
Copy link
Copy Markdown
Collaborator Author

rubdos commented Apr 13, 2026

I think the main problem here is that in a QQmlExtensionPlugin, we just don't have the QApplication/QQuickView to pass around, so we can't even construct the QmlEngineHolder / QmlEngine. Creating the additional QQmlEngine wrapper will make it more confusing...

Is it worth to break API (since we were talking about that anyway), and to rename Rust's QmlEngine to "QmlEngineApplication" or "QmlEngineHolder" directly (the C++ name), and impl Deref<Target=QQmlEngine> for QmlEngineHolder?

@rubdos rubdos force-pushed the qqmlextension-initialize-engine branch from 8384370 to deaffb1 Compare April 18, 2026 14:07
@ratijas
Copy link
Copy Markdown
Collaborator

ratijas commented Apr 18, 2026

Is it worth to break API (since we were talking about that anyway), and to rename Rust's QmlEngine to "QmlEngineApplication" or "QmlEngineHolder" directly (the C++ name), and impl Deref<Target=QQmlEngine> for QmlEngineHolder?

Whatever suits you, I guess. It's not like there is a ton of users to care about porting. As long as version is bumped

@rubdos rubdos marked this pull request as ready for review April 18, 2026 15:56
@rubdos
Copy link
Copy Markdown
Collaborator Author

rubdos commented Apr 18, 2026

Is it worth to break API (since we were talking about that anyway), and to rename Rust's QmlEngine to "QmlEngineApplication" or "QmlEngineHolder" directly (the C++ name), and impl Deref<Target=QQmlEngine> for QmlEngineHolder?

Whatever suits you, I guess. It's not like there is a ton of users to care about porting. As long as version is bumped

When I wrote that, I didn't yet notice that QmlEngine != QQmlEngine. The current patch works, and only slightly breaks the API... I'm not even sure if it really breaks it; I moved a bunch of methods to the Deref'd implementation, and I suppose this only breaks when literally referring to e.g. QmlEngine::set_property instead of calling the function itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants