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

First take on cursor position #32

Closed
wants to merge 1 commit into from
Closed

First take on cursor position #32

wants to merge 1 commit into from

Conversation

thomasballinger
Copy link

This is an in-progress PR so there's something to talk about while I make progress. Things I can think of to worry about:

  • How should this be tested?
  • Anything different to do for signals received during this blocking call?
  • Reporting cursors current location erikrose/blessings#65 says Sun needs to be treated differently, and that this won't work in Windows. What's the right way to detect these situations?

I'll keep working on these but would appreciate thoughts.

@@ -280,6 +281,27 @@ def width(self):
"""
return self._height_and_width().ws_col

@property
def cursor_position(self):
Copy link
Owner

Choose a reason for hiding this comment

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

As much of a fan of @Property as I am, I want to use a function, get_cursor_position(), because we should indicate this is an expensive/blocking call, where-as a @property-backed attribute somewhat implies otherwise.

Copy link
Owner

Choose a reason for hiding this comment

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

Please provide argument timeout=5 and honor it.

Please return the (invalid) value of (0,0) if:

  • self.keyboard_fd is None
  • HAS_TTY is False
  • or a response is not received within timeout

and document as such.

This is so that programs that use it (like the proposed bin/resize.py) may work even if stdin is not a tty.

@jquast
Copy link
Owner

jquast commented Dec 3, 2014

how should this be tested?

For testing, lets port resize.c from x11.org's xterm in python in as bin/resize.py and bin/sunsize.py!

http://cvsweb.openbsd.org/cgi-bin/cvsweb/xenocara/app/xterm/resize.c?rev=1.10&content-type=text/x-cvsweb-markup

I know it looks scary but ignore all the cruft, its simple:

  • echo(term.move(998,998))
  • width, height = term.get_cursor_position()
  • echo('COLUMNS={0};\nLINES={1};\nexport COLUMNS LINES;\n'.format(width, height))

This can then be manually tested, that it reports the same COLUMNS and LINES that your terminal emulator already has.

Secondly, this will have to be "mocked", but not in the mock module sense, just in that the build agents are not terminal emulators and we will have to mock one. You see many tests around keyboard input that spawns a pty and reads its output and sends input, you'll do the same, look for the "report cursor position" sequence, send one manually into the child process, which then asserts the child's (height, width) returned is what was sent.

Anything different to do for signals received during this blocking call?

I don't think so, hardcoding _intr_continue=True is fine until somebody can report a use-case where they need otherwise. Can you think of an edge case?

erikrose#65 says Sun needs to be treated differently

Don't worry about it, trust the values of term.u7 (request) and term.u6 (response)

and that this won't work in Windows.

Nope, we'll cross that moat when we get to it -- don't worry about it.

first = 0
keystrokes = []
while True:
keystrokes.append(self.inkey(_intr_continue=True))
Copy link
Owner

Choose a reason for hiding this comment

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

please conditionally check return value of keystrokes it not None, as you will be adding a timeout parameter. Make sure to mark the start-time and compute the remaining time for each call to inkey(), You will have to use some kind of '_timeleft' function to compute remaining time like inkey() does.

@jquast
Copy link
Owner

jquast commented Dec 4, 2014

Doing some research

  • forget kind argument and sun support.
  • use term.u6 and term.u7:
#   u7  cursor position request (equiv. to VT100/ANSI/ECMA-48 DSR 6)
#   u6  cursor position report (equiv. to ANSI/ECMA-48 CPR)
#
# The cursor position request (<u7>) string should elicit a cursor position
# report.  A typical value (for VT100 terminals) is \E[6n.
#
# The cursor position report (<u6>) string must contain two scanf(3)-style 
# %d format elements.  The first of these must correspond to the Y coordinate
# and the second to the %d.  If the string contains the sequence %i, it is
# taken as an instruction to decrement each value after reading it (this is
# the inverse sense from the cup string).  The typical CPR value is
# \E[%i%d;%dR (on VT100/ANSI/ECMA-48-compatible terminals).

@thomasballinger
Copy link
Author

This is terrific, thanks! Unfortunately I probably won't be able to work on this for about a week, but will get back to it then.

@jquast
Copy link
Owner

jquast commented Feb 28, 2015

Well there is plenty of time, now. I'd really like to dive into and help with your curtsies project (i've been watching and seeing your issues/pr's... i see what you're doing, but no comment) I'm currently focused on getting blessed merged back into blessings.

The blessed project will become very little code: only "imports blessings, monkey patch the previous behavior/methods for compatibility, recommend blessings to be used instead".

Erik transferred blessed code into his project as a branch we're working through some weeks ago, so in effect, "blessed" is in feature freeze. I proposed we get this specific feature in before the transfer, but erik declined it. This feature will have to be a part of some later release of blessings.

I will transfer this and other tickets after the merge.

As you're looking at using blessed for keyboard, you should know that I fought a tough battle over some of the api's and won some and lost some. Erik declined the inkey, cbreak and raw methods, preferring instead to rename them to keystroke, and keystroke_input(raw=False), there may be even more in the keyboard.py module, for a time he renamed the Keystroke class to Key but I think I got that reversed. There is some danger that it won't be directly compatible with the underlying code.

(It's too bad, really I asked for his feedback almost 2 years ago on these methods and their names before blessed was born, and he didn't have any opinion to share then! I have over 267 uses of inkey currently across projects, and that's only me!)

@jquast
Copy link
Owner

jquast commented Feb 28, 2015

Also, Good luck with your pycon talk!

@thomasballinger
Copy link
Author

Thanks for the updates, and great work on doing this merge with Eric. Thanks for the great feedback on the PR a while ago, sorry I didn't make any progress on it after you gave great specific feedback.

self._keyboard_buf.extendleft(
u''.join(keystrokes)[:m.start()])
return tuple(int(x) for x in m.groups())

Copy link
Owner

Choose a reason for hiding this comment

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

man... lot of mojo here without any test cases !

@jquast jquast removed the M label Oct 10, 2015
keystrokes.append(self.inkey(_intr_continue=True))
m = re.search('\x1b[[]([0-9]+);([0-9]+)R', u''.join(keystrokes))
if m:
self._keyboard_buf.extendleft(
Copy link
Owner

Choose a reason for hiding this comment

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

this may warrant the need for a _ungetch(ucs) method, where ucs is characters to be pushed back into the keyboard buffer.

@jquast jquast self-assigned this Oct 11, 2015
@jquast
Copy link
Owner

jquast commented Oct 11, 2015

resuming

@jquast
Copy link
Owner

jquast commented Oct 11, 2015

This PR superseded by #72

@jquast jquast closed this Oct 11, 2015
jquast added a commit that referenced this pull request Oct 13, 2015
This PR succeeds #32 by @thomasballinger.

major
--------
- **new method**, ``.get_location(timeout=None)``.
- **new method**, ``.ungetch()`` which pushes data back for next
  ``inkey()``.
- **new example** program, ``bin/resize.py``, which required such
  condition.
- **new example** program, ``bin/detect-multibyte.py`` which uses this
  method.
- enhancement: now possible to use stdin when stream=stderr

minor
--------
- method-local function, ``time_left()``, moved to ``keyboard.py`` as
  ``_time_left()``.
- new function in keyboard.py, ``_read_until``, something like a
  mini-pexpect.
- new private state value ``_line_buffered``, used to determine whether
  the terminal has already entered cbreak or raw mode. This may become public later if we also use termios routines, instead as a read-only \@Property.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants