-
Notifications
You must be signed in to change notification settings - Fork 82
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
Add qobject extension trait #1226
base: main
Are you sure you want to change the base?
Add qobject extension trait #1226
Conversation
da2aeeb
to
627a8a6
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1226 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 73 73
Lines 12612 12612
=========================================
Hits 12612 12612 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
e0c231e
to
65f097a
Compare
85550c4
to
0a3410d
Compare
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.
Some nitpicks, and some documentation is missing.
Otherwise looks good :)
// | ||
// SPDX-License-Identifier: MIT OR Apache-2.0 | ||
|
||
use cxx_qt::QObject; |
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.
Please make this a pub use
and add a cxx-qt-lib/qobject.h
header that just includes the header from Qt.
This way we the import follows the usual pattern:
extern "C++" {
include("cxx-qt-lib/qobject.h");
type QObject = cxx_qt_lib::QObject;
}
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.
This breaks an orphan rule, so I think it needs to just be the type def, which is what happens inside the cxx bridge in cxx_qt::QObject.
pub fn parent(self: &QObjectExternal) -> *mut QObjectExternal; | ||
|
||
#[rust_name = "set_parent"] | ||
pub unsafe fn setParent(self: Pin<&mut QObjectExternal>, parent: *mut QObjectExternal); |
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.
You should be able to use parent: *mut QObject
here and re-import the QObject from cxx_qt.
type QString = crate::QString; | ||
|
||
#[rust_name = "block_signals"] | ||
pub fn blockSignals(self: Pin<&mut QObjectExternal>, block: bool) -> bool; |
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.
Nit: You can use the shorthand syntax self: Pin<&mut Self>
/&self
here if you move the QString import into a separate extern "C++"
block.
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.
Also a good reminder that we don't support this in CXX-Qt yet...
I've opened an issue for it: #1245
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.
Can fix once #1249 gets merged
0a3410d
to
2f5dae6
Compare
- cxx-qt-lib trait for additional qobject wrappers - adds blockSignals - adds signalsBlocked - adds setObjectName - adds objectName - adds parent - adds setParent
2f5dae6
to
0fc5b11
Compare
Closes #562
The added methods are: