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

Incorrect doc on push_item_width #661

Closed
linw789 opened this issue Aug 23, 2022 · 2 comments · Fixed by #666
Closed

Incorrect doc on push_item_width #661

linw789 opened this issue Aug 23, 2022 · 2 comments · Fixed by #666

Comments

@linw789
Copy link
Contributor

linw789 commented Aug 23, 2022

push_item_with (and related functions) comes with the comment:

    /// Returns an `ItemWidthStackToken` that may be popped by calling `.pop()`

but ItemWidthStackToken doesn't seem to have a pop function. Instead, it seems that Drop should be used. Is the correct usage to create a scope and call push_item_with inside it?

@dbr
Copy link
Contributor

dbr commented Aug 29, 2022

Yep well spotted, we should fix up these docs. I think this was just missed in the large #440 PR where we moved to using drop for these tokens

Is the correct usage to create a scope and call push_item_with inside it?

Yep exactly - you can also manually call .end() on the token if it's more convinient (which works like the old .pop() method)

@linw789 linw789 mentioned this issue Aug 30, 2022
@dbr dbr closed this as completed in #666 Sep 2, 2022
@dbr
Copy link
Contributor

dbr commented Sep 2, 2022

Thanks for the PR!

It was changed to .end as the macro code for the tokens is shared across all similar tokens, including those used for things like ui.window(...) (where .end() makes more sense)

We could have maybe had separate macros for both an "end token" and "pop token" (or had both methods), but there's not a huge amount of benefit I think (both names are kind of interchangable, and the preferred way is just to use the drop impl)

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 a pull request may close this issue.

2 participants