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

Add Windows support for raw_display #448

Closed
wants to merge 1 commit into from

Conversation

mhils
Copy link

@mhils mhils commented Jan 4, 2021

Checklist
  • I've ensured that similar functionality has not already been implemented
  • I've ensured that similar functionality has not earlier been proposed and declined
  • I've branched off the master or python-dual-support branch
  • I've merged fresh upstream into my branch recently
  • I've ran tox successfully in local environment (yes, but I do not have 2.7, 3.5, and pypy at hand)
  • I've included docstrings and/or documentation and/or examples for my code (if this is a new feature)
Description:

Inspired by @asmith-kepler's fantastic work in #447, I set out to implement Windows support for raw_display. Turns out that this is all working pretty seamlessly with the latest Windows console improvements. 🎉

Main changes:

  • Use socket.socketpair() instead of os.pipe() on all OSes as Windows doesn't support non-socket file descriptors in select(). This probably has a tiny amount of overhead, but works across all platforms.
  • Use ctypes to interface with the Windows API, which works without any additional dependencies.
  • Use a dedicated thread to read input from the Windows console and pipe it into another socketpair, which is then processed by raw_display. This is slightly ugly, but works across all events loops.

@coveralls
Copy link

coveralls commented Jan 4, 2021

Coverage Status

Coverage decreased (-0.8%) to 77.084% when pulling f4fd669 on mhils:win32-rawdisplay into 4c739b6 on urwid:master.

mhils added a commit to mhils/mitmproxy that referenced this pull request Jan 4, 2021
self._old_termios_settings = termios.tcgetattr(fd)
tty.setcbreak(fd)

