Godzie44/run as widget by godzie44 · Pull Request #73 · orhun/binsider

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

❌ 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).
⚠️ Report is 22 commits behind head on main.

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.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

-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

+    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

+                    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

-            .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

        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

…fore loop ends

This will help to embedding binsider into another TUI applications.

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

@orhun please check this now

orhun · 2025-03-21T20:00:12Z

I got sidetracked for a couple of months and lost interest, sorry.

I'll probably get back to this when I start working on binsider again. Is the integration in bugstalker still depending on this?

godzie44 · 2025-03-25T17:46:28Z

Hi! No problem. Yep this change need for integration. Integration already done too godzie44/BugStalker#45 (however, this PR is somewhat outdated).

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

Uh oh!

Conversation

godzie44 commented Oct 7, 2024

Description of change

Uh oh!

orhun commented Oct 8, 2024

Uh oh!

codecov Bot commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

godzie44 commented Oct 8, 2024

Uh oh!

orhun commented Oct 9, 2024

Uh oh!

godzie44 commented Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orhun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

godzie44 Oct 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

godzie44 commented Nov 4, 2024

Uh oh!

orhun commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

godzie44 commented Mar 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Oct 8, 2024 •

edited

Loading

godzie44 commented Oct 9, 2024 •

edited

Loading

godzie44 Oct 24, 2024 •

edited

Loading

orhun commented Mar 21, 2025 •

edited

Loading