Add `serde` support for cxx-qt-lib using proxy types and deriving
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/QVector4D
Proxied to QString:
QDate,QDateTime, andQTime(ISO-8601)QFontQUrlQUuidQColor(hex encoding)
Using a macro for serializing and deserializing Qt sequential containers:
QListQPolygon/QPolygonFQSetQStringListQVector
The 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)].) QString proxies are used for QColor, QFont, QUuid, QUrl, and date/time types because a) it offloads ser/de logic to Qt, and b) the chrono and uuid crates 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 for QHash/QMap because they currently use QVariant values, which aren't serializable except by turning them into non-human-readable byte arrays. If we add more QHashPair and QMapPair specializations, 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 missingQSet::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 missing Debug implementations so that assert_eq! works with those types.
Closes #943
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 100.00%. Comparing base (
17c21f6) to head (8985a10).
Additional details and impacted files
@@ Coverage Diff @@
## main #1154 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 73 73
Lines 12527 12527
=========================================
Hits 12527 12527
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
🚀 New features to boost your workflow:
- ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
This should fix #943 right?
@redstrate I believe so.
@LeonMatthesKDAB Assuming this is what you wanted me to do, I've replaced handwritten serde implementations with the following new implementations:
From<&QFont> for QString+From<QFont> for QStringTryFrom<&QString> for QFont+TryFrom<QString> for QFontFrom<QString> for QUrlFrom<&QUrl> for QString+From<QUrl> for QStringFrom<QString> for QUuidFrom<QString> for QColorFrom<&QColor> for QString+From<QColor> for QStringFrom<&QByteArray> for Vec<u8>+From<QByteArray> for Vec<u8>From<Vec<u8>> for QByteArrayTryFrom<&QString> for QDate+TryFrom<QString> for QDateFrom<&QDate> for QString+From<QDate> for QStringTryFrom<&QString> for QDateTime+TryFrom<QString> for QDateTimeFrom<&QDateTime> for QString+From<QDateTime> for QStringTryFrom<&QString> for QTime+TryFrom<QString> for QTimeFrom<&QTime> for QString+From<QTime> for QString
However, I think there are some drawbacks to this approach that might make it worth reverting.
- That's a lot of new trait implementations, and all of the by-value ones are unnecessary except for the sake of serde attributes. Without serde's
fromandinto, there doesn't need to beFrom<QString> for QUrlbecause there's alreadyFrom<&QString>. - All those implementations are tightly coupled to (de)serialization. If someone submits a pull request that changes any of those implementations, it will break deserialization. That's a lot of code fragility. (Fortunately, the serde unit tests will catch those issues, but still.)
- Since serde's
fromandintoattributes use.clone(), every call to serialization now starts with.clone(). It's especially bad in the cases ofQStringandVec<u8>, which now allocate unnecessary strings and vectors. And sinceQStringis used as the proxy for all the above types, that means usinginto = "String"instead of a handwritten serialization function forQStringwill result in unnecessarily allocating a string for every type. - This isn't less code to maintain. It's more.
@jnbooth sorry for the late reply.
I'm currently discussing with @ahayzen-kdab how we want to deal with serde support going forward.
As you outlined, there's more issues with the From/Into implementations than we hoped.
This makes the overall maintenance burden for this higher than expected.
The manual implementations you wrote are good, but as you mention, we need to ensure that any serialized representations remain compatible with future versions, so we want to be careful about how we approach this. Many of these types are hopefully simple enough that we can support manual serde implementations, but we need a consensus on this.
If you want to chat with us directly, also feel free to join our Zulip instance: https://cxx-qt.zulipchat.com/
@jnbooth We posted an idea on how to get this merged on Zulip: https://cxx-qt.zulipchat.com/#narrow/channel/426350-cxx-qt-lib/topic/serde.20support/near/500690173
Let me know if you want me to copy the discussion here on Github.
@LeonMatthesKDAB Thanks for making that channel! No need to copy it here, I can participate in it directly.