Improve variable terminal width support #1074
Conversation
preetmishra
added
the
further discussion required
Rohitth007
changed the title
There was a problem hiding this comment.
@Rohitth007 The first commit looks good behavior-wise and the use of render here is effective 🎉
Code-wise you have some tidying/simplification included that could be a refactor commit first? I'll want to confirm how the auto-autohide interacts with the explicit settings before merging, and that we have tests passing on a commit basis too.
Style wise I'm not convinced by the design after the second commit for wide terminals - did we decide upon that somewhere that I don't remember? (This isn't web app behavior either?)
neiljp
added
the
PR awaiting update
zulipbot
added
size: XL
There was a problem hiding this comment.
@Rohitth007 You've not changed the label here, but you mentioned on the stream so I've done another review here.
It's good to see the not directly related changes in the first commit, but you've added others than I don't quite follow.
The behavior arising from the last commit looks better now, though we can discuss further in the stream whether we perhaps want a vertical line border close on either side (the edge of the users list looks somewhat odd otherwise?), and regarding the interaction between wide and autohide - possibly we could simply add a new 'dynamic' autohide value as the default, which would make this cleaner and retain backwards compatibility?
Maybe if we hide the column that is not in use currently then this problem would be solved.
Is this supposed to be resolved?
What would happen in the else statement that you are suggesting? The second part of the conditional is basically so that every render call should not change body.contents. Most of the time the render call is going to enter else. (Many time actually)
The else part would be a "this shouldn't happen" part :)
For the last commit it may look cleaner with a if x < size_n chain with increasing size_n?
|
@neiljp I think having assert statements in else is unnecessary and disturbs the way it reads. Do we need it? |
|
Re preserving focus, yes it is resolved. |
Rohitth007
force-pushed
the
column-zoom
branch
2 times, most recently
from
f612a07 to
de11cde
Compare
There was a problem hiding this comment.
@Rohitth007 This works well, particularly the autohide which I almost merged - but we don't yet have tests (adapted, or new) and there are a few locations we could tidy.
I mentioned my logic re the asserts/exceptions in the stream, but basically it's a "code shouldn't reach here" check.
Rohitth007
force-pushed
the
column-zoom
branch
4 times, most recently
from
0927d04 to
91dcb9a
Compare
This commit combines the focus setting with autohide handling. This makes it easier to handle focus change of panels since both these functions are often seen used together. This also avoids the situation of the show_panels handling focus as moving to a different panel through focus_panel takes care of that. This also gives the advantage of knowing which panel we are on even if we are in autohide layout hence giving us the ability to manipulate the current panel (Eg: resizing) depending where we are. Tests added.
This commit: * stores `self.controller.autohide` as `self.autohide` to make the attribute call shorter. Tests updated.
This commit moves from a self.autohide which was a bool to self.layout which can take multiple options. This enables addition of new panel layouts in the future. Tests updated.
This commit changes the internal code attributes form `autohide` to `layout` Tests updated.
This commit adds Padding around a fixed total width of MAX_TOTAL_WIDTH (200) of all panels combined. The padding is added around the Frame instead of the body so that even the title and footer have a fixed max width. A LineBox is added around the padded_frame to give it borders in a wide terminal. self.has_border is used to make sure every render call does not create a LineBox. All this is done inside `add_padding_and_border_to_frame` method Tests added.
A dynamic option is added it ZT which adapts to various terminal widths to make it more readable. To shift from using a bool to options `self.autohide` to changed to `self.layout`. This makes ZT behave like autohide when in a small terminal which is defined using MINNORMALWIDTH. After crossing this limit ZT behaves like no-autohide until MAXNORMALWIDTH after which, side panel widths are not fixed anymore and they start occupying space in the ratio 20:60:20. This gives the effect that the side panels are expanding. This stops when we reach the max width set for ZT. To do so this makes use of urwid's render method to get maxcols, a Literal `self.mode` which takes values "small", "normal" and "wide" and existing `show_left_panel` and `show_right_panel` methods with small changes.
This commit make the side panels in autohide mode expand after crossing MAXNORMALWIDTH (160) such that it occupies 20% of the frame this stops after MAX_TOTAL_WIDTH (200) columns is reached. Tests added.
This commit: * Adds recently introduced *layout* config which defaults to `dynamic` to the zuliprc template in README. * Keeps backwards compatible *autohide* config. * Adds spacing after each config. * Comments out *footlinks* as both this and *maximum-footlinks* can't be used together.
Rohitth007
added
PR needs review
|
Heads up @Rohitth007, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the |
neiljp
added
the
area: terminal: layout
What does this PR do?
This PR improves terminal width readability and support for both small and wide terminals.
Partial fix for #1005
topic link
Tested?
Commit flow
Commit 1: ui: Make ZT go to into autohide mode if terminal is small.
Commit 2: ui: Fix center panel width and add padding for wide terminals.
Interactions
Related PR #388
Visual changes
To be added (many cases)