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

Issue2936 #6799

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Issue2936 #6799

wants to merge 4 commits into from

Conversation

gellge
Copy link

@gellge gellge commented Apr 14, 2024

Description

My attempt at increasing header name line-wrap length according to unused console columns. Specifically:

  • line-wrap length is never lower than 30 columns.
  • it is increased to use as many unused columns as possible, as long as this does not cause the longest amongst the header values to be wrapped.
  • column gaps and indentation whitespace are accounted for when calculating unused columns.

Fixes #2936 .

Checklist

  • I have updated tests where applicable.
  • I have added an entry to the CHANGELOG.

Copy link
Member

@mhils mhils left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! 😃 🍰

@@ -67,6 +68,26 @@ def format_keyvals(
if indent > 2:
indent -= 2 # We use dividechars=2 below, which already adds two empty spaces

cols, _ = urwid.raw_display.Screen().get_cols_rows()
Copy link
Member

Choose a reason for hiding this comment

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

I haven't tested this, but this looks like it will fall apart when using layouts that don't span the entire width. Can we pass in the available space instead?

Copy link
Author

@gellge gellge Apr 17, 2024

Choose a reason for hiding this comment

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

Hi @mhils ,
thanks for the review and the awesome work you people do here!

I'm not sure I'm following you: my (admittedly empirical) tests proved the fix to
work on layouts smaller than full screen (see attached screenshot - header values are not wrapped even if the terminal size is half that of the screen, instead the header name is wrapped at 30 chars). Are there different scenarios you're thinking of?

Furthermore, I investigated a bit more and saw that mitmproxy already uses the same way to retrieve screen columns elsewhere, for instance in mitmproxy/tools/console/statusbar.py (note that self.master.ui inherits from urwid.raw_display.Screen as it can be seen in mitmproxy/tools/console/window.py).

Anyhow I am by no means an expert on urwid, so I'll be glad to refine the PR if you still think this approach is broken.

Screenshot:
mitmproxy_pr

Copy link
Member

Choose a reason for hiding this comment

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

Are there different scenarios you're thinking of?

\You can press - to toggle between views. If you enable the two column view, urwid.raw_display.Screen().get_cols_rows() will give you twice the space available:

image

The statusbar "gets away" with this by always being full width.

After looking into this for a bit, the correct fix is unfortunately more complex. Urwid widgets famously have no widths, so we'd probably need to implement a custom widget here or something like that. :/ I haven't looked super close, but it looks a bit ugly.

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.

header names are needlessly line-wrapped
2 participants