Skip to content

Conversation

@timsueberkrueb
Copy link
Contributor

I'm new to qmetaobject-rs so please correct me if this should be done differently.

@ogoffart
Copy link
Member

ogoffart commented Jan 6, 2019

Thanks for the pull request! This looks good.

It would be nice if this did not require a dependency over qtquickcontrols2.
For example, this could be done with std::env::set_var("QT_QUICK_CONTROLS_STYLE", style)
another alternative would be to add and api in QmlEngine to be able to pass arguments to the QApplication (constructed by QmlEngineHolder)

Since all the api of QQuickStyle are static anyway, a struct does not really make sense in the rust API, it could just be functions in a module.

@timsueberkrueb
Copy link
Contributor Author

Okay, will do, thanks for the feedback!

@timsueberkrueb
Copy link
Contributor Author

Which one would you prefer? Using the envvar or adding it to QmlEngine? I personally would prefer the QQuickStyle version as it mirrors the Qt api => therefore less surprising.

@ogoffart
Copy link
Member

ogoffart commented Jan 6, 2019

You are right, mimicking the QQuickStyle API is probably the best

@timsueberkrueb
Copy link
Contributor Author

timsueberkrueb commented Jan 6, 2019

Done. I'm using QString because it matches the Qt API most closely. Is this okay or should I use &str? In this case it involves unnecessary overhead. EDIT: Or should it be &QString?

@timsueberkrueb
Copy link
Contributor Author

Unrelated: I think it might also make sense to use a binding generator to offer a more complete binding set on top of qmetaobject-rs, perhaps as a different crate. What do you think?

@ogoffart
Copy link
Member

ogoffart commented Jan 6, 2019

yes, &str is maybe best.

I think it might also make sense to use a binding generator to offer a more complete binding set

The problem with binding generator is that it is hard to automatically derive a proper safe rust API out of a C++ API automatically.

@ogoffart ogoffart merged commit ab4b8c1 into woboq:master Jan 6, 2019
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.

2 participants