Detect terminal text-based-browsers better and block them #1483
Conversation
Temporarily unsetting TERM environ so text-browsers aren't detected. This results in an exception which displays an error for text-browsers. Fixes zulip#1475.
Sushmey
force-pushed
the
text-based-browser-freeze
branch
from
6b50465
to
2cdbefb
Compare
| if isinstance(term, str): | ||
| os.environ["TERM"] = term # resetting |
There was a problem hiding this comment.
This certainly shouldn't be in the exceptional block. As it is now, TERM will not be reset if there is no error. If you want something to happen in every case, you could use finally.
The simplest solution would be to reset TERM as soon as the browser is determined. If so, this may be clearer with two try blocks, so you can isolate the 'only-terminal-browsers' error from others.
That is, unless the webbrowser module does other detection of this kind after that point, which could require the environment to be set for later operations?
There was a problem hiding this comment.
Using finally is a very good idea!
Re using 2 try blocks, we don't need that since TERM environ isn't used anywhere during or after unsetting it. Resetting it in finally seems better it seems like the cleaner approach.
| term = os.environ.get("TERM") # Saving the original value | ||
| del os.environ["TERM"] # Temporarily deleting variable |
| # Set TERM environ to be able to check text-browsers | ||
| os.environ["TERM"] = "xterm-256color" |
There was a problem hiding this comment.
Your test changes make the tests pass, but don't test the expectations from the new behavior:
- is the environment variable unchanged?
- can we test specifically what happens if there is no 'valid' webbrowser found?
There was a problem hiding this comment.
I'm a bit confused about writing the tests for this. The TERM environ doesn't really change, how do we test its transitory absence?
Also writing a test specifically for text_browser sounds good but again not sure what we can test. We can assert if TERM environ doesn't exist inos.environ but that's a value we are setting and not what we are fetching from the function.
neiljp
added
the
PR awaiting update
Sushmey
changed the title
What does this PR do, and why?
Opening links in browser for users using text-based browsers results in ZT being frozen indefinitely. This might be because of the loop to open link in browser clashing with urwid loop.
This PR blocks out text-browsers better than what it used to earlier. It does so by temporarily unsetting TERM environ so text-browsers aren't detected.
This results in an exception which displays an error for text-browsers.
External discussion & connections
topicOpening URLs can freeze terminal #T1475How did you test this?
Self-review checklist for each commit