Add serde support for cxx-qt-lib using proxy types and deriving
#1154
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1154 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 71 71
Lines 11967 11967
=========================================
Hits 11967 11967 ☔ View full report in Codecov by Sentry. |
|
This should fix #943 right? |
|
@redstrate I believe so. |
There was a problem hiding this comment.
Hi @jnbooth , thank you for the contribution!
I haven't yet had the time to give this PR a full review, as it's quite large.
However, what I noticed is that there are more manual Deserialize/Serialize implementations than I would like to support in the future.
It is good that some of these already proxy through e.g. QString, however it would be best if we can use serdes from and into attributes for this, as we then can get rid of a lot of boilerplate.
See also: https://serde.rs/container-attrs.html#from
We may even want to implement e.g. From and Into for Vec<>, Hashmap<>, etc. to get rid of the manual container implementations.
| #[cfg(feature = "serde")] | ||
| #[test] | ||
| fn qstringlist_serde() { | ||
| let mut qstringlist = QStringList::default(); | ||
| qstringlist.append(QString::from("element 1")); | ||
| qstringlist.append(QString::from("element 2")); | ||
| assert_eq!(crate::serde_impl::roundtrip(&qstringlist), qstringlist) | ||
| } |
There was a problem hiding this comment.
I'm confused, I can't find the Serialize/Deserialize impl for QStringList. Where is this implemented?
There was a problem hiding this comment.
It's implemented using the containers macro in serde_impl.rs.
| /// Returns the QTime represented by the string, using the format given, or None if the string cannot be parsed. | ||
| pub fn from_string_opt(string: &ffi::QString, format: &ffi::QString) -> Option<Self> { | ||
| let time = ffi::qtime_from_string(string, format); | ||
| if time.is_valid() { | ||
| Some(time) | ||
| } else { | ||
| None | ||
| } | ||
| } |
There was a problem hiding this comment.
Hm, it seems we deviated from our usual best-practice with the previous from_string impl.
Usually we try to adhere to Rust best practice first and expose the Qt-style version of the function with a suffix.
So in this case from_string would return an Option<Self> and from_string_or_default would return Self.
I'm unsure how we should go about it in this case, as changing the original behavior would be a breaking change.
Any ideas @ahayzen-kdab ?
There was a problem hiding this comment.
(Side note: I've noticed in libraries such as chrono that if a function returns Option<Self> rather than Self, they add _opt to the name.)
|
@LeonMatthesKDAB Assuming this is what you wanted me to do, I've replaced handwritten serde implementations with the following new implementations:
However, I think there are some drawbacks to this approach that might make it worth reverting.
|
Per #943, adds serialization and deserialization for a number of types, using various strategies.
Using
#[derive(Serialize, Deserialize)]:QLine/QLineFQMargins/QMarginsFQPoint/QPointFQRect/QRectFQSize/QSizeFQVector2D/QVector3D/QVector4DProxied to
QString:QDate,QDateTime, andQTime(ISO-8601)QFontQUrlQUuidQColor(hex encoding)Using a macro for serializing and deserializing Qt sequential containers:
QListQPolygon/QPolygonFQSetQStringListQVectorThe goal of this PR is to avoid handwritten serde logic as much as possible in order to minimize maintenance cost.
#[derive(Serialize, Deserialize)]does expose field names, but that is intended behavior. (If it's a problem, we can use #[serde(rename)].)QStringproxies are used forQColor,QFont,QUuid,QUrl, and date/time types because a) it offloads ser/de logic to Qt, and b) thechronoanduuidcrates use string proxies for ser/de anyway, so if we used them as proxies, it'd have the same effect. Ser/de has not been added forQHash/QMapbecause they currently useQVariantvalues, which aren't serializable except by turning them into non-human-readable byte arrays. If we add moreQHashPairandQMapPairspecializations, that could be worth revisiting.In order to write round-trip unit tests, there needs to be a dev-dependency for a serde format. Currently the unit tests use serde_json.
Note: This PR also adds the missing
QSet::reserve(&mut self, size: isize)function in order to reserve capacity before deserialization. That's where most of the changed files are coming from. It also adds a few missingDebugimplementations so thatassert_eq!works with those types.Closes #943