Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

BUG: qtconsole -- non-standard handling of \a and \b. [Fixes #1561] #1569

Merged
merged 2 commits into from

4 participants

@punchagan

Fixes #1561 and adds many tests

@jdmarch
Collaborator

This PR also fixes an invisible bug in #1089 -- the actions list was not cleared after processing bell and carriage return, which caused these to be processed twice. This was inconsequential (beep+beep sounds like beep, return+return looks like return), but not so for backspace. The former tests did not catch this because they only tested one element of the actions list; this PR also corrects and expands those tests.

@jdmarch
Collaborator

This PR makes handling of \a and \b compatible [EDIT: almost compatible, see below] with IPython terminal. However it could cause problems for any users who rely on the non-standard behavior implemented in #1089. Any concerns, @mdboom or @epatters ?

@jdmarch
Collaborator

Actually in an odd corner case, backspace handling in this PR is still not compatible with terminal.
Terminal:
In [7]: print 'xyz\b\b='
x=z

This PR:
In [1]: print 'xyz\b\b='
x=

Apparently the terminal metaphor is that the backspace character moves the print position backwards but does not delete any characters. I'm not sure that this is optimal but it probably doesn't matter and may be set in stone by now, so I would think that qtconsole should simply follow suite.

EDIT: replacing spaces with "=" for clarity.

...frontend/qt/console/tests/test_ansi_code_processor.py
((49 lines not shown))
+ def test_backspace(self):
+ """ Are backspace characters processed correctly?
+ """
+ string = 'foo\bbar' # backspace
+ splits = []
+ actions = []
+ for split in self.processor.split_string(string):
+ splits.append(split)
+ actions.append([action.action for action in self.processor.actions])
+ self.assertEquals(splits, ['foo', None, 'bar'])
+ self.assertEquals(actions, [[], ['backspace'], []])
+
+ def test_combined(self):
+ """ Are return and backspace characters processed correctly in combination?
+ """
+ string = 'abc\rdef\b' # CR and backspace
@jdmarch Collaborator
jdmarch added a note

A comment should explain that for compatibility with IPython terminal, a BS at EOL is effectively ignored because it is treated as a change in print position rather than a backwards character deletion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mdboom

My stuff doesn't appear to rely on the broken functionality (it works just as well with this PR), so +1.

@punchagan

@jdmarch Thanks for reviewing and testing the patch, and the elaborate comments! I force pushed a change, to handle the corner case you pointed out.

@jdmarch
Collaborator

This looks good now, thanks! Tests are much stronger. The new module to test actual output can provide a basis for other such tests in the future.

This is admittedly very 20th Century but still useful for some beginner training in escape sequences, and terminal progress bars. OK to merge?

IPython/frontend/qt/console/ansi_code_processor.py
((19 lines not shown))
groups = filter(lambda x: x is not None, match.groups())
- if groups[0] == '\r':
+ if groups[0] == '\a':
@fperez Owner
fperez added a note

Minor nitpick: since groups[0] is now used multiple times for this dispatch, it might be a good idea to create a local g0 = groups[0] and do the whole if/elif/elif... dispatch on g0 instead, to avoid the cost of multiple lookups on the groups object...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
IPython/frontend/qt/console/tests/test_console_widget.py
((26 lines not shown))
+ def test_special_characters(self):
+ """ Are special characters displayed correctly?
+ """
+ w = ConsoleWidget()
+ cursor = w._get_prompt_cursor()
+
+ test_inputs = ['xyz\b\b=\n', 'foo\b\nbar\n', 'foo\b\nbar\r\n', 'abc\rxyz\b\b=']
+ expected_outputs = [u'x=z\u2029', u'foo\u2029bar\u2029', u'foo\u2029bar\u2029', 'x=z']
+ for i, text in enumerate(test_inputs):
+ w._insert_plain_text(cursor, text)
+ cursor.select(cursor.Document)
+ selection = cursor.selectedText()
+ self.assertEquals(expected_outputs[i], selection)
+ # clear all the text
+ cursor.insertText('')
+
@fperez Owner
fperez added a note

We don't use the 'main' section calling nose explicitly in IPython. Instead, there's an 'iptest' script that is in charge of running the whole test suite. I prefer that devs don't get into the habit of having a separate entry point for their tests, because it increases the chance that they don't notice a problem with their tests when run in the 'normal' fashion. So the block below should be removed, and these tests can be run via

iptest -vv IPython.frontend.qt.console.test_console_widget
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@punchagan

@fperez -- Thanks for the comments. Fixed the issues and pushed changes.

@fperez
Owner

Great, these changes look good now. I concur with @jdmarch that it's best to have \b match the pure terminal behavior: if nothing else, we want the qtconsole to feel as much as a classic console as possible, just better (i.e. with images, highlighting, multiline input, etc).

I'll go ahead and merge this one so it's out of the way; on-list we discussed @jdmarch will take care of shepherding these Qt-related PRs, which is fantastic. We'll have a quicker-moving pipeline for all of them.

@fperez fperez merged commit ed4fe90 into ipython:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
45 IPython/frontend/qt/console/ansi_code_processor.py
@@ -29,16 +29,22 @@
# An action for the carriage return character
CarriageReturnAction = namedtuple('CarriageReturnAction', ['action'])
+# An action for the \n character
+NewLineAction = namedtuple('NewLineAction', ['action'])
+
# An action for the beep character
BeepAction = namedtuple('BeepAction', ['action'])
+# An action for backspace
+BackSpaceAction = namedtuple('BackSpaceAction', ['action'])
+
# Regular expressions.
CSI_COMMANDS = 'ABCDEFGHJKSTfmnsu'
CSI_SUBPATTERN = '\[(.*?)([%s])' % CSI_COMMANDS
OSC_SUBPATTERN = '\](.*?)[\x07\x1b]'
ANSI_PATTERN = ('\x01?\x1b(%s|%s)\x02?' % \
(CSI_SUBPATTERN, OSC_SUBPATTERN))
-ANSI_OR_SPECIAL_PATTERN = re.compile('(\b|\r(?!\n))|(?:%s)' % ANSI_PATTERN)
+ANSI_OR_SPECIAL_PATTERN = re.compile('(\a|\b|\r(?!\n)|\r?\n)|(?:%s)' % ANSI_PATTERN)
SPECIAL_PATTERN = re.compile('([\f])')
#-----------------------------------------------------------------------------
@@ -83,24 +89,41 @@ def split_string(self, string):
self.actions = []
start = 0
+ # strings ending with \r are assumed to be ending in \r\n since
+ # \n is appended to output strings automatically. Accounting
+ # for that, here.
+ last_char = '\n' if len(string) > 0 and string[-1] == '\n' else None
+ string = string[:-1] if last_char is not None else string
+
for match in ANSI_OR_SPECIAL_PATTERN.finditer(string):
raw = string[start:match.start()]
substring = SPECIAL_PATTERN.sub(self._replace_special, raw)
if substring or self.actions:
yield substring
+ self.actions = []
start = match.end()
- self.actions = []
groups = filter(lambda x: x is not None, match.groups())
- if groups[0] == '\r':
- self.actions.append(CarriageReturnAction('carriage-return'))
- yield ''
- elif groups[0] == '\b':
+ g0 = groups[0]
+ if g0 == '\a':
self.actions.append(BeepAction('beep'))
- yield ''
+ yield None
+ self.actions = []
+ elif g0 == '\r':
+ self.actions.append(CarriageReturnAction('carriage-return'))
+ yield None
+ self.actions = []
+ elif g0 == '\b':
+ self.actions.append(BackSpaceAction('backspace'))
+ yield None
+ self.actions = []
+ elif g0 == '\n' or g0 == '\r\n':
+ self.actions.append(NewLineAction('newline'))
+ yield g0
+ self.actions = []
else:
params = [ param for param in groups[1].split(';') if param ]
- if groups[0].startswith('['):
+ if g0.startswith('['):
# Case 1: CSI code.
try:
params = map(int, params)
@@ -110,7 +133,7 @@ def split_string(self, string):
else:
self.set_csi_code(groups[2], params)
- elif groups[0].startswith(']'):
+ elif g0.startswith(']'):
# Case 2: OSC code.
self.set_osc_code(params)
@@ -119,6 +142,10 @@ def split_string(self, string):
if substring or self.actions:
yield substring
+ if last_char is not None:
+ self.actions.append(NewLineAction('newline'))
+ yield last_char
+
def set_csi_code(self, command, params=[]):
""" Set attributes based on CSI (Control Sequence Introducer) code.
View
25 IPython/frontend/qt/console/console_widget.py
@@ -1553,8 +1553,31 @@ def _insert_plain_text(self, cursor, text):
elif act.action == 'beep':
QtGui.qApp.beep()
+ elif act.action == 'backspace':
+ if not cursor.atBlockStart():
+ cursor.movePosition(
+ cursor.PreviousCharacter, cursor.KeepAnchor)
+
+ elif act.action == 'newline':
+ cursor.movePosition(cursor.EndOfLine)
+
format = self._ansi_processor.get_format()
- cursor.insertText(substring, format)
+
+ selection = cursor.selectedText()
+ if len(selection) == 0:
+ cursor.insertText(substring, format)
+ elif substring is not None:
+ # BS and CR are treated as a change in print
+ # position, rather than a backwards character
+ # deletion for output equivalence with (I)Python
+ # terminal.
+ if len(substring) >= len(selection):
+ cursor.insertText(substring, format)
+ else:
+ old_text = selection[len(substring):]
+ cursor.insertText(substring + old_text, format)
+ cursor.movePosition(cursor.PreviousCharacter,
+ cursor.KeepAnchor, len(old_text))
else:
cursor.insertText(text)
cursor.endEditBlock()
View
65 IPython/frontend/qt/console/tests/test_ansi_code_processor.py
@@ -106,28 +106,65 @@ def test_carriage_return(self):
""" Are carriage return characters processed correctly?
"""
string = 'foo\rbar' # carriage return
- self.assertEquals(list(self.processor.split_string(string)), ['foo', '', 'bar'])
- self.assertEquals(len(self.processor.actions), 1)
- action = self.processor.actions[0]
- self.assertEquals(action.action, 'carriage-return')
+ splits = []
+ actions = []
+ for split in self.processor.split_string(string):
+ splits.append(split)
+ actions.append([action.action for action in self.processor.actions])
+ self.assertEquals(splits, ['foo', None, 'bar'])
+ self.assertEquals(actions, [[], ['carriage-return'], []])
def test_carriage_return_newline(self):
"""transform CRLF to LF"""
- string = 'foo\rbar\r\ncat\r\n' # carriage return and newline
+ string = 'foo\rbar\r\ncat\r\n\n' # carriage return and newline
# only one CR action should occur, and '\r\n' should transform to '\n'
- self.assertEquals(list(self.processor.split_string(string)), ['foo', '', 'bar\r\ncat\r\n'])
- self.assertEquals(len(self.processor.actions), 1)
- action = self.processor.actions[0]
- self.assertEquals(action.action, 'carriage-return')
+ splits = []
+ actions = []
+ for split in self.processor.split_string(string):
+ splits.append(split)
+ actions.append([action.action for action in self.processor.actions])
+ self.assertEquals(splits, ['foo', None, 'bar', '\r\n', 'cat', '\r\n', '\n'])
+ self.assertEquals(actions, [[], ['carriage-return'], [], ['newline'], [], ['newline'], ['newline']])
def test_beep(self):
""" Are beep characters processed correctly?
"""
- string = 'foo\bbar' # form feed
- self.assertEquals(list(self.processor.split_string(string)), ['foo', '', 'bar'])
- self.assertEquals(len(self.processor.actions), 1)
- action = self.processor.actions[0]
- self.assertEquals(action.action, 'beep')
+ string = 'foo\abar' # bell
+ splits = []
+ actions = []
+ for split in self.processor.split_string(string):
+ splits.append(split)
+ actions.append([action.action for action in self.processor.actions])
+ self.assertEquals(splits, ['foo', None, 'bar'])
+ self.assertEquals(actions, [[], ['beep'], []])
+
+ def test_backspace(self):
+ """ Are backspace characters processed correctly?
+ """
+ string = 'foo\bbar' # backspace
+ splits = []
+ actions = []
+ for split in self.processor.split_string(string):
+ splits.append(split)
+ actions.append([action.action for action in self.processor.actions])
+ self.assertEquals(splits, ['foo', None, 'bar'])
+ self.assertEquals(actions, [[], ['backspace'], []])
+
+ def test_combined(self):
+ """ Are CR and BS characters processed correctly in combination?
+
+ BS is treated as a change in print position, rather than a
+ backwards character deletion. Therefore a BS at EOL is
+ effectively ignored.
+ """
+ string = 'abc\rdef\b' # CR and backspace
+ splits = []
+ actions = []
+ for split in self.processor.split_string(string):
+ splits.append(split)
+ actions.append([action.action for action in self.processor.actions])
+ self.assertEquals(splits, ['abc', None, 'def', None])
+ self.assertEquals(actions, [[], ['carriage-return'], [], ['backspace']])
if __name__ == '__main__':
View
40 IPython/frontend/qt/console/tests/test_console_widget.py
@@ -0,0 +1,40 @@
+# Standard library imports
+import unittest
+
+# System library imports
+from IPython.external.qt import QtGui
+
+# Local imports
+from IPython.frontend.qt.console.console_widget import ConsoleWidget
+
+
+class TestConsoleWidget(unittest.TestCase):
+
+ @classmethod
+ def setUpClass(cls):
+ """ Create the application for the test case.
+ """
+ cls._app = QtGui.QApplication([])
+ cls._app.setQuitOnLastWindowClosed(False)
+
+ @classmethod
+ def tearDownClass(cls):
+ """ Exit the application.
+ """
+ QtGui.QApplication.quit()
+
+ def test_special_characters(self):
+ """ Are special characters displayed correctly?
+ """
+ w = ConsoleWidget()
+ cursor = w._get_prompt_cursor()
+
+ test_inputs = ['xyz\b\b=\n', 'foo\b\nbar\n', 'foo\b\nbar\r\n', 'abc\rxyz\b\b=']
+ expected_outputs = [u'x=z\u2029', u'foo\u2029bar\u2029', u'foo\u2029bar\u2029', 'x=z']
+ for i, text in enumerate(test_inputs):
+ w._insert_plain_text(cursor, text)
+ cursor.select(cursor.Document)
+ selection = cursor.selectedText()
+ self.assertEquals(expected_outputs[i], selection)
+ # clear all the text
+ cursor.insertText('')
Something went wrong with that request. Please try again.