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

Add support of enums as dynamic types #1220

Merged
merged 15 commits into from
Dec 16, 2023
Merged

Conversation

fbrouille
Copy link
Contributor

This PR aims to add support of enums registered as dynamic types. See https://docs.gtk.org/gobject/class.TypeModule.html and https://docs.gtk.org/gobject/iface.TypePlugin.html.
The new macro dynamic_enum_derive allows to register enums that are part of a module or of a plugin (e.g. a shared library on linux). The macro mimic enum_derive but generates code that support registration when the plugin (TypePlugin) is used, following the behavior defined in Glib doc. However it is possible to postpone the registration at first use of the enum (like within enum_derive) by explicitly setting the macro attribute lazy_registration = true.
Some examples in glib-macros/tests/dynamic_enums.rs

Signed-off-by: fbrouille <fbrouille@users.noreply.github.com>
@pbor
Copy link
Contributor

pbor commented Nov 4, 2023

naive question: why not a dynamic = true attribute to the existing macro?

@fbrouille
Copy link
Contributor Author

Do you mean like this ?

#[derive(Debug, Copy, Clone, PartialEq, Eq, glib::Enum)]
#[enum_type(name = "MyEnum", dynamic = true, plugin_type = MyPlugin, lazy_registration = true)]
enum MyEnum{...}

Goal is to have a clear distinction between "static" enums and "dynamic" ones. With separate helper attributes (glib::Enum and glib::DynamicEnum), a compilation error is raised if plugin_type and/or lazy_registration appear within a glib::Enum enum. I know it would also be possible within your suggestion, but IMO it is less clear for developers.

However I realize it might be even more clear:

#[derive(Debug, Copy, Clone, PartialEq, Eq, glib::Enum)]
#[enum_type(name = "MyEnum")]
#[dynamic_enum(plugin_type = MyPlugin, lazy_registration = true)]
enum MyEnum{...}

instead of:

#[derive(Debug, Copy, Clone, PartialEq, Eq, glib::DynamicEnum)]
#[enum_type(name = "MyEnum", plugin_type = MyPlugin, lazy_registration = true)]
enum MyEnum{...}

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.

That's a lot of code but I also don't see how to simplify that, right now :)

Can you also add the same for flags once we agreed on an API / implementation here?

@bilelmoussaoui What do you think about the API?

@sdroege
Copy link
Member

sdroege commented Nov 19, 2023

@bilelmoussaoui What do you think?

glib-macros/src/enum_derive.rs Outdated Show resolved Hide resolved
glib-macros/src/enum_derive.rs Outdated Show resolved Hide resolved
glib-macros/src/enum_derive.rs Outdated Show resolved Hide resolved
glib-macros/src/lib.rs Outdated Show resolved Hide resolved
glib-macros/src/lib.rs Outdated Show resolved Hide resolved
glib-macros/tests/dynamic_enums.rs Show resolved Hide resolved
glib/src/enums.rs Outdated Show resolved Hide resolved
glib/src/enums.rs Outdated Show resolved Hide resolved
glib/src/gobject/type_module.rs Outdated Show resolved Hide resolved
glib/src/subclass/type_plugin.rs Outdated Show resolved Hide resolved
@sdroege
Copy link
Member

sdroege commented Dec 2, 2023

Sorry for taking so long to review this!

…amic_enum macro

Signed-off-by: fbrouille <fbrouille@users.noreply.github.com>
@sdroege
Copy link
Member

sdroege commented Dec 4, 2023

However I realize it might be even more clear:
[...]

I actually like that. The main concern then is that it's a bit inconsistent with dynamic GObjects and those should ideally be changed similarly.

@pbor
Copy link
Contributor

pbor commented Dec 4, 2023

#[dynamic_enum(plugin_type = MyPlugin, lazy_registration = true)]
enum MyEnum{...}

maybe should be

#[enum_dynamic(plugin_type = MyPlugin, lazy_registration = true)]
enum MyEnum{...}

so that we follow the convention that we use the enum_ prefix?

Signed-off-by: fbrouille <fbrouille@users.noreply.github.com>
Signed-off-by: fbrouille <fbrouille@users.noreply.github.com>
Signed-off-by: fbrouille <fbrouille@users.noreply.github.com>
Signed-off-by: fbrouille <fbrouille@users.noreply.github.com>
Signed-off-by: fbrouille <fbrouille@users.noreply.github.com>
Signed-off-by: fbrouille <fbrouille@users.noreply.github.com>
…egistered)"

This reverts commit cd7ec87.

Signed-off-by: fbrouille <fbrouille@users.noreply.github.com>
Signed-off-by: fbrouille <fbrouille@users.noreply.github.com>
Signed-off-by: fbrouille <fbrouille@users.noreply.github.com>
Signed-off-by: fbrouille <fbrouille@users.noreply.github.com>
Signed-off-by: fbrouille <fbrouille@users.noreply.github.com>
Signed-off-by: fbrouille <fbrouille@users.noreply.github.com>
sdroege
sdroege previously approved these changes Dec 14, 2023
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.

Good for me. @bilelmoussaoui @pbor last chance :) I'll merge it during the weekend if there are no objections.

…ariables

Signed-off-by: fbrouille <fbrouille@users.noreply.github.com>
@fbrouille
Copy link
Contributor Author

fbrouille commented Dec 14, 2023

one more thing... Using cargo expand --package glib-macros --test dynamic_enums to view the generated code, I realized that there was an issue with the RegistrationStatus type: it would be defined with the same name for all enums defined in the same module (collision).
So I have refactored the code to generate a dedicated <EnumName>RegistrationStatus for each enum.
I also have taken the opportunity to replace the get_registration_status_ref() and get_gtype_ref() methods by static variables. The reason of those methods was to scope the embedded static variable to the enum type. Generating the static variables outside the enum type but with a dedicated name (<ENUM_NAME>REGISTRATION_STATUS) is simpler or more clear (IMO).
I also did it for the enum values.
see c9674e9

@fbrouille
Copy link
Contributor Author

fbrouille commented Dec 14, 2023

However I realize it might be even more clear:
[...]

I actually like that. The main concern then is that it's a bit inconsistent with dynamic GObjects and those should ideally be changed similarly.

I won't be able to do it before this weekend. Maybe another PR to address it (and also dynamic objects, interfaces) ?

@sdroege
Copy link
Member

sdroege commented Dec 16, 2023

Maybe another PR to address it (and also dynamic objects, interfaces)

Let's do that separately, yes. Unless you prefer to wait? Let me know. Otherwise I would merge now

@fbrouille
Copy link
Contributor Author

No, you can merge

@sdroege sdroege merged commit d982e08 into gtk-rs:master Dec 16, 2023
48 checks passed
@fbrouille fbrouille deleted the dynamic_enums branch December 17, 2023 17:05
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