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

Properties macro #494

Merged
merged 3 commits into from
Jan 30, 2023
Merged

Properties macro #494

merged 3 commits into from
Jan 30, 2023

Conversation

ranfdev
Copy link
Member

@ranfdev ranfdev commented Jan 18, 2022

The implementation is a mess... I will clean it up.

Take a look at the newly added test case to see how it can be used.
I'm interested in comments about the API:

#[derive(Props, Default)]
pub struct Foo {
    #[prop("bar", "bar", "This is a bar", None, glib::ParamFlags::READWRITE)]
    bar: RefCell<String>,
    #[prop("buzz", "buzz", "This is a buzz", 1, 100, 1, glib::ParamFlags::READWRITE)]
    buzz: RefCell<u32>,
}

Currently you can't set more than one flag at a time. Everything else seems to work. Oh, and I need to finish implementing the Parametrize trait for all the type that have a ParamSpec.

Related to #27

glib-macros/tests/props.rs Outdated Show resolved Hide resolved
glib/src/param_spec.rs Outdated Show resolved Hide resolved
glib-macros/src/lib.rs Outdated Show resolved Hide resolved
glib-macros/tests/props.rs Outdated Show resolved Hide resolved
glib-macros/tests/props.rs Outdated Show resolved Hide resolved
@bilelmoussaoui
Copy link
Member

Currently you can't set more than one flag at a time

I would not use the flags directly but make them separate attributes like (read/write/notify) and map them internally to the corresponding flag

@ids1024
Copy link
Contributor

ids1024 commented Jan 18, 2022

