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.

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

Serde struct #2727

Merged
merged 1 commit into from
May 22, 2023
Merged

Serde struct #2727

merged 1 commit into from
May 22, 2023

Conversation

Mehrbod2002
Copy link
Contributor

fixes: #2660
Suppose missed some struct and should add serde features to toml but with cycle importing I suppose collabrators could help to solve it .

Copy link
Member

@ogoffart ogoffart left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR. This is a great start.

However, there is a bunch of things which doesn't make sense to serialize as is.

Because we probably should maintain compatibility over the serialization so we should be careful about that.

Also could you perhaps add a test. I'd say in api/rs/slint/tests/
The test should serialize and deserialize a slint struct and check they are equal, or something like that.

internal/core/Cargo.toml Outdated Show resolved Hide resolved
internal/core/sharedvector.rs Outdated Show resolved Hide resolved
internal/core/string.rs Outdated Show resolved Hide resolved
internal/core/model.rs Outdated Show resolved Hide resolved
@Mehrbod2002 Mehrbod2002 requested a review from ogoffart May 14, 2023 17:14
@Mehrbod2002
Copy link
Contributor Author

Mehrbod2002 commented May 14, 2023

@ogoffart there are some error on test on uefi-demo and mcu .
i can't recognize it . can you guide what are those ?

Edit: fixed

api/rs/slint/Cargo.toml Outdated Show resolved Hide resolved
@Mehrbod2002 Mehrbod2002 requested a review from tronical May 14, 2023 19:38
api/rs/slint/Cargo.toml Outdated Show resolved Hide resolved
api/rs/slint/Cargo.toml Outdated Show resolved Hide resolved
api/rs/slint/tests/simple_macro.rs Outdated Show resolved Hide resolved
@Mehrbod2002 Mehrbod2002 requested a review from ogoffart May 15, 2023 10:59
Copy link
Member

@ogoffart ogoffart left a comment

Choose a reason for hiding this comment

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

I discussed this with @tronical and we think we should do this PR in several pieces.

  1. Make our struct such as SharedVector and SharedString as serializable.
    I think what you have there is good: A feature in slint, forwarded in i-slint-core to enable serialization of our structs.

  2. Then we can enable the serialization of struct declared in slint. But the question is whether this should be enabled by default as that may actually be a breaking change.
    Maybe it should be opt-in per struct, or maybe it should be done only using import from Rust (Ability to import Rust and C++ struct as .slint struct #1726)

So now i think we should try to get the first part polished and merged.

api/rs/slint/tests/simple_macro.rs Outdated Show resolved Hide resolved
api/rs/slint/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Member

@ogoffart ogoffart left a comment

Choose a reason for hiding this comment

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

So just to be clear, i think first we should only keep the changes in the internal/core library (and add a test there) and also a way to enable the feature from slint/Cargo.toml

@Mehrbod2002
Copy link
Contributor Author

So just to be clear, i think first we should only keep the changes in the internal/core library (and add a test there) and also a way to enable the feature from slint/Cargo.toml

Do we miss any of these ? What exactly should we do?
Feature won't call with serde as there are optional?

@ogoffart
Copy link
Member

The problem is that if we add in the generated code something like

#[cfg_attr(feature="serde", derive(Serialize, Deserialize))]
struct Foo { 
 ...
}

the feature="serde" that this represent, is the feature enabled on the app itself, not the slint feature. And it only compiles if the serde feature is enabled in i-slint-core because every field must be serializable.
And right now, some crate might have a serde feature without wanting this option
But things like image or [int] are not de-serializable. We'd need to have it opt-in.

Some idea include:

  • An an option in slint-build API to enable that. and a feature in slint-macro crate.

  • add something like (in .slint)

@rust-attr(cfg_attr(feature="serde", derive(Serialize, Deserialize)))
struct Foo { str: string }
  • Or something like (also in .slint)
@pragma(serde)
struct Foo { str: string }

So this turns out to be harder than first thought.

But for sure the first step is to allow SharedString and SharedVector to be serialized.
Maybe Image can also be made serializable. Model might be a bit difficult.
We could then generate the derive only for struct that can be serialized.

@Mehrbod2002
Copy link
Contributor Author

@ogoffart
By this , can we use feature flags to enable just now and then implement the something to slint like what you said :
pragma ? I can work on it .
if there is and we can do it just now with serde feature can you commit the changes about flag here to merge it ?

@Mehrbod2002 Mehrbod2002 requested a review from ogoffart May 16, 2023 13:15
@ogoffart
Copy link
Member

By this , can we use feature flags to enable just now

Not sure what you mean. But what i mean is to have feature flag to implement serialize and deserialize on Slint's core types such as SharedVector and SharedString

@Mehrbod2002
Copy link
Contributor Author

Mehrbod2002 commented May 16, 2023

@ogoffart
I meant , can you do it on this PR a small commit for flag on toml ?
I don't know exactly how can I feature it on toml and event worst I don't which toml should I put features table for serde :)

Copy link
Member

@ogoffart ogoffart left a comment

Choose a reason for hiding this comment

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

In the api/rs/slint/Cargo.toml , just add a feature to forward the core serde feature

## Enable the serialization and deserialization of Slint's core types by implementing 
## the traits from the [serde](https://crates.io/crates/serde) crate
serde = ["i-slint-core/serde"]

api/rs/slint/Cargo.toml Outdated Show resolved Hide resolved
api/rs/slint/Cargo.toml Outdated Show resolved Hide resolved
@Mehrbod2002 Mehrbod2002 requested a review from ogoffart May 17, 2023 11:53
internal/compiler/generator/rust.rs Outdated Show resolved Hide resolved
api/rs/slint/tests/simple_macro.rs Outdated Show resolved Hide resolved
@Mehrbod2002 Mehrbod2002 requested a review from ogoffart May 17, 2023 13:16
@Mehrbod2002
Copy link
Contributor Author

@ogoffart is there any issue with it?

Copy link
Member

@ogoffart ogoffart left a comment

Choose a reason for hiding this comment

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

Almost good.
Please move the test to core, and make sure it is being run on CI (we pass --all-features on our CI so it should normally be run)

Also move the feature a bit down in the file for the documentation.

api/rs/slint/tests/simple_macro.rs Outdated Show resolved Hide resolved
api/rs/slint/Cargo.toml Outdated Show resolved Hide resolved
internal/core/sharedvector.rs Show resolved Hide resolved
internal/core/string.rs Show resolved Hide resolved
@Mehrbod2002 Mehrbod2002 requested a review from ogoffart May 19, 2023 15:11
@Mehrbod2002
Copy link
Contributor Author

@ogoffart done ?

@Mehrbod2002
Copy link
Contributor Author

@ogoffart it can't be merge ?

@tronical
Copy link
Member

Yes, there are merge conflicts. Would you like me to rebase and merge it for you?

@Mehrbod2002
Copy link
Contributor Author

@tronical Sure , Thanks :)

@Mehrbod2002
Copy link
Contributor Author

@tronical is it done ?

@tronical
Copy link
Member

Yes, let's get this in. If you feel like digging further, one thing that would be nice is if the de-serialization into a SharedVector would be possible without the allocation of an intermediate Vec.

@tronical tronical merged commit cf71ccb into slint-ui:master May 22, 2023
@Mehrbod2002
Copy link
Contributor Author

@tronical I will do it these days , almost working to add serde feature in slint .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

serde serialization of struct
3 participants