-
Notifications
You must be signed in to change notification settings - Fork 89
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow using QGuiApplication
and not linking to QtWidgets
#211
base: master
Are you sure you want to change the base?
Conversation
…` instead of `QApplication`
qttypes/Cargo.toml
Outdated
@@ -31,6 +31,8 @@ qtmultimediawidgets = [] | |||
qtsql = [] | |||
# Link against QtTest | |||
qttest = [] | |||
# Don't link QtWidgets and use QGuiApplication | |||
nowidgets = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Rust, features need to be additive.
So this should be a qtwidget
feature and enable it by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But then what about users who already have no-default-features enabled? It would break the build for them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right.
I suppose we will have to increase the semver then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like rust-lang/rfcs#3146 would be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, the change starts to look good.
Since this require a semver version change. I'm wondering about merging this now or waiting a bit to see if there are other needed breaking changes. How much do you require this change?
It's not super important, it'll be just nice to save some space by not having to deploy QtWidgets module when it's not used. It can wait, it's not a blocker for me |
qmetaobject/Cargo.toml
Outdated
chrono_qdatetime = ["qttypes/chrono"] | ||
webengine = ["qttypes/qtwebengine"] | ||
widgets = ["qttypes/qtwidgets"] | ||
|
||
[dependencies] | ||
qttypes = { path = "../qttypes", version = "0.2.5", features = ["qtquick"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qttypes = { path = "../qttypes", version = "0.2.5", features = ["qtquick"] } | |
qttypes = { path = "../qttypes", version = "0.2.5", default-features = false, features = ["required", "qtquick"] } |
This PR is extremely helpful for me: I'm creating an app for the reMarkable e-ink tablet, which only supports QtQuick. However, qttypes
still tries to link to QtWidgets with its default features, which causes an error for me because it doesn't exist.
… as supported, but it's required on Android so add it explicitly
…detected as supported, but it's required on Android so add it explicitly" This reverts commit e359084.
Maybe it's soon time to make a breaking change release so we can merge this. |
This PR adds a new feature flag that allows to skip linking the QtWidgets module and use
QGuiApplication
instead ofQApplication
. I added it as an optional feature that disables the linking to not break any existing code and feature set that users may already have.