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

Added filters for Name and Type #22

Merged
merged 1 commit into from
Mar 18, 2024

Conversation

tyrone-wu
Copy link
Contributor

@tyrone-wu tyrone-wu commented Mar 14, 2024

Added input fields for filtering Name and Type of BPF program. #5

@tyrone-wu tyrone-wu marked this pull request as ready for review March 14, 2024 18:03
@tyrone-wu tyrone-wu changed the title Added filter input fields for Name and Type Added filterd for Name and Type Mar 15, 2024
@tyrone-wu tyrone-wu changed the title Added filterd for Name and Type Added filters for Name and Type Mar 15, 2024
@jfernandez jfernandez self-requested a review March 15, 2024 04:29
@jfernandez
Copy link
Contributor

Thanks @tyrone-wu. I'll make sure to check it out tomorrow. It would help to provide screenshots or a video of the new feature.

@jfernandez
Copy link
Contributor

Could you also squash the commits into just one?

@tyrone-wu tyrone-wu force-pushed the enh/filter-name-type branch from 0205780 to 0d21b79 Compare March 15, 2024 05:49
@tyrone-wu tyrone-wu marked this pull request as draft March 15, 2024 06:07
@tyrone-wu tyrone-wu force-pushed the enh/filter-name-type branch from 0d21b79 to dd176ef Compare March 15, 2024 15:23
@tyrone-wu tyrone-wu marked this pull request as ready for review March 15, 2024 15:25
@tyrone-wu tyrone-wu force-pushed the enh/filter-name-type branch from dd176ef to 09e3811 Compare March 15, 2024 15:41
@tyrone-wu
Copy link
Contributor Author

tyrone-wu commented Mar 15, 2024

The commits are squashed now. 👍 (and sorry about the messy forced pushes 🙃)
And here's a small demo of filters in action.
filters

@tyrone-wu tyrone-wu force-pushed the enh/filter-name-type branch from 09e3811 to 824896c Compare March 15, 2024 16:21
@jfernandez
Copy link
Contributor

Looks great. I'll take a closer look this weekend

@jfernandez
Copy link
Contributor

@tyrone-wu I tried it out today and have some feedback.

I'm concerned about using up all space in the footer. If we were to add more features, there wouldn't be much room left. I suggest just adding another option to to the existing footer to toggle the filter input field, something like this:

(q) quit | (↑,k) move up | (↓,j) move down | (↵) show graphs | (f) filter

When f is pressed, then show the filter input.

Also, I think we only need one filter field that works on both the name and type of the program, we don't need separate ones.

Remove the blinking, I don't think it helps. But I like the golden border color.

@tyrone-wu
Copy link
Contributor Author

tyrone-wu commented Mar 17, 2024

For the graph and table state of the UI, would it be alright if I refactor it to use a Mode enum instead of the show_graphs bool for representing the UI state? I was thinking of using the Mode enum to have it also hold a Mode::Filter state (and also possibly in the future a sorting state for #2).

Some of the refactoring would end up looking something like this:

match app.mode {
    Mode::Table => match key.code {
        KeyCode::Down | KeyCode::Char('j') => app.next_program(),
        KeyCode::Up | KeyCode::Char('k') => app.previous_program(),
        KeyCode::Enter => app.toggle_graphs(),
        KeyCode::Char('f') => app.toggle_filter(),
        KeyCode::Char('q') | KeyCode::Esc => return Ok(()),
        _ => {}
    },
    Mode::Graph => match key.code {
        KeyCode::Enter | KeyCode::Esc => app.toggle_graphs(),
        KeyCode::Char('q') => return Ok(()),
        _ => {}
    }
    Mode::Filter => match key.code {
        KeyCode::Enter => app.toggle_filter(),
        _ => {
            app.type_input
                .lock()
                .unwrap()
                .handle_event(&Event::Key(key));
        }
    },

@tyrone-wu tyrone-wu marked this pull request as draft March 17, 2024 17:05
@jfernandez
Copy link
Contributor

jfernandez commented Mar 17, 2024

For the graph and table state of the UI, would it be alright if I refactor it to use a Mode enum instead of the show_graphs bool for representing the UI state?

Yes, that seems like a good evolution for the code. Go for it!

@tyrone-wu tyrone-wu force-pushed the enh/filter-name-type branch from 94d4526 to 25894b5 Compare March 18, 2024 01:43
@tyrone-wu tyrone-wu marked this pull request as ready for review March 18, 2024 01:45
@tyrone-wu
Copy link
Contributor Author

The updated version that's ready for review!
Here's the new design:
filters

Copy link
Contributor

@jfernandez jfernandez left a comment

Choose a reason for hiding this comment

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

It's almost there. I have one small request around explicitly dropping the mutex lock.

src/app.rs Outdated
@@ -71,6 +82,8 @@ impl App {
.map(|prog| (prog.id.clone(), prog))
.collect();

let filter = filter.lock().unwrap().value().to_lowercase();
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe by re-assigning filter you are immediately dropping the lock, which is what we want. But I would prefer doing this more explicitly, something like:

let filter = filter.lock.unwrap();
let filter_str = filter.value().to_lowercase():
drop(filter); 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The explicit mutex drop is added now. 👍

@tyrone-wu tyrone-wu force-pushed the enh/filter-name-type branch from 25894b5 to 946756c Compare March 18, 2024 02:45
Copy link
Contributor

@jfernandez jfernandez left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for taking the initiative to add this feature and being responsive to feedback. I will cut a 0.3.0 release with this change.

@jfernandez jfernandez merged commit 4de0e3a into Netflix:main Mar 18, 2024
1 check passed
@roger-cruz
Copy link

A vote of support for the filtering capability. Thanks Tyrone

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.

3 participants