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 support for save/load of enums #3012

Merged
merged 2 commits into from
Jun 29, 2023

Conversation

Guiguiprim
Copy link
Contributor

Fixes for #3011

This uses a hack to serialize enum values in string format, maybe this can be made in a better way.

@CLAassistant
Copy link

CLAassistant commented Jun 28, 2023

CLA assistant check
All committers have signed the CLA.

@ogoffart
Copy link
Member

Thank you for your contribution and the pull request. I really appreciate your effort.

I must say, your idea of serializing using __enum<> is quite clever. However, I would prefer a less hackish approach, ideally just saving the string representation of the enum.

To achieve this, I believe there are two possibilities that we can explore:

  1. The cleanest approach would be to determine the type of the property so that we can generate an EnumValue from a string. We can identify the type by utilizing the properties_and_callbacks() function on component. It would be helpful to cache it in a HashMap and then pass the expected type to the from_json function. This approach is somewhat similar to what we do in the JS integration:

    fn to_eval_value<'cx>(

    The advantage of this approach is that it would allow us to serialize other types that can be deserialized from a string, such as images (from their filename), colors, etc. However, I understand that this may be more difficult.

  2. An alternative approach would be to modify the code to accept a Value::String(...) wherever we currently expect a Value::EnumerationValue(...). In other words, we would allow assigning a Value("bar") for an enum that has "bar" as one of its values.

From looking at the code, it appears that these modifications would not be required in many places:

(As a side node, we might want to change Value::EnumerationValue from its current form. Instead, it would be more efficient to represent it with an integer as indicated by the "FIXME" comment where it is declared, but that's out of the scope of this PR)

Once again, I want to express my gratitude for your pull request. I hope my suggestions and explanations make sense to you. Please let me know if you have any further questions or if there's anything else I can assist you with.

@Guiguiprim Guiguiprim force-pushed the interpreter-enum-save-load branch from 0f41928 to a6a2f5b Compare June 28, 2023 16:56
@Guiguiprim
Copy link
Contributor Author

I went for the first solution.

I'm not entirely sure about the error handling, but it works.

@Guiguiprim
Copy link
Contributor Author

By the way, thank you for the detailed briefing on how to do it well !

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.

Thank you! this looks good. I'll merge tomorrow.

@ogoffart ogoffart merged commit c57354e into slint-ui:master Jun 29, 2023
@Guiguiprim Guiguiprim deleted the interpreter-enum-save-load branch July 5, 2023 12:29
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