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

Remove dependency on smallvec and set MSRV of 1.75 #253

Open
PoignardAzur opened this issue May 3, 2024 · 4 comments
Open

Remove dependency on smallvec and set MSRV of 1.75 #253

PoignardAzur opened this issue May 3, 2024 · 4 comments
Assignees
Labels
masonry Issues relating to the Masonry widget layer

Comments

@PoignardAzur
Copy link
Contributor

PoignardAzur commented May 3, 2024

Smallvec in Masonry was always intended to be a placeholder until return-position impl Trait was stabilized. Well, now it's stable, it's been stable for a year, we should use it.

Methods that return Smallvec<T> should instead return impl Iterator<Item = T>. The MSRV of Masonry and xilem_masonry should be set to reflect that.

EDIT: Complicated by the fact that we need Widget to be dyn-safe. Might need to be postponed.

@PoignardAzur PoignardAzur added the masonry Issues relating to the Masonry widget layer label May 3, 2024
@PoignardAzur PoignardAzur self-assigned this May 3, 2024
@Philipp-M
Copy link
Contributor

I want to note, that RPITIT prevents trait-objects (more specifically Box<dyn Widget> is not possible then). It's also generally discouraged for public APIs (see also rust-lang/rfcs#3425 (comment)).

@PoignardAzur
Copy link
Contributor Author

PoignardAzur commented May 4, 2024

I want to note, that RPITIT prevents trait-objects (more specifically Box<dyn Widget> is not possible then).

How so?

It's also generally discouraged for public APIs

Overall I'm... mostly confident we're never going to bump into the problems that link mentions? Right now SmallVec is used to get a list of children, and that list is virtually always going to be either iterated through immediately, or stored as a collection.

There reason I used SmallVec is that a lot of widgets return a children list with a size of one. But returning impl IntoIterator covers that case better.

@Philipp-M
Copy link
Contributor

How so?

See https://doc.rust-lang.org/beta/reference/items/traits.html#object-safety

mostly confident we're never going to bump into the problems that link mentions

Probably, main issue is mostly trait object safety for Widget AFAICS.

I'm not yet that confident in the masonry codebase, but is it even necessary to have children in the Widget trait? (I.e. have it instead on the concrete widget types)?

@PoignardAzur
Copy link
Contributor Author

PoignardAzur commented May 4, 2024

Probably, main issue is mostly trait object safety for Widget

Oh, right, the Widget trait is the one that would have a generic method. Well, there would be ways to implement it and preserve dyn-safety, but I do agree that complicates things a bit.

I'm not yet that confident in the masonry codebase, but is it even necessary to have children in the Widget trait? (I.e. have it instead on the concrete widget types)?

Yup. This means that the framework can eg take a position and return a reference to the Widget at that position without having to write code in WidgetPod. It's also useful for debugging, building the accessibility tree, and in the future the event system will rely on it a lot more.

Having children() on the Widget trait is a core design element.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
masonry Issues relating to the Masonry widget layer
Projects
None yet
Development

No branches or pull requests

2 participants