Vala supports two types of properties (https://wiki.gnome.org/Projects/Vala/Manual/Classes#Properties):

Properties can either be declared with code that will perform particular actions on get and set, or can simply declare which actions are allowed and allow Vala to implement simple get and set methods. This second pattern (automatic property) will result in fields being added to the class to store values that the property will get and set. If either get or set has custom code, then the other must either be also written in full, or omitted altogether.

So properties can be backed in something like a RefCell with automatic get/set implementations, or manual get/set which may not even have a struct member. I'm honestly not sure what a good Rust API to support both use cases would be, but it seems it should be done in some way.

In addition to properties, GObject has signals, and I think the two should be considered together. (I wrote an idea for signals in #214). Luckily signals are simpler in comparison.

glib/src/param_spec.rs Outdated Show resolved Hide resolved
glib/src/param_spec.rs Outdated Show resolved Hide resolved
glib/src/param_spec.rs Outdated Show resolved Hide resolved
glib-macros/tests/props.rs Outdated Show resolved Hide resolved
@ranfdev
Copy link
Member Author

ranfdev commented Jan 20, 2022

The latest API is:

#[derive(Props, Default)]
pub struct Foo {
    #[prop(get, set)]
    bar: Mutex<String>,
    #[prop(get = Self::hello_world)]
    _buzz: RefCell<String>,
    #[prop(get, set = Self::set_fizz, name = "fizz")]
    fizz: RefCell<String>,
}

@jf2048
Copy link
Member

jf2048 commented Jan 20, 2022

Nice to see someone working on this! 😃 It's looking good, I was thinking about this for the last few months and I had a few ideas:

  • Should it have flags to auto generate getters and setters? Maybe notify methods and pspec accessors too?
    #[prop(auto_get(pub), auto_set)]
    fizz: RefCell<String>,
    Generates something like:
     impl <Foo as ObjectSubclass>::Type {
         pub fn fizz(&self) -> Ref<String> {
             self.imp().fizz.borrow()
         }
          fn set_fizz(&self, value: String) {
              if self.imp().fizz.borrow() != value {
                  self.imp().fizz.replace(value)
                  self.fizz_notify();
              }
          }
          fn fizz_notify(&self) {
              self.notify_by_pspec(Self::fizz_pspec());
          }
          fn fizz_pspec() -> &glib::ParamSpec {
              <Self as ObjectSubclass>::properties()[2]
          }
     }
  • Or should it generate a "fields" enum to index the properties array, like is common when writing GObjects in C?
    #[derive(Props, Default)]
    #[glib_props(fields)]
    struct Foo {
        // ...
    }
    Generates something like:
    enum FooField {
        Bar = 0,
        Buzz = 1,
        Fizz = 2,
    }
    impl Foo {
        fn pspec(field: FooField) -> &glib::ParamSpec {
            <Self as ObjectSubclass>::properties()[field as usize]
       }
    }
  • Should it allow for a custom implementation of ObjectImpl, if someone wants to layer functionality on top of the generated code?
    #[derive(Props, Default)]
    #[glib_props(no_trait)]
    struct Foo { /* ... */ }
    Generates something like:
    impl Foo {
        fn properties() { /* ... */ }
        fn property() { /* ... */ }
        fn set_property() { /* ... */ }
    }

@ranfdev
Copy link
Member Author

ranfdev commented Jan 20, 2022

auto generate ...

This is already implemented:
#[prop(get)] -> autogenerate
#[prop(get = func)] -> custom
#[prop()] -> not readable

should it generate a "fields" enum

Yes, I could do it, makes sense.

Should it allow for a custom implementation of ObjectImpl, if someone wants to layer functionality on top of the generated code?

The general opinion is "yes". My current idea is to define a trait AutoProps and then call that in ObjectImpl.

@ranfdev
Copy link
Member Author

ranfdev commented Jan 20, 2022

Should it have flags to auto generate getters and setters?

Sorry @jf2048, I misread. You actually want to generate getters/setters on the wrapper type. I don't know if this is technically possible.
Read the comments above, there's another thread about this in the code comments.

@bilelmoussaoui
Copy link
Member

bilelmoussaoui commented Jan 20, 2022

Should it have flags to auto generate getters and setters?

Sorry @jf2048, I misread. You actually want to generate getters/setters on the wrapper type. I don't know if this is technically possible. Read the comments above, there's another thread about this in the code comments.

Well, the code suggested by @jf2048 should actually work just fine because it doesn't impl T but instead it does impl <T as ObjectSubclass>::Type which is the public type :)

@ranfdev
Copy link
Member Author

ranfdev commented Jan 20, 2022

That syntax is invalid:

impl <#name as ObjectSubclass>::Type {
    fn hi() {}
}
error[E0118]: no nominal type found for inherent implementation
  --> glib-macros/tests/props.rs:20:22
   |
20 |             #[derive(Props, Default)]
   |                      ^^^^^ impl requires a nominal type
   |
   = note: either implement a trait on it or create a newtype to wrap it instead

Explanation: https://stackoverflow.com/questions/49277733/what-is-a-nominal-type-in-the-context-of-an-inherent-implementation

@bilelmoussaoui
Copy link
Member

@ranfdev right, but you can generate a trait and implement it for T::Type no?

@jf2048
Copy link
Member

jf2048 commented Jan 20, 2022

you can generate a trait and implement it for T::Type no?

That's what I was getting at, I have been able to do that in some of my experiments. Sorry I should have been clearer, my code above was some really rough pseudocode.

@ranfdev
Copy link
Member Author

ranfdev commented Jan 20, 2022

but you can generate a trait and implement it for T::Type no?

Yes, I thought about it, but then when you import the type you must import the related trait too, else you don't get access to any method.

It's the best option for now, but it's not perfect

Copy link
Member

@bilelmoussaoui bilelmoussaoui left a comment

Choose a reason for hiding this comment

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

Sorry, reviewing from phone is hard

glib-macros/tests/props.rs Outdated Show resolved Hide resolved
@ranfdev ranfdev force-pushed the props_macro branch 2 times, most recently from 9185a95 to b9a3d34 Compare February 15, 2022 15:44
@ranfdev ranfdev force-pushed the props_macro branch 2 times, most recently from b848e97 to c678ad6 Compare February 23, 2022 13:07
glib-macros/tests/props.rs Outdated Show resolved Hide resolved
glib-macros/tests/props.rs Outdated Show resolved Hide resolved
@ranfdev ranfdev force-pushed the props_macro branch 2 times, most recently from 19b6eca to 0e44f5d Compare February 24, 2022 15:27
@ranfdev ranfdev changed the title WIP: Initial Props macro Properties macro Jun 27, 2022
@sdroege sdroege self-requested a review June 27, 2022 09:26
@melix99
Copy link
Contributor

melix99 commented Jul 11, 2022

A feature I see missing is a way to create a property with a WeakRef.

@sdroege
Copy link
Member

sdroege commented Aug 24, 2022

To move this forward, maybe this PR can be split up a bit? Let's first try to get those paramspec traits merged, for example. That way it can all be reviewed in smaller chunks.

glib-macros/src/properties.rs Outdated Show resolved Hide resolved
@bilelmoussaoui
Copy link
Member

Would you mind rebasing this PR & fixing the test failure?

@bilelmoussaoui bilelmoussaoui force-pushed the props_macro branch 2 times, most recently from 9bb93fd to 438406d Compare January 27, 2023 15:06
@bilelmoussaoui bilelmoussaoui force-pushed the props_macro branch 2 times, most recently from f8dc3b1 to 0b6951d Compare January 27, 2023 20:01
ranfdev and others added 3 commits January 30, 2023 12:04
The macro would simplify the creation of properties and generate
traits that can be used in ObjectImpl for now until further integration
with signals & the rest of GObject creation is figured out.
@bilelmoussaoui bilelmoussaoui merged commit 3974526 into gtk-rs:master Jan 30, 2023
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.

10 participants