self.signal_init()
if IS_WINDOWS:
hOut = win32.GetStdHandle(win32.STD_OUTPUT_HANDLE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

will be hood if such long branches will be in own methods like:

if IS_WINDOWS:
    self.windows_signal_init()
else:
    self.signal_init()

@@ -274,6 +310,14 @@ def _stop(self):
if self._old_signal_keys:
self.tty_signal_keys(*(self._old_signal_keys + (fd,)))

if IS_WINDOWS:
hOut = win32.GetStdHandle(win32.STD_OUTPUT_HANDLE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

also here windows_stop will be good method

@@ -395,6 +439,10 @@ def unhook_event_loop(self, event_loop):
"""
Remove any hooks added by hook_event_loop.
"""
if self._input_thread is not None:
self._input_thread.should_exit = True
self._input_thread = None
Copy link
Collaborator

Choose a reason for hiding this comment

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

no wait / join?

buf = fcntl.ioctl(self._term_output_file.fileno(),
termios.TIOCGWINSZ, ' '*4)
y, x = struct.unpack('hh', buf)
if IS_WINDOWS:
Copy link
Collaborator

Choose a reason for hiding this comment

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

better to make dedicated methods for both branches

Copy link
Collaborator

@penguinolog penguinolog left a comment

Choose a reason for hiding this comment

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

please split logic to reduce platform specific if/else branches size

@penguinolog penguinolog added the Feature Feature request/implementation label May 1, 2023
@penguinolog penguinolog added the Windows Windows support label Oct 2, 2023
penguinolog added a commit to penguinolog/urwid that referenced this pull request Nov 27, 2023
Restore PR urwid#448 with pick of urwid#447 for `tty_signal_keys`
penguinolog added a commit to penguinolog/urwid that referenced this pull request Nov 27, 2023
Restore PR urwid#448 with pick of urwid#447 for `tty_signal_keys`
penguinolog added a commit to penguinolog/urwid that referenced this pull request Nov 27, 2023
Restore PR urwid#448 with pick of urwid#447 for `tty_signal_keys`
penguinolog added a commit to penguinolog/urwid that referenced this pull request Nov 27, 2023
Restore PR urwid#448 with pick of urwid#447 for `tty_signal_keys`

* Vterm is fully not supported under windows
* Do not import ZMQEventLoop globally due to python crash
* rework test for event loops to be windows compatible:
  on windows `select` do not support pipe

Skip tests:
* TwistedEventLoop is not supported under windows
* ZMQEventLoop is not supported under windows
penguinolog added a commit to penguinolog/urwid that referenced this pull request Dec 7, 2023
Restore PR urwid#448 with pick of urwid#447 for `tty_signal_keys`

* Vterm is fully not supported under windows
* Do not import ZMQEventLoop globally due to python crash
* rework test for event loops to be windows compatible:
  on windows `select` do not support pipe

Skip tests:
* TwistedEventLoop is not supported under windows
* ZMQEventLoop is not supported under windows
penguinolog added a commit to penguinolog/urwid that referenced this pull request Dec 7, 2023
Restore PR urwid#448 with pick of urwid#447 for `tty_signal_keys`

* Vterm is fully not supported under windows
* Do not import ZMQEventLoop globally due to python crash
* rework test for event loops to be windows compatible:
  on windows `select` do not support pipe

Skip tests:
* TwistedEventLoop is not supported under windows
* ZMQEventLoop is not supported under windows
penguinolog added a commit to penguinolog/urwid that referenced this pull request Dec 7, 2023
Restore PR urwid#448 with pick of urwid#447 for `tty_signal_keys`

* Vterm is fully not supported under windows
* Do not import ZMQEventLoop globally due to python crash
* rework test for event loops to be windows compatible:
  on windows `select` do not support pipe

Skip tests:
* TwistedEventLoop is not supported under windows
* ZMQEventLoop is not supported under windows
penguinolog added a commit to penguinolog/urwid that referenced this pull request Dec 7, 2023
Restore PR urwid#448 with pick of urwid#447 for `tty_signal_keys`

* Vterm is fully not supported under windows
* Do not import ZMQEventLoop globally due to python crash
* rework test for event loops to be windows compatible:
  on windows `select` do not support pipe

Skip tests:
* TwistedEventLoop is not supported under windows
* ZMQEventLoop is not supported under windows
penguinolog added a commit to penguinolog/urwid that referenced this pull request Dec 7, 2023
Restore PR urwid#448 with pick of urwid#447 for `tty_signal_keys`

* Vterm is fully not supported under windows
* Do not import ZMQEventLoop on windows due to python crash
* rework test for event loops to be windows compatible:
  on windows `select` do not support pipe

Skip tests:
* ZMQEventLoop is not supported under windows
penguinolog added a commit to penguinolog/urwid that referenced this pull request Dec 7, 2023
Restore PR urwid#448 with pick of urwid#447 for `tty_signal_keys`

* Vterm is fully not supported under windows
* Do not import ZMQEventLoop on windows due to python crash
* rework test for event loops to be windows compatible:
  on windows `select` do not support pipe

Skip tests:
* ZMQEventLoop is not supported under windows
penguinolog added a commit to penguinolog/urwid that referenced this pull request Dec 7, 2023
Restore PR urwid#448 with pick of urwid#447 for `tty_signal_keys`

* Vterm is fully not supported under windows
* Do not import ZMQEventLoop on windows due to python crash
* rework test for event loops to be windows compatible:
  on windows `select` do not support pipe

Skip tests:
* ZMQEventLoop is not supported under windows
* `AsyncioEventLoop`, `TornadoEventLoop` and `TwistedEventLoop`:
  * Issues with asyncio loop (changes between python version, select support)
  * Twisted and tornado tests show race conditions
penguinolog added a commit to penguinolog/urwid that referenced this pull request Dec 7, 2023
* Vterm is fully not supported under windows
* Do not import ZMQEventLoop on windows due to python crash
* rework test for event loops to be windows compatible: on windows `select` do not support pipe
* on windows11 console encoding is detected as cp1252, BTW it's validated manually that cms uses UTF-8
* fix 24 bit colors terminal

Skip tests:
* ZMQEventLoop is not supported under windows
* `AsyncioEventLoop`, `TornadoEventLoop` and `TwistedEventLoop`:
  * Issues with asyncio loop (changes between python version, select support)
  * Twisted and tornado tests show race conditions
penguinolog added a commit to penguinolog/urwid that referenced this pull request Dec 7, 2023
* Vterm is fully not supported under windows
* Do not import ZMQEventLoop on windows due to python crash
* rework test for event loops to be windows compatible: on windows `select` do not support pipe
* on windows11 console encoding is detected as cp1252, BTW it's validated manually that cms uses UTF-8
* fix 24 bit colors terminal

Skip tests:
* ZMQEventLoop is not supported under windows
* `AsyncioEventLoop`, `TornadoEventLoop` and `TwistedEventLoop`:
  * Issues with asyncio loop (changes between python version, select support)
  * Twisted and tornado tests show race conditions
penguinolog added a commit that referenced this pull request Dec 8, 2023
* Restore PR #448 with pick of #447 for `tty_signal_keys`

* Vterm is fully not supported under windows
* Do not import ZMQEventLoop on windows due to python crash
* rework test for event loops to be windows compatible: on windows `select` do not support pipe
* on windows11 console encoding is detected as cp1252, BTW it's validated manually that cms uses UTF-8
* fix 24 bit colors terminal

Skip tests:
* ZMQEventLoop is not supported under windows
* `AsyncioEventLoop`, `TornadoEventLoop` and `TwistedEventLoop`:
  * Issues with asyncio loop (changes between python version, select support)
  * Twisted and tornado tests show race conditions

* Make shared base class for raw display

* Reduce amount of copy-paste
* Base class is abstract
  (too huge difference in concrete implementations)
* use `selectors.DefaultSelector` instead of `select.select`
  Automatically select OS specific optimal route by python stdlib.
@penguinolog
Copy link
Collaborator

Restored updated, tested and merged

@penguinolog penguinolog closed this Dec 8, 2023
@mhils
Copy link
Author

mhils commented Dec 8, 2023

I totally missed the comments in March, my apologies. Thanks for taking care of things, this is super duper fantastic! 😃 🍰 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Feature request/implementation Waiting for response Windows Windows support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants