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

Document and clean up WidgetState #707

Merged
merged 4 commits into from
Oct 22, 2024
Merged

Conversation

PoignardAzur
Copy link
Contributor

No description provided.


Contains the WidgetState type, around which a lot of pass code is based.

The WidgetState documentation has helpful information on its fields and their naming scheme.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this line is helpful. To me, this reads like a comment that the rest of the codebase doesn't have helpful documentation, and so WidgetState is special in that it does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe the doc could be reworded.

What I'm trying to say is "WidgetState is more important than other types. A lot of pass code uses its fields. You should look at its documentation and the pass code will probably be easier to understand."

@@ -34,6 +34,9 @@ macro_rules! impl_context_method {
};
}

// Note - Look at the documentation for WidgetState information on its fields
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Note - Look at the documentation for WidgetState information on its fields
// Note - Look at the documentation for `WidgetState` for information on its fields

Although similarly, I'm not sure who this comment helps.

Comment on lines 287 to 295

#[test]
#[ignore]
fn test_widget_size() {
// See https://github.com/linebender/xilem/issues/706
let state = WidgetState::new(WidgetId::next(), "test");
dbg!(std::mem::size_of_val(&state));
panic!();
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[test]
#[ignore]
fn test_widget_size() {
// See https://github.com/linebender/xilem/issues/706
let state = WidgetState::new(WidgetId::next(), "test");
dbg!(std::mem::size_of_val(&state));
panic!();
}

The comment // See https://github.com/linebender/xilem/issues/706 isn't unhelpful, but should just go above the struct definition.

For most users, this information is readily available in the IDE.

A potentially useful test would be that the size isn't larger than say 256 bytes, but that's not what this is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For most users, this information is readily available in the IDE.

Oh, I didn't think of that.

Copy link
Contributor

Choose a reason for hiding this comment

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

For most users, this information is readily available in the IDE.

Yeah it's super useful in rust-analyzer I use that a lot, and another pretty cool thing that recently landed in rust-analyzer (I'm not sure if it's stable though yet), is trait-object-safety (and the reason why it's not) that is shown when "hovering" above a trait.

Remove redundant test.
@PoignardAzur PoignardAzur added this pull request to the merge queue Oct 22, 2024
Merged via the queue into main with commit 9c397da Oct 22, 2024
17 checks passed
@PoignardAzur PoignardAzur deleted the rewrite_widget_state_doc branch October 22, 2024 09:30
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.

3 participants