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

Use percentages for relative heights of panels #67

Closed
wants to merge 21 commits into from

Conversation

sumwale
Copy link
Contributor

@sumwale sumwale commented Sep 14, 2020

To avoid cutting off panels altogether, use percentages for the various panels
instead of fixed heights (for all but process panel).

By default the process panel occupies 10% more space than other non-zero ones.

See the comparison in the two attached screenshots for before and after the patch in my default terminal having a size of 140x40.

original
updated

These screenshots compare maximized terminal windows for before and after where the two are similar.
original-fullscreen
updated-fullscreen

To avoid cutting off panels altogether use percentages for the various panels
instead of fixed heights for all but process panel.

By default the process panel occupies 10% more space than other non-zero ones.
@bvaisvil
Copy link
Owner

I had tried using percentages before for arranging these panels, but I wasn't able to come to a good layout. I'll take a look at this and your other PR probably this weekend. Thank you for the contributions!

@bvaisvil
Copy link
Owner

So a few comments -

  • Sometimes there ends up being some wasted space with an extra line (see screenshot - between disks and processes section)
  • One of the constraints on section size might be that they have to be even numbers - for example, in the screenshot with uneven height the instant CPU usage ends up with two different sized columns
  • Expanding and contracting a section (E and M) don't always expand and contract the selected sections - sometimes they expand a different section?
  • You can contract a section completely and then it disappears, which is fine. But there will need to be a way to restore it.

Screenshot from 2020-09-19 15-10-17

@sumwale
Copy link
Contributor Author

sumwale commented Sep 19, 2020

So a few comments -

* Sometimes there ends up being some wasted space with an extra line (see screenshot - between disks and processes section)

I see, but the same issue can be seen with the original code too (see screenshot using original build). The tui library must be trying to fit everything in given constraints and there must be some "residues" which are not being redistributed. Will have to take a look at its code.

original

* One of the constraints on section size might be that they have to be even numbers - for example, in the screenshot with uneven height the instant CPU usage ends up with two different sized columns

Makes sense. Will need changes to the tui library.

* Expanding and contracting a section (E and M) don't always expand and contract the selected sections - sometimes they expand a different section?

Cannot reproduce this problem. Do you see this with Mac or Linux or both? I don't see how this PR can effect that behaviour.

* You can contract a section completely and then it disappears, which is fine. But there will need to be a way to restore it.

Yeah, that's a bit of a problem. This is also the case when one starts with zero size on command-line with and without this change. Pressing TAB actually lands up in that invisible section in both cases but there is no visual feedback though one can expand it back. Can make the change to avoid going down to zero size but the problem with zero size on command-line remains (visually the issue is that pressing TAB lands up in that invisible area with no visual feedback to the user and 'e' does expand it). The command-line issue should be tracked with a separate bug where the fix should be to make the section completely disappear rather than being invisible.

@bvaisvil
Copy link
Owner

I see, but the same issue can be seen with the original code too (see screenshot using original build). The tui library must be trying to fit everything in given constraints and there must be some "residues" which are not being redistributed. Will have to take a look at its code.

I tried reproducing this on the original code for a while and I wasn't able to do it. What screen dimensions did you use?

* One of the constraints on section size might be that they have to be even numbers - for example, in the screenshot with uneven height the instant CPU usage ends up with two different sized columns

Makes sense. Will need changes to the tui library.

I was thinking we could do this in zenith but still providing the specific heights to TUI but figuring what they should be using the percentages and some rules. So for example if we had the CPU section at 20% - screen height was 43 Characters - (43 * .2) = 8.6. From here we could round to nearest even integer to come up with specific section size (8).

* Expanding and contracting a section (E and M) don't always expand and contract the selected sections - sometimes they expand a different section?

Cannot reproduce this problem. Do you see this with Mac or Linux or both? I don't see how this PR can effect that behaviour.

Did my testing on Ubuntu.

