Skip to content
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.

Sign up for GitHub

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

Jump to bottom

RFC: glib: Add optional support for serialization and deserialization with serde #1070

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

RFC: glib: Add optional support for serialization and deserialization with serde #1070

wants to merge 1 commit into from

Conversation

rmnscnce
Copy link
Contributor

@rmnscnce rmnscnce commented Apr 5, 2023 •
edited
Loading

If merged, some GLib types will gain serialization and deserialization support through the serde serialization framework. This feature is gated as serde in the glib package.

Supported types for both deserialization and serialization:

  • glib::ByteArray
  • glib::Bytes
  • glib::GString

Some types are only serializable:

  • glib::GStr
  • glib::StrV

Collection types are also supported as long as the type parameters implement the necessary traits:

  • glib::Slice<T: TransparentType + ..>
  • glib::PtrSlice<T: TransparentPtrType + ..>
  • glib::List<T: TransparentPtrType + ..>
  • glib::SList<T: TransparentPtrType + ..>

@rmnscnce rmnscnce marked this pull request as draft April 5, 2023 07:35
@rmnscnce rmnscnce force-pushed the serde branch 2 times, most recently from 3479d6a to 6c67b19 Compare April 5, 2023 07:45
@sdroege
Copy link
Member

sdroege commented Apr 5, 2023

I like this and we have something similar in place in the GStreamer bindings already. You probably want to mention this new feature in the README.md though.

Also do you see a possibility of moving all that repetitive code into a macro or two? :)

@GuillaumeGomez
Copy link
Member

Looks like a good idea.

@rmnscnce rmnscnce force-pushed the serde branch 4 times, most recently from 5e2d762 to d95e941 Compare April 5, 2023 22:08
sdroege
sdroege reviewed Apr 6, 2023
View reviewed changes
glib/Cargo.toml Outdated
@@ -59,6 +60,7 @@ log_macros = ["log"]
dox = ["ffi/dox", "gobject_ffi/dox", "log_macros"]
compiletests = []
gio = ["gio_ffi"]
serialize = ["dep:serde"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why serialize instead of serde?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to use serde but seems like the tests break on it.

https://github.com/gtk-rs/gtk-rs-core/actions/runs/4616156154/jobs/8160815190#step:19:414
https://github.com/gtk-rs/gtk-rs-core/tree/5e2d7622885fd64cfbf7a727273d78368cdc139e

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrm that looks like a bug in trybuild2. @GuillaumeGomez you're maintaining that, can you take a look? :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I published a new version of trybuild2 with the commits of the upstream trybuild.

sdroege
sdroege reviewed Apr 6, 2023
View reviewed changes
glib/src/serde.rs Outdated Show resolved Hide resolved
sdroege
sdroege reviewed Apr 6, 2023
View reviewed changes
glib/src/serde.rs Outdated Show resolved Hide resolved
@rmnscnce rmnscnce force-pushed the serde branch 6 times, most recently from 3dd4e9c to 297c255 Compare April 10, 2023 23:04
bilelmoussaoui
bilelmoussaoui reviewed Apr 10, 2023
View reviewed changes
glib/src/lib.rs Outdated Show resolved Hide resolved
@rmnscnce rmnscnce force-pushed the serde branch 2 times, most recently from 9388fbf to 407d50b Compare April 10, 2023 23:32
sdroege
sdroege reviewed Apr 11, 2023
View reviewed changes
Copy link
Member

@sdroege sdroege left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add something to the CI so that it builds with and without the serde feature so we can make sure things continue to work?

glib/src/serde.rs Outdated Show resolved Hide resolved
glib/src/serde.rs Outdated Show resolved Hide resolved
@rmnscnce rmnscnce force-pushed the serde branch 2 times, most recently from d222857 to 59b15e0 Compare April 12, 2023 01:29
@rmnscnce rmnscnce force-pushed the serde branch 3 times, most recently from c45eb0b to abe5dfd Compare April 12, 2023 15:50
@rmnscnce rmnscnce force-pushed the serde branch 5 times, most recently from 89d0f10 to e4e7b7f Compare April 27, 2023 16:27
sdroege
sdroege reviewed Oct 27, 2023
View reviewed changes
.github/workflows/CI.yml Outdated
@@ -23,7 +23,7 @@ jobs:
- { name: "cairo", features: "png,pdf,svg,ps,use_glib,v1_18,freetype,script,xcb,xlib,win32-surface", nightly: "--features 'png,pdf,svg,ps,use_glib,v1_18,freetype,script,xcb,xlib,win32-surface'", test_sys: true }
- { name: "gdk-pixbuf", features: "v2_42", nightly: "--all-features", test_sys: true }
- { name: "gio", features: "v2_74", nightly: "--all-features", test_sys: true }
- { name: "glib", features: "v2_74", nightly: "--all-features", test_sys: true }
- { name: "glib", features: "v2_74,serde", nightly: "--all-features", test_sys: true }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- { name: "glib", features: "v2_74,serde", nightly: "--all-features", test_sys: true }
- { name: "glib", features: "v2_74", nightly: "--all-features", test_sys: true }
- { name: "glib serde", features: "v2_74,serde", nightly: "--all-features", test_sys: true }

or so maybe so we test both configurations?

sdroege
sdroege previously approved these changes Oct 27, 2023
View reviewed changes
Copy link
Member

@sdroege sdroege left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me otherwise

@rmnscnce rmnscnce force-pushed the serde branch 4 times, most recently from ea985eb to 5fd8889 Compare January 13, 2024 18:25
@rmnscnce
glib: Add optional support for serialization and deserialization with…
43b0a38 
… serde

This feature is gated as `serde`

Supports both serialization and deserialization:
- glib::ByteArray
- glib::Bytes
- glib::GString

Supports serialization only:
- glib::GStr
- glib::StrV

Collection types are also supported  as long as the type parameters implement the necessary traits:
- glib::Slice<T: TransparentType + ..>
- glib::PtrSlice<T: TransparentPtrType + ..>
- glib::List<T: TransparentPtrType + ..>
- glib::SList<T: TransparentPtrType + ..>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bilelmoussaoui bilelmoussaoui bilelmoussaoui left review comments

@sdroege sdroege sdroege left review comments

@pbor pbor Awaiting requested review from pbor

@GuillaumeGomez GuillaumeGomez Awaiting requested review from GuillaumeGomez

@ranfdev ranfdev Awaiting requested review from ranfdev

@jf2048 jf2048 Awaiting requested review from jf2048

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
glib
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rmnscnce @sdroege @GuillaumeGomez @bilelmoussaoui