From ac2ec3457df6b699916864f26a89a1cc6fcadd9d Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 5 Apr 2016 11:33:33 -0700 Subject: [PATCH 1/4] NcursesDisplay.menu: treat ESC as cancel Currently it will fire a weird traceback like: File "/home/ubuntu/letsencrypt/letsencrypt/plugins/selection.py", line 113, in choose_plugin code, index = disp.menu(question, opts, help_label="More Info") File "/home/ubuntu/letsencrypt/letsencrypt/display/util.py", line 129, in menu return code, int(index) - 1 ValueError: invalid literal for int() with base 10: '' --- letsencrypt/display/util.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt/display/util.py b/letsencrypt/display/util.py index 20c6be156..40dd00f8b 100644 --- a/letsencrypt/display/util.py +++ b/letsencrypt/display/util.py @@ -123,7 +123,7 @@ class NcursesDisplay(object): # pylint: disable=star-args code, index = self.dialog.menu(message, **menu_options) - if code == CANCEL: + if code == CANCEL or index == "": return code, -1 return code, int(index) - 1 From 8a28cb7352eb5d534e79cdd213ede8fd8283dedb Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 22 Jun 2016 15:50:21 -0700 Subject: [PATCH 2/4] Implement Brad's more systematic solution for this --- certbot/display/util.py | 33 ++++++++++++++++++++++-------- certbot/tests/display/util_test.py | 1 + 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/certbot/display/util.py b/certbot/display/util.py index 683dbc037..c998f78f3 100644 --- a/certbot/display/util.py +++ b/certbot/display/util.py @@ -29,7 +29,6 @@ CANCEL = "cancel" HELP = "help" """Display exit code when for when the user requests more help.""" - def _wrap_lines(msg): """Format lines nicely to 80 chars. @@ -51,6 +50,21 @@ def _wrap_lines(msg): return os.linesep.join(fixed_l) + +def _clean(dialog_result): + """Work around inconsistent return codes from python-dialog. + + :param tuple dialog_result: (code, result) + :returns: the argument but with unknown codes set to -1 (Error) + :rtype: tuple + """ + code, result = dialog_result + if code in (OK, HELP): + return dialog_result + else: + return (CANCEL, result) + + @zope.interface.implementer(interfaces.IDisplay) class NcursesDisplay(object): """Ncurses-based display.""" @@ -92,7 +106,7 @@ class NcursesDisplay(object): :param dict unused_kwargs: absorbs default / cli_args :returns: tuple of the form (`code`, `index`) where - `code` - int display exit code + `code` - display exit code `int` - index of the selected item :rtype: tuple @@ -111,7 +125,7 @@ class NcursesDisplay(object): # Can accept either tuples or just the actual choices if choices and isinstance(choices[0], tuple): # pylint: disable=star-args - code, selection = self.dialog.menu(message, **menu_options) + code, selection = _clean(self.dialog.menu(message, **menu_options)) # Return the selection index for i, choice in enumerate(choices): @@ -126,7 +140,7 @@ class NcursesDisplay(object): (str(i), choice) for i, choice in enumerate(choices, 1) ] # pylint: disable=star-args - code, index = self.dialog.menu(message, **menu_options) + code, index = _clean(self.dialog.menu(message, **menu_options)) if code == CANCEL or index == "": return code, -1 @@ -140,7 +154,7 @@ class NcursesDisplay(object): :param dict _kwargs: absorbs default / cli_args :returns: tuple of the form (`code`, `string`) where - `code` - int display exit code + `code` - display exit code `string` - input entered by the user """ @@ -148,7 +162,7 @@ class NcursesDisplay(object): # each section takes at least one line, plus extras if it's longer than self.width wordlines = [1 + (len(section) / self.width) for section in sections] height = 6 + sum(wordlines) + len(sections) - return self.dialog.inputbox(message, width=self.width, height=height) + return _clean(self.dialog.inputbox(message, width=self.width, height=height)) def yesno(self, message, yes_label="Yes", no_label="No", **unused_kwargs): """Display a Yes/No dialog box. @@ -164,6 +178,7 @@ class NcursesDisplay(object): :rtype: bool """ + assert OK == self.dialog.DIALOG_OK, "What kind of absurdity is this?" return self.dialog.DIALOG_OK == self.dialog.yesno( message, self.height, self.width, yes_label=yes_label, no_label=no_label) @@ -179,7 +194,7 @@ class NcursesDisplay(object): :returns: tuple of the form (`code`, `list_tags`) where - `code` - int display exit code + `code` - display exit code `list_tags` - list of str tags selected by the user """ @@ -193,7 +208,7 @@ class NcursesDisplay(object): :param str message: prompt to give the user :returns: tuple of the form (`code`, `string`) where - `code` - int display exit code + `code` - display exit code `string` - input entered by the user """ @@ -355,7 +370,7 @@ class FileDisplay(object): :param str message: prompt to give the user :returns: tuple of the form (`code`, `string`) where - `code` - int display exit code + `code` - display exit code `string` - input entered by the user """ diff --git a/certbot/tests/display/util_test.py b/certbot/tests/display/util_test.py index 4a38803d1..94338118d 100644 --- a/certbot/tests/display/util_test.py +++ b/certbot/tests/display/util_test.py @@ -96,6 +96,7 @@ class NcursesDisplayTest(unittest.TestCase): @mock.patch("certbot.display.util." "dialog.Dialog.inputbox") def test_input(self, mock_input): + mock_input.return_value = (mock.MagicMock(), mock.MagicMock()) self.displayer.input("message") self.assertEqual(mock_input.call_count, 1) From 23f0ccbc8ee7f03b094c96fd2f776919b4f72d7c Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Sat, 25 Jun 2016 12:22:45 -0700 Subject: [PATCH 3/4] Address review issues --- certbot/display/util.py | 21 ++++++++++++++++----- certbot/tests/display/util_test.py | 2 ++ 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/certbot/display/util.py b/certbot/display/util.py index c998f78f3..4e7d45741 100644 --- a/certbot/display/util.py +++ b/certbot/display/util.py @@ -1,4 +1,5 @@ """Certbot display.""" +import logging import os import textwrap @@ -9,6 +10,9 @@ from certbot import interfaces from certbot import errors from certbot.display import completer + +logger = logging.getLogger(__name__) + WIDTH = 72 HEIGHT = 20 @@ -29,6 +33,10 @@ CANCEL = "cancel" HELP = "help" """Display exit code when for when the user requests more help.""" +ESC = "esc" +"""Display exit code when the user hits Escape""" + + def _wrap_lines(msg): """Format lines nicely to 80 chars. @@ -52,7 +60,7 @@ def _wrap_lines(msg): def _clean(dialog_result): - """Work around inconsistent return codes from python-dialog. + """Treat sundy python-dialog return codes as CANCEL :param tuple dialog_result: (code, result) :returns: the argument but with unknown codes set to -1 (Error) @@ -61,7 +69,10 @@ def _clean(dialog_result): code, result = dialog_result if code in (OK, HELP): return dialog_result + elif code in (CANCEL, ESC): + return (CANCEL, result) else: + logger.info("Surprising dialog return code %s", code) return (CANCEL, result) @@ -199,8 +210,8 @@ class NcursesDisplay(object): """ choices = [(tag, "", default_status) for tag in tags] - return self.dialog.checklist( - message, width=self.width, height=self.height, choices=choices) + return _clean(self.dialog.checklist( + message, width=self.width, height=self.height, choices=choices)) def directory_select(self, message, **unused_kwargs): """Display a directory selection screen. @@ -213,9 +224,9 @@ class NcursesDisplay(object): """ root_directory = os.path.abspath(os.sep) - return self.dialog.dselect( + return _clean(self.dialog.dselect( filepath=root_directory, width=self.width, - height=self.height, help_button=True, title=message) + height=self.height, help_button=True, title=message)) @zope.interface.implementer(interfaces.IDisplay) diff --git a/certbot/tests/display/util_test.py b/certbot/tests/display/util_test.py index 94338118d..a6ced90ab 100644 --- a/certbot/tests/display/util_test.py +++ b/certbot/tests/display/util_test.py @@ -113,6 +113,7 @@ class NcursesDisplayTest(unittest.TestCase): @mock.patch("certbot.display.util." "dialog.Dialog.checklist") def test_checklist(self, mock_checklist): + mock_checklist.return_value = (mock.MagicMock(), mock.MagicMock()) self.displayer.checklist("message", TAGS) choices = [ @@ -126,6 +127,7 @@ class NcursesDisplayTest(unittest.TestCase): @mock.patch("certbot.display.util.dialog.Dialog.dselect") def test_directory_select(self, mock_dselect): + mock_dselect.return_value = (mock.MagicMock(), mock.MagicMock()) self.displayer.directory_select("message") self.assertEqual(mock_dselect.call_count, 1) From 4b84538c8c9c0bb852eb2874cf38cc144dbd3f0d Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 6 Jul 2016 12:57:16 -0700 Subject: [PATCH 4/4] Address review comments --- certbot/display/util.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/certbot/display/util.py b/certbot/display/util.py index 4e7d45741..39486b2bd 100644 --- a/certbot/display/util.py +++ b/certbot/display/util.py @@ -72,7 +72,7 @@ def _clean(dialog_result): elif code in (CANCEL, ESC): return (CANCEL, result) else: - logger.info("Surprising dialog return code %s", code) + logger.debug("Surprising dialog return code %s", code) return (CANCEL, result) @@ -83,6 +83,7 @@ class NcursesDisplay(object): def __init__(self, width=WIDTH, height=HEIGHT): super(NcursesDisplay, self).__init__() self.dialog = dialog.Dialog() + assert OK == self.dialog.DIALOG_OK, "What kind of absurdity is this?" self.width = width self.height = height @@ -189,7 +190,6 @@ class NcursesDisplay(object): :rtype: bool """ - assert OK == self.dialog.DIALOG_OK, "What kind of absurdity is this?" return self.dialog.DIALOG_OK == self.dialog.yesno( message, self.height, self.width, yes_label=yes_label, no_label=no_label)