QImage: Add conversion from image::RgbaImage #1157
Conversation
LeonMatthesKDAB
force-pushed
the
qimage-constructors
branch
from
4f238f2 to
955d8c4
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1157 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 71 71
Lines 11967 11967
=========================================
Hits 11967 11967 ☔ View full report in Codecov by Sentry. |
LeonMatthesKDAB
force-pushed
the
qimage-constructors
branch
4 times, most recently
from
3bc2f5e to
15c2008
Compare
LeonMatthesKDAB
added
the
backport-candidate
LeonMatthesKDAB
force-pushed
the
qimage-constructors
branch
from
15c2008 to
47f440f
Compare
|
Looks good, small questions and conflicts |
LeonMatthesKDAB
force-pushed
the
qimage-constructors
branch
from
47f440f to
cced1e5
Compare
|
Needs some conflicts resolving otherwise looks good i think. |
crates/cxx-qt-lib/src/gui/qimage.rs
Outdated
| format: QImageFormat, | ||
| ) -> Self { | ||
| extern "C" fn delete_boxed_vec(boxed_vec: *mut ffi::c_void) { | ||
| drop(Box::new(boxed_vec as *mut Vec<u8>)); |
There was a problem hiding this comment.
this looks bogus to me, can you explain it please? isn't Box::new() like a std::make_unique, meaning it does a new memory allocation? Or is this a misnomer and it actually adopts the ptr that gets passed in, so more like std::unique_ptr<T>(...)?
There was a problem hiding this comment.
Very good catch, thank you!
It needs to be Box::from_raw. Should be fixed now.
There was a problem hiding this comment.
Curious that valgrind didn't spot this, or are only the Qt unit tests not the cargo test run under valgrind ? Maybe this should be added to the qt_types_standalone or we should run the cargo test (even just for cxx-qt-lib) under valgrind ? 🤔
There was a problem hiding this comment.
I just checked, CI indeed did not find the issue.
It seems to be that we really don't run cargo test under valgrind, but just the tests that we run via C++ (e.g. qml_features_test, etc.)
There was a problem hiding this comment.
We could try using: https://github.com/jfrimmel/cargo-valgrind
But that would probably be better served in a different PR, as that means we might want to remove/reduce the qt_types_standalone tests as well 🤔
There was a problem hiding this comment.
as an outsider: can you use sanitizers instead? they are much faster and also have leak checking built-in - that should be preferred over valgrind nowadays
There was a problem hiding this comment.
Maybe ? Could be something to investigate if they can be enabled easily in our Cargo and CMake setups :-)
Because the image crate changes versions somewhat frequently, the features in CXX-Qt are added with explicit image crate versions.
LeonMatthesKDAB
force-pushed
the
qimage-constructors
branch
from
cced1e5 to
d72f971
Compare
Because the image crate changes versions somewhat frequently, the
features in CXX-Qt are added with explicit image crate versions.