Have TestHarness methods use WidgetTag instead of WidgetId#1738
Have TestHarness methods use WidgetTag instead of WidgetId#1738PoignardAzur wants to merge 4 commits into
Conversation
xStrom
left a comment
There was a problem hiding this comment.
The changes look fine for the specified goal. However, could you say a few words about why we would want to do this? It's not obvious to me why the tags are better.
waywardmonkeys
left a comment
There was a problem hiding this comment.
Also, what is the lifecycle on the dynamically created tags (RenderRoot::make_tag_for_widget)? Is this just for tests? (but it is available in a way that people will call it?)
|
|
||
| for (&id, &next_id) in std::iter::zip(&focusable_widgets, &next_focusable_widgets) { | ||
| harness.focus_on(Some(id)); | ||
| let tag = harness.make_dyn_tag_for_widget(id); |
There was a problem hiding this comment.
You're subverting the point of what you're doing here that is ...
There was a problem hiding this comment.
I agree that this isn't very elegant, but also this test isn't really representative of how the API would usually be used.
As I mentioned in another comment, my ideal split is "WidgetTag when you know the widget before adding it, WidgetId when you know the widget after it's added".
While in most tests we know our tree ahead of time and WidgetTag is more convenient, there's always a few corner cases where we'd really want WidgetId instead. Which means we have three choices:
- 1: Duplicate every API to accept either WidgetTag or WidgetId.
- 2: Use an escape hatch to get a WidgetId from a WidgetTag.
- 3: Use an escape hatch to get a WidgetTag from a WidgetId.
Existing code does a mix of 1 and 2, this PR moves towards 3. The result is overall much less verbose, except for this specific code snippet.
There's two reasons:
In general, WidgetTags aren't garbage collected, so the lifecycle is "until the TestHarness/RenderRoot is dropped". This is fine because WidgetTags are mostly used in tests and example code and not in e.g. Xilem. |
|
Example code tends to end up as production code. So more generally speaking, if we're showing people that identifying widgets should happen with tags, then that will be the canonical production way. Which means it isn't great if they're not garbage collected at all. What's your vision for production code (i.e. apps that use Masonry) that needs to identify widgets. Is it |
Fair enough, though I'll note this PR doesn't touch example code, and the problem already exists on main. If you think this is pressing, I'd rather add a TODO comment and implement garbage collection in a separate PR.
Overall, my vision is "WidgetTag if you know which widget you want before it's added to the widget tree, WidgetId if you want to refer to a widget which is already in the tree". Unit tests will always "know" their tree ahead of time, hence why I want TestHarness APIs to use WidgetTag as much possible. |
5436408 to
41d3c68
Compare
|
No I don't think the garbage collection needs to happen in this PR, but if we're moving towards tags being the primary recommendation then I think it makes sense for them to become better in a bunch of ways. One thing that worries me is that tags will just be a level of indirection. You've talked before about taking total control over ids so that a slotmap could be used. I can see the performance benefit of that idea. However, now even if the widget owner knows the id, we would recommend using a tag instead? That seems to be the goal here, especially given your description of this PR moving towards the third path of "3: Use an escape hatch to get a WidgetTag from a WidgetId". So the owner already knows the id, that id would give fast slotmap access, but instead the owner should get some sort of tag created, just so the tag can be converted to the id again. Seems like a lot of busywork for no gain. Also, to be clear, I'm not so much worried about what the test harness does. I'm more so thinking about production usage and whether the overall shift towards tags provides any significant value for the extra indirection cost. |
You'll notice that this PR changes (The RenderRoot APIs that currently use WidgetTag are the ones where WidgetTag's strong typing lets you skip downcasting, which is really nice for keeping example code concise.) |
This is part of an effort to make WidgetTag the main primitive used in tests.
5ff72eb to
2857c1e
Compare
This is part of an effort to make WidgetTag the main primitive used in tests.
Part of this was generated with Claude Code, and then I rewrote almost everything by hand anyway.