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 column sorting feature on table #26

Merged
merged 1 commit into from
Mar 27, 2024

Conversation

tyrone-wu
Copy link
Contributor

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

Added functionality for sorting table based on a column. #2

Most of the added code is for the new sorting feature.
I modified the BpfProgram class with additional fields from the columns (period avg runtime, total avg runtime, events per sec, cpu time percent) so that they don't need to be computed twice when first displaying to UI, and then when sorting.
I also added a new method for initializing a BpfProgram object so that it doesn't take up too much lines (which was in conjunction to the additional fields).

@tyrone-wu tyrone-wu mentioned this pull request Mar 23, 2024
@tyrone-wu tyrone-wu marked this pull request as ready for review March 23, 2024 21:01
@tyrone-wu
Copy link
Contributor Author

tyrone-wu commented Mar 23, 2024

I don't have as much programs to work with at the moment, but here's how it looks like:
sort

Edit: update to correct gif

@jfernandez jfernandez self-requested a review March 24, 2024 16:30
@jfernandez
Copy link
Contributor

@tyrone-wu I think you pasted the gif for the filter feature.

I tried the feature locally and it's working great. The only thing I'd add functionally is to make Enter do the following when in sorting mode:

  • If no sorting is enabled on the column, sort by DESC and exit sorting mode
  • If sorting is already enabled, swap the direction and exit sorting mode

I don't think we need to add this to the footer, just make it a hidden feature for people that may want to do this due to muscle memory from other top-like apps.

@tyrone-wu tyrone-wu marked this pull request as draft March 24, 2024 23:03
@tyrone-wu tyrone-wu force-pushed the enh/sortable-columns branch 2 times, most recently from ce51c12 to aeae1e7 Compare March 25, 2024 16:05
@tyrone-wu
Copy link
Contributor Author

The new hidden feature is now added! 👍
I also corrected the gif from earlier 😅, and here's the hidden feature in action.
sort_hidden_feature

@tyrone-wu tyrone-wu marked this pull request as ready for review March 25, 2024 16:20
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.

Functionally this is working great. I had some questions about some design choices.

src/app.rs Outdated
@@ -29,15 +29,20 @@ use tui_input::Input;

use crate::bpf_program::BpfProgram;

const NUM_COLUMNS: i8 = 7;
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that we used to have a HEADER_COLUMNS constant in main.rs and then dropped in this PR. Can we bring that back and then ask its size? I worry that by having more than one place where we store the number of columns may lead to bugs.

Copy link
Contributor Author

@tyrone-wu tyrone-wu Mar 26, 2024

Choose a reason for hiding this comment

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

The main.rs HEADER_COLUMNS was moved into app.rs as header_columns field. This was so that the UI doesn't have to re-concat the header text and sort symbol in the draw loop.
In the PR, app.rs handles the text headers, which also handles applying the appropriate sort symbol to the selected header column. The main.rs now just renders the header text from the header_columns field in app.rs.

But I updated it so that the i8 const is no longer needed. It should now only rely on header_columns.len() when getting the size.

if self.period_ns == 0 {
return 0.0;
}
self.runtime_delta() as f64 / self.period_ns as f64 * 100.0
}

pub fn update_stats(&mut self) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we sort without having to cache the value? I would prefer not doing this caching unless it was absolutely necessary

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 sort is now updated without using the caching, and bpf_program.rs should now be reverted to what it previously was as well. 👍

src/main.rs Show resolved Hide resolved
@jfernandez jfernandez merged commit 1c80994 into Netflix:main Mar 27, 2024
1 check passed
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