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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support using heh as a Ratatui widget #120

Merged
merged 7 commits into from
Apr 8, 2024

Conversation

orhun
Copy link
Contributor

@orhun orhun commented Mar 30, 2024

Hey! 馃惢

I'm currently working on a TUI that allows analyzing ELF files and I would like to have heh as a component in there because I think it is a pretty awesome app! (and I don't want to rewrite the same functionality :D)

This PR simply makes that possible!

The summary of the changes:

  • Created lib.rs for using heh as a library
  • Made some types public (primarily Application, Encoding, ScreenHandler, ComponentLayouts)
  • Added a new function Application::render_frame for rendering a single frame.
    • This can be refactored into impl ratatui::widgets::Widget for Application but it requires some additional work on restructuring the internal types.
    • There is a bit of a code duplication there which I couldn't avoid due to ScreenHandler holding some state - i.e. I can't mutate it inside the terminal.draw closure.
  • Added documentation

See this commit for the implementation in my app.

Demo (see the "hexdump" tab):

rec_20240331T002725

Cheers!

Moves the module definitions to lib.rs and exposes some types for usage
Adds App::render_frame method
Copy link

codecov bot commented Mar 30, 2024

Codecov Report

Attention: Patch coverage is 1.97368% with 149 lines in your changes are missing coverage. Please review.

Project coverage is 20.5%. Comparing base (a044b9c) to head (91e8db5).

Additional details and impacted files
Files Coverage 螖
src/decoder.rs 98.7% <酶> (酶)
src/label.rs 73.3% <100.0%> (酶)
src/main.rs 0.0% <酶> (酶)
src/windows/unsaved_changes.rs 0.0% <0.0%> (酶)
src/windows/jump_to_byte.rs 0.0% <0.0%> (酶)
src/windows/search.rs 47.1% <0.0%> (酶)
src/windows/editor.rs 0.0% <0.0%> (酶)
src/windows/mod.rs 8.1% <0.0%> (酶)
src/app.rs 0.0% <0.0%> (酶)
src/screen.rs 20.9% <2.4%> (-1.1%) 猬囷笍

@ndd7xv
Copy link
Owner

ndd7xv commented Mar 30, 2024

Wow, this is amazing! I'll try and review and come back to you in a couple of days. I've been really busy with life lately, so I haven't had the time to really give this repo as much love as I want.

You demo looks really really cool, and I'm glad you could make use of heh :)

@orhun
Copy link
Contributor Author

orhun commented Mar 30, 2024

Wow, this is amazing! I'll try and review and come back to you in a couple of days.

Thanks a lot! I will be looking forward to your review and happy to improve the code 馃惢

I've been really busy with life lately, so I haven't had the time to really give this repo as much love as I want.

I'm thinking of submitting some other PRs, mainly about updating Ratatui to the latest version. Happy to work on other issues as well.

You demo looks really really cool, and I'm glad you could make use of heh :)

Right now it is fully functional and I am so satisfied with it. Thanks for building heh, it's a killer!

Copy link
Owner

@ndd7xv ndd7xv left a comment

Choose a reason for hiding this comment

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

Thanks for waiting, I think this looks good for the most part!

One thing I wanted to get your input on was the module_name_repititions clippy error that we're getting with LabelHandler, ScreenHandler, and AppData - to fix this, I'm thinking of changing the names of these structs right now and users of the library would have to specify screen::Handler, label::Handler, and app::Data. I'm of the mind that this is probably better for the longevity of heh as a library, but was curious what you thought.

There are other clippy lints failing, but if you don't have the time to fix them, I can go ahead and get around to cleaning the up after I push this through -- I don't wanna block any progress on your work!

Also let me know if after this gets merged, you would want me to make a release :)

```rust
heh.handle_input(crossterm::event::Event::Key(/* */)).unwrap();
```

Copy link
Owner

Choose a reason for hiding this comment

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

Feel free to add a link to binsider as an example use case of using this as a library!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

Added in 0d77149

src/app.rs Show resolved Hide resolved
}

/// Renders a single frame for the given area.
pub fn render_frame(&mut self, frame: &mut Frame, area: Rect) {
Copy link
Owner

Choose a reason for hiding this comment

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

Would you be willing to create an issue so that at some point I remember to create tests specifically for render_frame and handle_input? I'll have to look into it but I think I'll want more coverage on these now that they'll be library functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Beep beep, you got it!
#124

@orhun
Copy link
Contributor Author

orhun commented Apr 7, 2024

One thing I wanted to get your input on was the module_name_repititions [...] to fix this, I'm thinking of changing the names of these structs right now and users of the library would have to specify screen::Handler, label::Handler, and app::Data

I think that would be perfect! I personally never liked the name "Handler" (reminds me of Java 馃ぎ)

Do you want me to do it in this PR or are you going to work on that separately?

There are other clippy lints failing, but if you don't have the time to fix them, I can go ahead and get around to cleaning the up after I push this through -- I don't wanna block any progress on your work!

Works fine for me, thank you!

Also let me know if after this gets merged, you would want me to make a release :)

Please do 馃檪

@ndd7xv
Copy link
Owner

ndd7xv commented Apr 7, 2024

Do you want me to do it in this PR or are you going to work on that separately?

I'd appreciate if you could do it in this PR! Haha you can tell I come from Java xD

After the names are fixed to your liking, I'll merge this and hopefully have the time to push a release 馃憤

@orhun
Copy link
Contributor Author

orhun commented Apr 7, 2024

Okay, one issue:

use crate::{app::Data, label::Handler, screen::Handler};

In that case we are importing both Handlers which causes a conflict. We can solve this by using a type alias (e.g. as LabelHandler) but I'm not sure. What do you think?

I will think of an alternative name.

@ndd7xv
Copy link
Owner

ndd7xv commented Apr 7, 2024

Using an alias is what I had in mind! I completely trust whatever alternative names you come up with too.

@orhun
Copy link
Contributor Author

orhun commented Apr 7, 2024

Okay, all should be good now. I renamed the types and fixed the lints.

Let me know 馃檪

Copy link
Owner

@ndd7xv ndd7xv left a comment

Choose a reason for hiding this comment

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

Thanks! I'll try and get a release out soon, I really appreciate your PR :)

@ndd7xv ndd7xv merged commit 371260c into ndd7xv:main Apr 8, 2024
13 of 14 checks passed
@ndd7xv
Copy link
Owner

ndd7xv commented Apr 8, 2024

Just created a release 馃コ let me know if there's anything else I should do, thanks again!

@orhun
Copy link
Contributor Author

orhun commented Apr 8, 2024

Awesome, thanks a lot for your cooperation on this!

Here I bumped the heh version and everything is seamless!

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.

None yet

2 participants