select windows to display from CLI #107
Conversation
4c53c47 to
500a4bc
Compare
|
Hey - so this is a really good start. I think before the tests or the CLI arguments and such, we should try to figure out the functionality. Right now with this code, when I choose one window I see it on only half of the screen. I suggest we start by fixing this and then making sure it behaves well in all the terminal-window sizes. Once we do that, we can figure out how we'd like the CLI arguments to work (and documented in the help), and how to test all this. What do you think? |
|
Yes, I have it in plan. I will be able to look into it maybe tomorrow. If we stick to the single window, the math should be simple, since we want the table to take the whole available screen, as long as it's above the minimum. |
|
Now it should be working fine both for 1 and 2 windows. There are a bunch of if-else that I think can be handled more idiomatically but the logic is there. |
|
So far I've only looked at the functionality and it looks pretty cool. I like the CLI interface that lets me just add the windows I want by name: simple and straightforward. When we have just 1 single window now and a large terminal window (eg. full screen terminal), the columns seem a little dense. I'm fine with them not being spread to the whole screen (that would probably be uncomfortable), but I think we can give them more room to "breathe" (this is especially true to the connections table). Do you think we might be able to spread them out a little? Maybe by adding another breakpoint here and playing around with the column widths somehow? bandwhich/src/display/components/table.rs Line 15 in a3a108a |
|
I did something and it seems to be working fine: if the size is below the third breakpoint, it behaves the same as before, otherwise it expands according to the size and the behavior might slightly differ from the fixed size we had before but I don't see any usability issue in it. |
ae8a35d to
696e76f
Compare
696e76f to
a2477f7
Compare
8a18cfd to
f382bf9
Compare
|
@imsnif I think it's ready for review: tests are passing and there are no conflicts. |
|
@chobeat - my apologies that it's taking me time to get to this. I already started reviewing this last night - this is the next bandwhich thing I'm getting to. |
There was a problem hiding this comment.
Hey - solid work!
I really like this functionality, as well as the implementation. I think the idea of allowing the user to pass in as many or as few tables to render as they like is gold. I'm looking forward to releasing this change.
I left some nitpicks in the review. Once these are addressed I'd be very happy to merge this.
src/display/components/layout.rs
Outdated
| fn build_layout(&self, rect: Rect) -> Vec<Rect> { | ||
| if self.children.len() == 1 { | ||
| // if there's only one element to render, it can take the whole frame | ||
| self.progressive_split(rect, vec![]) |
There was a problem hiding this comment.
Can't we just return vec![rect] here?
src/display/components/table.rs
Outdated
| THIRD_COLUMN_WIDTHS[2], | ||
| rect.width * 53 / 100, | ||
| rect.width * 22 / 100, | ||
| rect.width * 23 / 100 - 1, |
There was a problem hiding this comment.
I like this approach - do you think it will be possible to place the percentages in static consts for clarity (like the FIRST_COLUMN_WIDTHS etc? Maybe something like MAX_FIRST_COLUMN_WIDTH_PERCENTAGE or preferably a clearer shorter name if you can think of one? :)
src/display/ui.rs
Outdated
| let opts = &self.opts; | ||
| let mut children: Vec<Table> = Vec::new(); | ||
| if opts.processes { | ||
| children.push(self.build_processes_table()); |
There was a problem hiding this comment.
Can we use Table::create_processes_table(...) here directly? Then we can get rid of the build_*_table methods above.
| @@ -112,6 +112,115 @@ fn pause_by_space() { | |||
| assert_snapshot!(&terminal_draw_events_mirror[2]); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn basic_only_processes() { | |||
There was a problem hiding this comment.
These tests are great! Let's add some data to them as well, ok? Just one additional snapshot per test with data. What do you think?
|
all done |
There was a problem hiding this comment.
Hey @chobeat - I was about to merge this and then noticed that when running this locally, it overflowed a little for me in certain situations (in a way that overrode the table border). So I pushed a small fix that just reduces the percentages of the second and third column by a bit and adjusted the snapshots.
Once the tests pass, I'll merge this.
This is a really great feature, and you've done great work. Thanks for bearing with me! :)
I very much hope to see more contributions from you if you want.
|
Also, @chobeat - I would like to add you to the "Authors" section of our Cargo.toml, since you've made some major contributions to the project. Would you be willing for your name and email address to appear there? |
No description provided.