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

Taking ownership of menu in runner #15

Merged
merged 7 commits into from
Nov 15, 2023
Merged

Taking ownership of menu in runner #15

merged 7 commits into from
Nov 15, 2023

Conversation

ryan-summers
Copy link
Member

This PR refactors the Runner to take ownership of the root menu. This enables the menu to be created on an ad-hoc basis (i.e. not at the crate root) to be handed to the Runner without lifetime worries.

The impact is that we can't keep references to the menu internally, so we have to traverse the menu throughout operation. Open to some suggestions.

For an example as to why this is needed, refer to https://github.com/quartiq/stabilizer/pull/813/files#diff-f0b20facc8d3d2dfccda7199e70cb8af5ba9255d80e1d35fdd950dc6a2d567a3R30

I'm trying to build a generic "Serial Console Settings Manager" that uses menu for providing the UI for settings management

@thejpster
Copy link
Member

thejpster commented Nov 15, 2023

The Runner object takes a &Menu so that the Menu object can live in .rodata in an immutable static. This change will mean the menu gets copied on to the stack, and could potentially use 100s of bytes (I'm guessing - I haven't checked).

Maybe we could change the runner to take AsRef<Menu>? Then it will work with either Menu or &Menu, like those Standard Library APIs that take AsRef<str> or AsRef<Path>?

@thejpster
Copy link
Member

Wait, no. I checked and my OS_MENU is only 24 bytes. Then I remembered that the menu items live in their own slice, so the Menu is just two Option<function pointers>, a &[Item] and a &str. 6 words, so 24 bytes. All checks out.

So this PR seems fine then.

src/lib.rs Outdated Show resolved Hide resolved
@thejpster
Copy link
Member

I think struct Menu will need to be clone, otherwise if it's a static you can't get it into the runner.

You can't derive(Clone) because rustc things that the <T> also needs to be cloneable, even though th Menu doesn't actually hold a T. So we just implement the trait manually.
@thejpster
Copy link
Member

Added a impl Clone for Menu

@thejpster
Copy link
Member

Sheesh, I haven't looked at this in ages. Have tidied up a bit.

@thejpster thejpster added this pull request to the merge queue Nov 15, 2023
Merged via the queue into master with commit bcc2488 Nov 15, 2023
4 checks passed
@ryan-summers ryan-summers deleted the feature/owned-menu branch November 16, 2023 09:16
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