cxx-qt icon indicating copy to clipboard operation
cxx-qt copied to clipboard

Add `serde` support for cxx-qt-lib using proxy types and deriving

Open jnbooth opened this issue 11 months ago • 7 comments

Per #943, adds serialization and deserialization for a number of types, using various strategies.

Using #[derive(Serialize, Deserialize)]:

  • QLine / QLineF
  • QMargins / QMarginsF
  • QPoint / QPointF
  • QRect / QRectF
  • QSize / QSizeF
  • QVector2D / QVector3D / QVector4D

Proxied to QString:

  • QDate, QDateTime, and QTime (ISO-8601)
  • QFont
  • QUrl
  • QUuid
  • QColor (hex encoding)

Using a macro for serializing and deserializing Qt sequential containers:

  • QList
  • QPolygon / QPolygonF
  • QSet
  • QStringList
  • QVector

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

jnbooth avatar Jan 12 '25 02:01 jnbooth

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.

codecov[bot] avatar Jan 12 '25 02:01 codecov[bot]

This should fix #943 right?

redstrate avatar Jan 16 '25 19:01 redstrate

@redstrate I believe so.

jnbooth avatar Jan 17 '25 21:01 jnbooth

@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 QString
  • TryFrom<&QString> for QFont + TryFrom<QString> for QFont
  • From<QString> for QUrl
  • From<&QUrl> for QString + From<QUrl> for QString
  • From<QString> for QUuid
  • From<QString> for QColor
  • From<&QColor> for QString + From<QColor> for QString
  • From<&QByteArray> for Vec<u8> + From<QByteArray> for Vec<u8>
  • From<Vec<u8>> for QByteArray
  • TryFrom<&QString> for QDate + TryFrom<QString> for QDate
  • From<&QDate> for QString + From<QDate> for QString
  • TryFrom<&QString> for QDateTime + TryFrom<QString> for QDateTime
  • From<&QDateTime> for QString + From<QDateTime> for QString
  • TryFrom<&QString> for QTime + TryFrom<QString> for QTime
  • From<&QTime> for QString + From<QTime> for QString

However, I think there are some drawbacks to this approach that might make it worth reverting.

  1. 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 from and into, there doesn't need to be From<QString> for QUrl because there's already From<&QString>.
  2. 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.)
  3. Since serde's from and into attributes use .clone(), every call to serialization now starts with .clone(). It's especially bad in the cases of QString and Vec<u8>, which now allocate unnecessary strings and vectors. And since QString is used as the proxy for all the above types, that means using into = "String" instead of a handwritten serialization function for QString will result in unnecessarily allocating a string for every type.
  4. This isn't less code to maintain. It's more.

jnbooth avatar Jan 22 '25 22:01 jnbooth

@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/

LeonMatthesKDAB avatar Feb 12 '25 14:02 LeonMatthesKDAB

@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 avatar Feb 26 '25 08:02 LeonMatthesKDAB

@LeonMatthesKDAB Thanks for making that channel! No need to copy it here, I can participate in it directly.

jnbooth avatar Mar 06 '25 19:03 jnbooth