Yeah, that's a bit of a problem. This is also the case when one starts with zero size on command-line with and without this change. Pressing TAB actually lands up in that invisible section in both cases but there is no visual feedback though one can expand it back. Can make the change to avoid going down to zero size but the problem with zero size on command-line remains (visually the issue is that pressing TAB lands up in that invisible area with no visual feedback to the user and 'e' does expand it). The command-line issue should be tracked with a separate bug where the fix should be to make the section completely disappear rather than being invisible.

Yes, I have to admit this was a bit of laziness on my part ;).

@bvaisvil bvaisvil mentioned this pull request Sep 27, 2020
bvaisvil added a commit that referenced this pull request Sep 27, 2020
- use an ordered vector to track the geometry of each section (height for now)
  instead of using separate variables for each section
- convert the height percentages into lengths before applying as constraints
- catch terminal resize event and recalculate the lengths as per percentages
- apply the constraints that non-process sections should have even number of
  rows, each section should be at least size 2, process section should have
  at least size 4; this is after accounting for all other section heights
@sumwale
Copy link
Contributor Author

sumwale commented Oct 4, 2020

@bvaisvil I have updated the commit as per the comments. In addition have changed to use an "ordered" vector of sections with their heights instead of separate variables. The vector contains only the sections that are non-zero in size. Additionally it does not depend on a fixed order of sections rather the code will now follow whatever order of sections (from top to bottom) appear in the vector. In future it will be much easier to have vertical splits etc which I think will be required if sensors is added, for example, due to lack of real estate even now. The code to remove sections if zero in size has been updated to only render as per what appears in the vector.

Conversion of percentages to actual heights is a bit more code than desirable due to constraints like each section must be have at least size 2, process section should have at least size 4 etc. It should be fairly straightforward though. Additionally when a section is resized (e/m), then other section sizes are proportionally reduced. Of course, this is only in terms of percentages while the translation to rows may or may not show up when those values are converted to integers (or even numbers) and user may need to press multiple times and that may reduce multiple sections at once. I think this is for the best since the alternative of picking up a random section to reduce the size can cause unexpected sections to shrink (from user's pov).

Please check how it looks and review the code. I have played with sizes and e/m quite a bit, and overall I think it now does about the best it can within the constraints even under extreme (e.g. keep pressing 'e' continuously for one section then other and so on).

@bvaisvil
Copy link
Owner

bvaisvil commented Oct 5, 2020

Really like how this works, thanks for reworking it!

I have noticed an issue, however. When shrinking the window, at some point there is a subtraction with overflow panic:
thread '<unnamed>' panicked at 'attempt to subtract with overflow', src/render.rs:1420:35

            process_index = section_index as i32;
            // floor to nearest integer and set constraint as Min
            process_height = max_section_height.min(required_height.floor().max(4.0) as u16);
            max_section_height -= process_height - 4; // <-- this line panics
            constraints.push(Constraint::Min(process_height));

These were the dimensions I was using to cause the crash:

=>  tput cols
153
=>  tput lines
24

use i32 in calculations and cast to u16 at the last moment where required
after ensuring that the i32's are all positive (>= 2 for heights in this case)
@sumwale
Copy link
Contributor Author

sumwale commented Oct 5, 2020

I have noticed an issue, however. When shrinking the window, at some point there is a subtraction with overflow panic:

Fixed. Using i32's for calculation now and convert to u16 at the last point (heights all ensured to be >=2 by then).

- pass a flag to check if process table had to "borrow" from others to keep its minimum size
  and abort size change due to e/m keys if that is the case
- some other cleanups
@sumwale
Copy link
Contributor Author

sumwale commented Oct 6, 2020

@bvaisvil Created a new cleaned up PR #76 with squashed commit to aid in merge, so closing this one. Thanks.

@sumwale sumwale closed this Oct 6, 2020
@sumwale sumwale deleted the scaling branch October 6, 2020 21:21
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