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

interpreter: Add API to do JSON encoding/decoding for Value #7498

Merged
merged 1 commit into from
Feb 4, 2025

Conversation

hunger
Copy link
Member

@hunger hunger commented Jan 29, 2025

…terpreter

I need to enccode/decode interpreter values into JSON for the viewer and the live-preview.

Extract the from-json code from the viewer into a new crate interpreter-json, so that the normal interpreter does not need to gain a dependency on serde_json.

Extend the from_json code a bit and implement to_json. Add some simple tests while at it.

@ogoffart
Copy link
Member

Any reason not to put that in a module in the slint-interpreter crate?
It can be put behind the internal feature flag.
It is annoying that it may be in public API, but additional crates have downsides too (make publishing harder as we may reach rate limit if we publish too many crates at once)
If it has to be in a different crate, its name should be starting by i-slint-

@ogoffart
Copy link
Member

so that the normal interpreter does not need to gain a dependency on serde_json

It should be behind a feature flag.
(btw, serde-json is already a build dependency of the interpreter through i-slint-core-macros)

@hunger hunger force-pushed the tobias/push-sxpxykqvmupy branch from 5868617 to cfc108c Compare January 30, 2025 08:19
@tronical
Copy link
Member

I have the same feeling, I think it makes sense to create a separate crate when there is a need/use for it, but at the moment I think everyone who needs this functionality also needs the interpreter.

@hunger
Copy link
Member Author

hunger commented Jan 30, 2025

OK, I'll merge it into the interpreter.

@hunger hunger force-pushed the tobias/push-sxpxykqvmupy branch 2 times, most recently from 780acd4 to 920aed0 Compare January 30, 2025 14:20
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.

This adds public API, with a public dependency on serde_json when the json feature is added.

Should it be documented, either document it that this is an internal feature (and then it should be renamed with "internal") or polish the documentation (of the feature, and of the functions, and make sure it is on docs.rs)

@hunger hunger force-pushed the tobias/push-sxpxykqvmupy branch 2 times, most recently from 9849b99 to 89b0013 Compare January 30, 2025 17:46
@hunger
Copy link
Member Author

hunger commented Jan 30, 2025

A bit better docs... not great, but the new functions should be pretty obvious.

@hunger
Copy link
Member Author

hunger commented Jan 30, 2025

This also disambiguates the rust test driver: The workspace turns on the json feature in the slint-interpreter, so that is active in the test as well and causes a build fail there.

@hunger hunger force-pushed the tobias/push-sxpxykqvmupy branch 2 times, most recently from 954c5eb to 07ec501 Compare January 30, 2025 19: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.

We'll need to do proper API review on this.

I wonder if a better API wouldn't be to implement the From trait between serde_json::Value and slint_interpreter::Value

Or maybe the best would be to implement manually the serde::Serialize and serde::Deserialize trait for Value

@ogoffart ogoffart changed the title interpreter-json: Add a crate doing JSON encoding/decoding for the in… interpreter: Add API to do JSON encoding/decoding for Value Jan 30, 2025
@hunger hunger force-pushed the tobias/push-sxpxykqvmupy branch from 07ec501 to faff175 Compare January 30, 2025 20:27
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 realize that using Serialize/Decerialize or just From don't work because we need the type.
I think we probably need to create a public Type. But we can't just make our Type public.

IMHO it would be better to actually keep this feature internal under the internal feature flag and not having it as public.

Have it under slint-interpreter::internal::value_from_json/value_to_json functions.

That said eventually we would need to expose a proper type for our properties. We could discuss about that

PS: I took the liberty to edit the PR title

@hunger hunger force-pushed the tobias/push-sxpxykqvmupy branch 5 times, most recently from 3862a11 to 0ded3f2 Compare February 3, 2025 16:41
@hunger
Copy link
Member Author

hunger commented Feb 3, 2025

So I moved back to the state from late last week, but made json an internal-json feature now. We use the internal type and use that to help us decide how to decode the json strings.

I did keep the Result<_, String> for the error reporting, as that worked pretty well.

I should probably test more error conditions, but most of them are todo items than things that should report an error ;-)

@hunger
Copy link
Member Author

hunger commented Feb 3, 2025

I hope I also fixed the image path test on windows this time round.

@hunger hunger force-pushed the tobias/push-sxpxykqvmupy branch 2 times, most recently from df0bc86 to 29256a4 Compare February 4, 2025 10:17
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. Looks good now.

Extract the `from_json` and `to_json` code from the viewer into the `interpreter`.

Extend the `from_json` and `to_json` a bit. Add some simple tests while at it.
@hunger hunger force-pushed the tobias/push-sxpxykqvmupy branch from 29256a4 to cd980f9 Compare February 4, 2025 11:08
@hunger hunger merged commit ded82cb into slint-ui:master Feb 4, 2025
37 checks passed
@hunger hunger deleted the tobias/push-sxpxykqvmupy branch February 4, 2025 12:06
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.

3 participants