Skip to content

Feature: Add internal logging for behavioral debug #708

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

Merged
merged 2 commits into from
Dec 15, 2023

Conversation

penguinolog
Copy link
Collaborator

  • Use standard python logging

  • Use nested loggers for possibility to set atomic logging

  • Add __repr__ to the part of critical types

  • In screen event loop with selectors usage: use IO objects instead of descriptors if possible: get human-friendly output in logs

  • MainLoop methodswatch_pipe / remove_watch_pipe are not available under Windows

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
  • I've included docstrings and/or documentation and/or examples for my code (if this is a new feature)

* Use standard python logging
* Use nested loggers for possibility to set atomic logging
* Add `__repr__` to the part of critical types
* In screen event loop with `selectors` usage:
  use IO objects instead of descriptors if possible:
  get human-friendly output in logs

* `MainLoop` methods`watch_pipe` / `remove_watch_pipe`
  are not available under Windows
@penguinolog penguinolog added the Feature Feature request/implementation label Dec 15, 2023
@coveralls
Copy link

coveralls commented Dec 15, 2023

Pull Request Test Coverage Report for Build 7220483902

  • 64 of 134 (47.76%) changed or added relevant lines in 15 files are covered.
  • 10 unchanged lines in 5 files lost coverage.
  • Overall coverage increased (+0.09%) to 71.046%

Changes Missing Coverage Covered Lines Changed/Added Lines %
urwid/display_common.py 2 3 66.67%
urwid/event_loop/select_loop.py 7 8 87.5%
urwid/event_loop/tornado_loop.py 3 4 75.0%
urwid/_win32_raw_display.py 1 4 25.0%
urwid/curses_display.py 1 4 25.0%
urwid/_posix_raw_display.py 2 7 28.57%
urwid/event_loop/main_loop.py 12 39 30.77%
urwid/_raw_display_base.py 5 34 14.71%
Files with Coverage Reduction New Missed Lines %
urwid/event_loop/main_loop.py 1 77.94%
urwid/event_loop/select_loop.py 1 88.52%
urwid/_posix_raw_display.py 1 12.54%
urwid/_win32_raw_display.py 1 17.84%
urwid/_raw_display_base.py 6 6.19%
Totals Coverage Status
Change from base Build 7210141019: 0.09%
Covered Lines: 7575
Relevant Lines: 10765

💛 - Coveralls

@penguinolog penguinolog requested review from wardi and ulidtko December 15, 2023 09:56
@@ -371,7 +382,7 @@ def get_cols_rows(self) -> tuple[int, int]:
y, x = super().get_cols_rows()
with contextlib.suppress(OSError): # Term size could not be determined
if hasattr(self._term_output_file, "fileno"):
buf = fcntl.ioctl(self._term_output_file.fileno(), termios.TIOCGWINSZ, " " * 4)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is buggy code: ioctl expect Buffer protocol, which includes bytes, but not str


class Screen(BaseScreen, RealTerminal):
def __init__(self, input: FileLikeObj, output: FileLikeObj): # noqa: A002
def __init__(self, input: io.IOBase, output: io.IOBase): # noqa: A002
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Use full type

@@ -171,7 +173,7 @@ def wrapper() -> tuple[list[str], typing.Any] | None:
return self.parse_input(event_loop, callback, self.get_available_raw_input())

fds = self.get_input_descriptors()
handles = [event_loop.watch_file(fd, wrapper) for fd in fds]
handles = [event_loop.watch_file(fd if isinstance(fd, int) else fd.fileno(), wrapper) for fd in fds]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do not break EventLoop API

@penguinolog penguinolog merged commit a166f3a into urwid:master Dec 15, 2023
@penguinolog penguinolog deleted the logging branch December 15, 2023 12:52
try:
watch_handle, pipe_rd = self._watch_pipes.pop(write_fd)
except KeyError:
return False
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is mainly used to trigger a screen update from a signal (like the terminal resize) or from another process/thread. On windows we need something like this, maybe a socket pair connected over loopback?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://github.com/urwid/urwid/blob/master/urwid/_win32_raw_display.py#L247 ->
https://github.com/urwid/urwid/blob/master/urwid/_win32_raw_display.py#L165C77-L165C94 ->
https://github.com/urwid/urwid/blob/master/urwid/_raw_display_base.py#L99

And socket instance is used for sending b"R" to trigger update.
Unfortunately it's not enough, so issue #700 created and in process of debugging, when I have physical access to the Windows machine.

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

Successfully merging this pull request may close these issues.

3 participants