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

Noline support MVP #24

Merged
merged 3 commits into from
Aug 30, 2024

Conversation

eivindbergem
Copy link
Contributor

I've done a quick implementation and got something working. There are some remaining issues that need to be figured out:

  • The prompt presents some problems. Noline only supports a static prompt. I tried to see if I can implement a dynamic prompt, but it will require big changes. I looked into supporting any prompt that implements Display. That would work with blocking IO, but since we don't have write_fmt for embedded_io_async::Write it would break async support in noline.
  • I added embedded_io as dependency and used Write from there instead of core::fmt::Write.
  • I changed Runner so that buffer is not owned, but instead is an argument to input_byte.

#23

@thejpster
Copy link
Member

thejpster commented Aug 27, 2024

I changed Runner so that buffer is not owned, but instead is an argument to input_byte.

I think this means the user of the crate is now free to fiddle with the contents of the buffer (even adding non-UTF-8 bytes). What prompted the change?

examples/noline.rs Outdated Show resolved Hide resolved
examples/noline.rs Outdated Show resolved Hide resolved
@eivindbergem
Copy link
Contributor Author

I changed Runner so that buffer is not owned, but instead is an argument to input_byte.

I think this means the user of the crate is now free to fiddle with the contents of the buffer (even adding non-UTF-8 bytes). What prompted the change?

Editor already owns it own buffer, so it was the shortest path to get something working. A possible solution is to make Runner generic over the buffer and have to concrete implementations with &mut [u8] and Editor.

@thejpster
Copy link
Member

Hmm. We might need a trait or something to abstract across either a raw buffer or an Editor. Or we can create some BasicEditor functionality which does what the crate currently does.

@eivindbergem
Copy link
Contributor Author

I've made Runner generic over buffer, allowing either anything that implements AsMut<[u8]>, or Editor, depending on which constructor is used. No traits needed.

I've also added support for the dynamic prompt in noline. I've pointed the noline crate to the branch for the prompt support for now. I'll make a new release of noline when this is ready.

@eivindbergem
Copy link
Contributor Author

I had to move the MenuManager to an InnerRunner to avoid Lifetime issues with the prompt, and pass interface as an argument to all the methods. I guess all the methods from InnerRunner could be moved into MenuManager itself, but I'll let you decide on how that is best solved.

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

This looks OK to me. Thank you!

As you've re-written the prompt handling, would you mind looking at #25? I spotted this whilst testing.

@thejpster
Copy link
Member

thejpster commented Aug 30, 2024

Thank you. Whilst testing I spotted that clippy was warning about unused imports - we now check clippy in CI in #26.

Would you mind rebasing and looking at the clippy output?

@eivindbergem eivindbergem force-pushed the noline-support branch 2 times, most recently from 2e1c0db to 637cb6c Compare August 30, 2024 13:15
@eivindbergem
Copy link
Contributor Author

Ok, now I've fixed everything, I hope :)

@thejpster thejpster added this pull request to the merge queue Aug 30, 2024
Merged via the queue into rust-embedded-community:master with commit d3bfe52 Aug 30, 2024
3 checks passed
@thejpster
Copy link
Member

Thank you!

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