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

Partially nested types in properties: newtypes, ThreadGuard #984

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ranfdev
Copy link
Member

@ranfdev ranfdev commented Feb 11, 2023

fixes #983
enables support for a correct handling of ThreadGuard, therefore supersedes #968.

In general, this kinda enables two-level nesting: this makes supporting newtypes trivial (conceptually, they are a nested type).

The change is backwards compatible

@ranfdev
Copy link
Member Author

ranfdev commented Feb 11, 2023

Nesting multiple writable types is still not supported.
So, Rc<Mutex<Rc<u32>>> is now supported, but Mutex<Mutex<Rc<u32>>> isn't supported at all (as before).

@ranfdev ranfdev changed the title Props type support Partially nested types in properties: newtypes, ThreadGuard Feb 11, 2023
@ranfdev
Copy link
Member Author

ranfdev commented Feb 11, 2023

I guess there's still one problem: the setter on the wrapper type accepts the inner type, that is, i32, not MyInt(i32).

CC: @YaLTeR

@YaLTeR
Copy link
Contributor

YaLTeR commented Feb 11, 2023

Yeah, it would be good if the generated public-facing methods had newtype in their signatures.

#[property(get, set)]
thread_guard_wrapped: Mutex<ThreadGuard<u32>>,
#[property(get, set)]
thread_guard_wrapping: ThreadGuard<Mutex<u32>>,
Copy link
Member

Choose a reason for hiding this comment

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

Would this also already handle a Mutex<Option<ThreadGuard<Paintable>>>? Asking for the Option there. It's a write-only property so I think that potentially works because there's From<T> for Option<T> but I think it doesn't work in practice because you'd need to call from() twice?

#crate_ident::PropertySetNested::set_nested(
&self.#field_ident,
move |v| v.#member = #crate_ident::Value::get(value)#expect
move |v| v.#member = ::std::convert::From::from(value)
);
}
),
Copy link
Member

Choose a reason for hiding this comment

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

Adding a discussion item for this somewhere so we don't forget

I guess there's still one problem: the setter on the wrapper type accepts the inner type, that is, i32, not MyInt(i32).

@bilelmoussaoui
Copy link
Member

Would you mind rebasing this one?

@ranfdev
Copy link
Member Author

ranfdev commented Oct 28, 2023

I can rebase, but I don't think this is ready to be merged. There are still some unresolved things, as pointed out by the unresolved comments

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.

[FEATURE REQUEST] Newtype support for #[property]
4 participants