Godzie44/run as widget #73

godzie44 · 2024-10-07T12:03:27Z

Description of change

Tui component has been removed. Integration with third-party applications has been simplified.

orhun · 2024-10-08T06:12:17Z

Can you go through your changes a bit and tell me about the rationales? Comparing widget.rs with the current demo.rs, I don't see how this makes things more simple 🤔

codecov · 2024-10-08T06:14:10Z

Codecov Report

Attention: Patch coverage is 0% with 111 lines in your changes missing coverage. Please review.

Project coverage is 3.31%. Comparing base (19bea73) to head (8395b85).

Files with missing lines	Patch %	Lines
src/lib.rs	0.00%	70 Missing ⚠️
src/tui/mod.rs	0.00%	18 Missing ⚠️
src/tui/event.rs	0.00%	10 Missing ⚠️
src/file.rs	0.00%	9 Missing ⚠️
src/app.rs	0.00%	4 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##            main     #73      +/-   ##
========================================
- Coverage   3.31%   3.31%   -0.00%     
========================================
  Files         19      19              
  Lines       2419    2422       +3     
========================================
  Hits          80      80              
- Misses      2339    2342       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

godzie44 · 2024-10-08T08:55:12Z

Currently it's make possible to handle restart event correctly - because now, only this event requires user implementation. Tui component removed, and manipulation with terminal at restart, or dynamic analysis removed too, I hope that this will allow to integrate binsider nto existing tui's like window-in-window.

About simplification - if you compare with demo.rs, then all event processing is now hidden in the handle_event function, and only cumbersome initialization remains (it can be hidden in the next step of refactoring)

orhun · 2024-10-09T00:09:52Z

I see. I think it would be easier to review this after seeing the usage in action. So if you can incorporate these changes in #45, I can take a look again :)

godzie44 · 2024-10-09T13:11:26Z

You can check this out now

orhun

I was traveling - sorry for the delay. Just got a chance to look at this again :)

orhun · 2024-10-16T07:15:13Z

src/file.rs

-pub struct FileInfo<'a> {
+pub struct FileInfo {
    /// Path of the file.
-    pub path: &'a str,
+    pub path: String,
    /// Arguments of the file.
-    pub arguments: Option<Vec<&'a str>>,
+    pub arguments: Option<Vec<String>>,
    /// Bytes of the file.
-    pub bytes: &'a [u8],
+    pub bytes: Box<[u8]>,


Why do we need to remove the lifetimes here?

Short answer - beacause now all application live in one loop in lib.rs (without recreating it with run function).

More detailed. Reference path: &'a str removed because when wee create a new analyzer in Event::Restart handler live time of analizer (and FileInfo in it) is greater than the lifetime of the variable path from the event. &'a [u8] changed to Box<[u8]> with same reason, file data will be freed but we still need it in an analyzer. I dont think that this is a big problem at all, Box<[u8]> still protects us from copy of the file data, and own file path instead of reference to it - not a big overhead.

orhun · 2024-10-16T07:16:42Z

examples/widget.rs

+    crossterm::event::{KeyCode},
+    Frame,
+};
+use binsider::prelude::Event;


Can you remove the other implicit imports (e.g. binsider::file and so on) and use the prelude module? Feel free to update the prelude if we are not already covering some of the types.

orhun · 2024-10-16T07:17:15Z

examples/widget.rs

+                    binsider::app::Analyzer::new(file_info, 15, vec![]).unwrap();
+
+                state.change_analyzer(analyzer);
+                state.handle_tab().unwrap();


Let's avoid unwraps in the example. I think we can return the error type from the library instead.

orhun · 2024-10-16T07:22:54Z

src/tui/mod.rs

-            .store(self.paused, Ordering::Relaxed);
-        Ok(())
-    }
+pub fn tui_toggle_pause(paused: &mut bool, events: &EventHandler) {


Can you expand on this change a bit and explain the rationale?

I honestly don't like the way that this currently works. Instead of taking a mutable reference to a bool, we should handle this in a more stateful way - that's why I had the Tui struct before.

Looks like we can remove paused variable and simply lock user input at Event::Trace event, and unlock at Event::TraceResult.

orhun · 2024-10-16T07:23:54Z

src/tui/state.rs

@@ -91,6 +91,10 @@ impl<'a> State<'a> {
        state.handle_tab()?;
        Ok(state)
    }
+
+    pub fn change_analyzer(&mut self, analyzer: Analyzer<'a>) {


analyzer is a public field, do we need this function? (if so, let's rename it to update_analyzer)

Agreed, i think more consistently will be remove this

src/tui/mod.rs

…fore loop ends

This will help to embedding binsider into another TUI applications.

godzie44 · 2024-11-04T12:42:37Z

@orhun please check this now

godzie44 mentioned this pull request Oct 7, 2024

Support GDB #29

Open

godzie44 force-pushed the godzie44/run_as_widget branch from 7a1623f to c224619 Compare October 7, 2024 21:13

orhun requested changes Oct 16, 2024

View reviewed changes

godzie44 force-pushed the godzie44/run_as_widget branch from c224619 to 803731d Compare October 24, 2024 18:29

fix(tui): fix panic in event creating loop when application closed be…

b764d58

…fore loop ends

godzie44 force-pushed the godzie44/run_as_widget branch 2 times, most recently from f0fe341 to dc54752 Compare October 27, 2024 13:33

godzie44 marked this pull request as ready for review October 27, 2024 13:33

godzie44 force-pushed the godzie44/run_as_widget branch from dc54752 to 3d912dc Compare October 27, 2024 15:45

feat: introduce "binsider as a widget" mechanism

8395b85

This will help to embedding binsider into another TUI applications.

godzie44 force-pushed the godzie44/run_as_widget branch from 3d912dc to 8395b85 Compare October 27, 2024 15:48

godzie44 requested a review from orhun November 1, 2024 09:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Godzie44/run as widget #73

Godzie44/run as widget #73

godzie44 commented Oct 7, 2024

orhun commented Oct 8, 2024

codecov bot commented Oct 8, 2024 •

edited

Loading

godzie44 commented Oct 8, 2024

orhun commented Oct 9, 2024

godzie44 commented Oct 9, 2024 •

edited

Loading

orhun left a comment

orhun Oct 16, 2024

godzie44 Oct 24, 2024

orhun Oct 16, 2024

godzie44 Oct 24, 2024

orhun Oct 16, 2024

godzie44 Oct 24, 2024

orhun Oct 16, 2024

godzie44 Oct 24, 2024 •

edited

Loading

orhun Oct 16, 2024

godzie44 Oct 24, 2024

godzie44 commented Nov 4, 2024

Godzie44/run as widget #73

Are you sure you want to change the base?

Godzie44/run as widget #73

Conversation

godzie44 commented Oct 7, 2024

Description of change

orhun commented Oct 8, 2024

codecov bot commented Oct 8, 2024 • edited Loading

Codecov Report

godzie44 commented Oct 8, 2024

orhun commented Oct 9, 2024

godzie44 commented Oct 9, 2024 • edited Loading

orhun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

godzie44 Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

godzie44 commented Nov 4, 2024

codecov bot commented Oct 8, 2024 •

edited

Loading

godzie44 commented Oct 9, 2024 •

edited

Loading

godzie44 Oct 24, 2024 •

edited

Loading