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.

Sign up for GitHub

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

Jump to bottom

Issue2936 #6799

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

Issue2936 #6799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a28bcad
use COL_GAP constant instead of hard-coded value
gellge Apr 13, 2024
30a5b49
increase header name length if there are unused columns
gellge Apr 13, 2024
970da68
update CHANGELOG.md
gellge Apr 13, 2024
61a3b0f
[autofix.ci] apply automated fixes
autofix-ci[bot] Apr 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@
([#6790](https://github.com/mitmproxy/mitmproxy/pull/6790), @mhils)
* Fix a bug when proxying unicode domains.
([#6796](https://github.com/mitmproxy/mitmproxy/pull/6796), @mhils)
* Update header name line-wrapping according to unused console columns.
([#6799](https://github.com/mitmproxy/mitmproxy/pull/6799), @gellge)


## 07 March 2024: mitmproxy 10.2.4
Expand Down
23 changes: 22 additions & 1 deletion mitmproxy/tools/console/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ def highlight_key(text, key, textattr="text", keyattr="key"):


KEY_MAX = 30
COL_GAP = 2


def format_keyvals(
Expand All @@ -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 •
edited

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.

available_cols = cols - (
indent + 2 * COL_GAP
) # total console columns minus the indent whitespace and the column gaps
key_lengths = []
val_lengths = []
for k, v in entries:
if k is not None:
key_lengths.append(len(k))
if v is not None and isinstance(v, str):
val_lengths.append(len(v))
max_entry_key_len = max(key_lengths, default=0)
max_entry_val_len = max(val_lengths, default=0)
unused_cols = available_cols - (max_entry_key_len + max_entry_val_len)
max_key_len = (
max(KEY_MAX, available_cols - max_entry_val_len)
if unused_cols < 0
else max_entry_key_len
)

ret = []
for k, v in entries:
if v is None:
Expand All @@ -80,7 +101,7 @@ def format_keyvals(
("fixed", max_key_len, urwid.Text([(key_format, k)])),
v,
],
dividechars=2,
dividechars=COL_GAP,
)
)
return ret
Expand Down