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

More flexible X/"extra" type handling #163

Merged
merged 4 commits into from
Feb 21, 2023
Merged

Conversation

alexheretic
Copy link
Owner

@alexheretic alexheretic commented Feb 21, 2023

This is a less breaking version of #162. Mainly rolling back the Section::default changes and adding Section::builder to help when using a custom X. This avoids the breakage that the Section::default change would have caused.

It's still a little breaking with the Text::new changes. However, I'm willing to try releasing that as a non-breaking bump as I suspect this won't break usage in the wild.

Closes #162

@alexheretic alexheretic force-pushed the non-breaking-extra++ branch 2 times, most recently from 5f5d949 to 9910be9 Compare February 21, 2023 12:25
@alexheretic
Copy link
Owner Author

@aweinstock314 with this version I'd be comfortable releasing as a non-breaking bump, does that work for you?

I'm not against breaking changes, they'll just take a little more thought as I want to minimise the number of different breaking versions so want to bundle up some other breakers too.

@aweinstock314
Copy link
Contributor

Yes, this version suffices to make hecrj/wgpu_glyph#96 work (Section::new already doesn't constrain X=Extra, so I'm using that for the examples there, though that becomes a breaking change for wgpu_glyph).

Should I leave #162 open, as the breaking changes version, or close it?

@alexheretic alexheretic merged commit 804fcb2 into main Feb 21, 2023
@alexheretic alexheretic deleted the non-breaking-extra++ branch February 21, 2023 21:39
@alexheretic
Copy link
Owner Author

alexheretic commented Feb 21, 2023

Thanks @aweinstock314 I opened #164 to help remember to fix the Section::default() thing.

I wonder if there's a different & cleaner way to do the X stuff, like type-erasure. Hopefully I'll find some time to look into it. Overall I'm not 100% happy with the complexity of the generics. I've tried to be "easy to use" directly but at the same time customisable for wrapper libraries like wgpu_glyph, but it's definitely not perfect.

@aweinstock314
Copy link
Contributor

aweinstock314 commented Feb 21, 2023

It's reasonably straightforward to extend in a single-inheritance way with the current approach, via adding the parent struct as a field and implementing Deref/DerefMut into the parent (along with Debug/Clone/Copy/Hash/PartialEq/Default). It's a bit verbose when adding an extension trait to provide the with_field methods for all the inherited fields (each struct has to mention all of its ancestor's fields) (though this might not be a true limitation, and deref coercion might just do the right thing).

Making use of deref coercion doesn't support multiple inheritance though (e.g. with the classic diamond inheritance example where ExtraB and ExtraC each extend ExtraA, and ExtraD wants to extend ExtraB and ExtraC, ExtraD can't have multiple Deref impls targetting both ExtraB and ExtraC).

HashMap<String, Box<dyn Any>> might work as a dynamic expansion mechanism, but it'd be less statically checked and probably less performant (i.e. there would be runtime checks for fields not existing and fields being the wrong type).

frunk's LabelledGeneric might be worth looking into. Essentially each Extra struct would contain only the fields it needs and derive LabelledGeneric, and the overall type to use as the extra data would be the hlist-concatenation of each struct's LabelledGeneric::Repr. Each struct-definer would define with_field methods for any hlist containing their fields (so each updater would only need to be defined once). Any struct containing any subset of the fields could be materialized from the combined representation with LabelledGeneric::from. It doesn't look like frunk::hlist has concatenation yet, but it does have foldr, so it could probably be straightforwardly expressed as concat xs ys = foldr cons ys xs at the trait level. This would be entirely statically checked, but would be more heavyweight trait-wise than the current approach.

@aweinstock314
Copy link
Contributor

Update:

Removing with_color and with_z from TextExt and adding a DerefMut<glyph_brush::Extra> for wgpu_glyph::Extra impl doesn't help since the with_field methods take self, not &mut self, but it wouldn't be ergonomic to change that, since (among other things), that would mean that you'd need to set child fields before parent fields, since each with_field method would return a reference to the deref target. This means that the limitation from the first paragraph is a true limitation: with_field methods need to be repeated for each ancestor field in each child, even in the single-inheritance case.

aweinstock314 added a commit to aweinstock314/wgpu_glyph that referenced this pull request Mar 11, 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.

2 participants