Merge lp:~frankban/juju-quickstart/improve-input-handling into lp:juju-quickstart

[Francesco Banconi]

Improve views input handling.

Add a new set_return_value_on_exit API that
views can call to set the value encapsulated
in the AppExit exception raised when the user
quits the application with ^X.

Tests: `make check`

QA: start the demo app
(`make` and `./cli-app-demo.py).
Try exiting with ^X: you should see the
"no environment selected" message.
Restart and exit selecting an environment:
you should see a message indicating that
environment has been selected.
Restart and change the default environment,
then exit: you should see the new env_db
printed and the new default environment.

https://codereview.appspot.com/43600047/

[Francesco Banconi (frankban)]

Reviewers: mp+199640_code.launchpad.net,

Message:
Please take a look.

Description:
Improve views input handling.

Add a new set_return_value_on_exit API that
views can call to set the value encapsulated
in the AppExit exception raised when the user
quits the application with ^X.

Tests: `make check`

QA: start the demo app
(`make` and `./cli-app-demo.py).
Try exiting with ^X: you should see the
"no environment selected" message.
Restart and exit selecting an environment:
you should see a message indicating that
environment has been selected.
Restart and change the default environment,
then exit: you should see the new env_db
printed and the new default environment.

https://code.launchpad.net/~frankban/juju-quickstart/improve-input-handling/+merge/199640

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/43600047/

Affected files (+110, -41 lines):
   A [revision details]
   M cli-app-demo.py
   M quickstart/cli/base.py
   M quickstart/cli/views.py
   A quickstart/tests/cli/helpers.py
   M quickstart/tests/cli/test_base.py
   M quickstart/tests/cli/test_views.py

[Madison Scott-Clary (makyo)]

LGTM, thanks!

https://codereview.appspot.com/43600047/

[lp:~frankban/juju-quickstart/improve-input-handling]

44. By Francesco Banconi on 2013-12-19

Add missing import unicode_literals.

[Francesco Banconi (frankban)]

*** Submitted:

Improve views input handling.

Add a new set_return_value_on_exit API that
views can call to set the value encapsulated
in the AppExit exception raised when the user
quits the application with ^X.

Tests: `make check`

QA: start the demo app
(`make` and `./cli-app-demo.py).
Try exiting with ^X: you should see the
"no environment selected" message.
Restart and exit selecting an environment:
you should see a message indicating that
environment has been selected.
Restart and change the default environment,
then exit: you should see the new env_db
printed and the new default environment.

R=matthew.scott
CC=
https://codereview.appspot.com/43600047

https://codereview.appspot.com/43600047/

[Francesco Banconi (frankban)]

Thank you!

https://codereview.appspot.com/43600047/

=== modified file 'cli-app-demo.py'
--- cli-app-demo.py	2013-12-18 15:41:31 +0000
+++ cli-app-demo.py	2013-12-19 17:41:55 +0000
@@ -27,6 +27,8 @@
     unicode_literals,
 )
 
+import pprint
+
 from quickstart.cli import views
 from quickstart.models import envs
 from quickstart.tests import helpers
@@ -43,6 +45,7 @@
         views.env_index, env_type_db, env_db, save_callable)
     if new_env_db != env_db:
         print('saved a new env db')
+        pprint.pprint(new_env_db)
     print('default: {}'.format(new_env_db.get('default')))
     if env_data is None:
         print('no environment selected')

=== modified file 'quickstart/cli/base.py'
--- quickstart/cli/base.py	2013-12-19 09:12:37 +0000
+++ quickstart/cli/base.py	2013-12-19 17:41:55 +0000
@@ -40,6 +40,7 @@
         'set_contents', 'get_contents',
         'set_status', 'get_status',
         'set_message', 'get_message',
+        'set_return_value_on_exit',
     ],
 )
 
@@ -102,7 +103,12 @@
         - set_message(text): set/change a notification message, which is
           displayed in the footer for a couple of seconds before disappearing;
         - get_message(): return the message currently displayed in the
-          notifications area.
+          notifications area;
+
+        - set_return_value_on_exit(value): set the value to be encapsulated
+          in the AppExit exception raised when the user quits the application
+          with the exit shortcut. See the quickstart.cli.views module docstring
+          for more information about this functionality.
     """
     # Set up the application header.
     title = urwid.Text('\npreparing...')
@@ -143,10 +149,18 @@
         valign='middle', height=('relative', 90),
         min_width=78, min_height=20)
     # Instantiate the Urwid main loop.
-    loop = _MainLoop(top_widget, palette=ui.PALETTE)
+    loop = _MainLoop(
+        top_widget, palette=ui.PALETTE,
+        unhandled_input=ui.exit_and_return(None))
     # Add a timeout to the notification message.
     timeout_message = ui.TimeoutText(
         message, 3, loop.set_alarm_in, loop.remove_alarm)
+
+    # Allow views to set the value returned when the user quits the session.
+    def set_return_value_on_exit(return_value):
+        unhandled_input = ui.exit_and_return(return_value)
+        loop.set_unhandled_input(unhandled_input)
+
     # Create the App named tuple. If, in the future, we have a view that
     # requires additional capabilities or API access, this is the place to add
     # those.
@@ -159,5 +173,6 @@
         get_status=lambda: status.text,
         set_message=lambda msg: timeout_message.set_text(('message', msg)),
         get_message=lambda: timeout_message.text,
+        set_return_value_on_exit=set_return_value_on_exit,
     )
     return loop, app

=== modified file 'quickstart/cli/views.py'
--- quickstart/cli/views.py	2013-12-19 09:30:08 +0000
+++ quickstart/cli/views.py	2013-12-19 17:41:55 +0000
@@ -38,7 +38,7 @@
 
     def myview(app, title):
         app.set_title(title)
-        return 42
+        app.set_return_value_on_exit(42)
 
 The view above, requiring a title argument, can be started this way:
 
@@ -52,9 +52,12 @@
     2) a view decides it is time to quit (e.g. reacting to an event/input).
 
 In both cases, the show function returns something to the caller:
-    1) when the user explicitly requests to quit, the value returned by the
-       view itself (the callable passed to show()) is returned. For instance,
-       the show(myview...) call above would return 42;
+    1) when the user explicitly requests to quit, None is returned by default.
+       However, the view can change this default value by calling
+       app.set_return_value_on_exit(some value). For instance, the
+       show(myview...) call above would return 42. It is safe to call the
+       set_return_value_on_exit API multiple times in order to overwrite the
+       value returned on user exit;
     2) to force the end of the interactive session, a view can raise a
        quickstart.cli.ui.AppExit exception, passing a return value: if the
        application is exited this way, then show() returns the value
@@ -71,18 +74,19 @@
         def exit():
             raise ui.AppExit(True)
 
+        app.set_return_value_on_exit(False)
         app.set_title('behold the button below')
         button = ui.MenuButton('press to exit', ui.thunk(exit))
         widgets = urwid.ListBox(urwid.SimpleFocusListWalker([button]))
         app.set_contents(widgets)
-        return False
 
     pressed = views.show(button_view)
 
 In this example the button_view function configures the App to show a button.
-Clicking that button an AppExit(True) is raised. The view itself instead just
-returns False. This means that "pressed" will be True if the user exited using
-the button, or False if the user exited using the global shortcut.
+Clicking that button an AppExit(True) is raised. The return value on exit
+instead is set by the view itself to False. This means that "pressed" will be
+True if the user exited using the button, or False if the user exited using the
+global shortcut.
 
 As a final note, it is absolutely safe for a view to call, directly or
 indirectly, other views, as long as all the arguments required by the other
@@ -111,16 +115,14 @@
     The view is called passing an App named tuple and the provided *args.
 
     Block until the main loop is stopped, either by the user with the exit
-    shortcut or by the view itself with the AppExit exception. In the former
-    case, return what is returned by the view. In the AppExit case, return
-    the value encapsulated in the exception.
+    shortcut or by the view itself. In both cases, an ui.AppExit is raised, and
+    the return value is encapsulated in the exception.
     """
     loop, app = base.setup_urwid_app()
-    default_return_value = view(app, *args)
-    unhandled_input = ui.exit_and_return(default_return_value)
-    loop.set_unhandled_input(unhandled_input)
-    # Start the Urwid interactive session (main loop).
     try:
+        # Execute the view.
+        view(app, *args)
+        # Start the Urwid interactive session (main loop).
         loop.run()
     except ui.AppExit as err:
         return err.return_value
@@ -137,6 +139,10 @@
         - save_callable: a function called to save a new environment database.
     """
     env_db = copy.deepcopy(env_db)
+    # All the environment views return a tuple (new_env_db, env_data).
+    # Set the env_data to None in the case the user quits the application
+    # without selecting an environment to use.
+    app.set_return_value_on_exit((env_db, None))
     detail_view = functools.partial(
         env_detail, app, env_type_db, env_db, save_callable)
     # Alphabetically sort the existing environments.
@@ -185,7 +191,6 @@
     if status:
         app.set_status(status)
     app.set_contents(contents)
-    return env_db, None
 
 
 def env_detail(app, env_type_db, env_db, save_callable, env_data):
@@ -201,6 +206,10 @@
         - env_data: the environment data.
     """
     env_db = copy.deepcopy(env_db)
+    # All the environment views return a tuple (new_env_db, env_data).
+    # Set the env_data to None in the case the user quits the application
+    # without selecting an environment to use.
+    app.set_return_value_on_exit((env_db, None))
     index_view = functools.partial(
         env_index, app, env_type_db, env_db, save_callable)
 
@@ -250,4 +259,3 @@
     widgets.append(ui.create_controls(*controls))
     listbox = urwid.ListBox(urwid.SimpleFocusListWalker(widgets))
     app.set_contents(listbox)
-    return env_db, None

=== added file 'quickstart/tests/cli/helpers.py'
--- quickstart/tests/cli/helpers.py	1970-01-01 00:00:00 +0000
+++ quickstart/tests/cli/helpers.py	2013-12-19 17:41:55 +0000
@@ -0,0 +1,31 @@
+# This file is part of the Juju Quickstart Plugin, which lets users set up a
+# Juju environment in very few steps (https://launchpad.net/juju-quickstart).
+# Copyright (C) 2013 Canonical Ltd.
+#
+# This program is free software: you can redistribute it and/or modify it under
+# the terms of the GNU Affero General Public License version 3, as published by
+# the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful, but WITHOUT
+# ANY WARRANTY; without even the implied warranties of MERCHANTABILITY,
+# SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# Affero General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+"""Test helpers for the Juju Quickstart CLI infrastructure."""
+
+from __future__ import unicode_literals
+
+from quickstart.cli import ui
+
+
+class CliAppTestsMixin(object):
+    """Helper methods to test Quickstart CLI applications."""
+
+    def get_on_exit_return_value(self, loop):
+        """Return the value returned by the application when the user quits."""
+        with self.assertRaises(ui.AppExit) as context_manager:
+            loop.unhandled_input(ui.EXIT_KEY)
+        return context_manager.exception.return_value

=== modified file 'quickstart/tests/cli/test_base.py'
--- quickstart/tests/cli/test_base.py	2013-12-19 09:12:37 +0000
+++ quickstart/tests/cli/test_base.py	2013-12-19 17:41:55 +0000
@@ -23,6 +23,7 @@
 import urwid
 
 from quickstart.cli import base
+from quickstart.tests.cli import helpers as cli_helpers
 
 
 class TestMainLoop(unittest.TestCase):
@@ -57,7 +58,7 @@
         self.assertEqual(1, sum(times_called))
 
 
-class TestSetupUrwidApp(unittest.TestCase):
+class TestSetupUrwidApp(cli_helpers.CliAppTestsMixin, unittest.TestCase):
 
     def setUp(self):
         # Set up the base Urwid application.
@@ -164,3 +165,21 @@
         message_widget = self.get_message_widget(self.loop)
         message_widget.set_text('42')
         self.assertEqual('42', self.app.get_message())
+
+    def test_default_unhandled_input(self):
+        # The default unhandled_input function is configured so that a
+        # quickstart.cli.ui.AppExit exception is raised. The exception's
+        # return value is None by default.
+        return_value = self.get_on_exit_return_value(self.loop)
+        self.assertIsNone(return_value)
+
+    def test_set_return_value_on_exit(self):
+        # It is possible to change the value returned by the AppExit exception
+        # when the user quits the application using the exit shortcut.
+        self.app.set_return_value_on_exit(42)
+        return_value = self.get_on_exit_return_value(self.loop)
+        self.assertEqual(42, return_value)
+        # The value can be changed multiple times.
+        self.app.set_return_value_on_exit([47, None])
+        return_value = self.get_on_exit_return_value(self.loop)
+        self.assertEqual([47, None], return_value)

=== modified file 'quickstart/tests/cli/test_views.py'
--- quickstart/tests/cli/test_views.py	2013-12-19 09:30:08 +0000
+++ quickstart/tests/cli/test_views.py	2013-12-19 17:41:55 +0000
@@ -31,6 +31,7 @@
 )
 from quickstart.models import envs
 from quickstart.tests import helpers
+from quickstart.tests.cli import helpers as cli_helpers
 
 
 class TestShow(unittest.TestCase):
@@ -70,17 +71,6 @@
             return_value = views.show(view)
         self.assertEqual('bad wolf', return_value)
 
-    def test_unhandled_input(self):
-        # The unhandled_input callable is properly set up and registered to
-        # the main loop.
-        view = mock.Mock(return_value=42)
-        with self.patch_setup_urwid_app() as (mock_loop, mock_app):
-            views.show(view)
-        unhandled_input = mock_loop.set_unhandled_input.call_args[0][0]
-        with self.assertRaises(ui.AppExit) as context_manager:
-            unhandled_input(ui.EXIT_KEY)
-        self.assertEqual(42, context_manager.exception.return_value)
-
     def test_view_arguments(self):
         # The view is invoked passing the app and all the optional show
         # function arguments.
@@ -90,7 +80,7 @@
         view.assert_called_once_with(mock_app, 'arg1', 'arg2')
 
 
-class EnvViewTestsMixin(object):
+class EnvViewTestsMixin(cli_helpers.CliAppTestsMixin):
     """Shared helpers for testing environment views."""
 
     env_type_db = envs.get_env_type_db()
@@ -145,12 +135,13 @@
 
 class TestEnvIndex(EnvViewTestsMixin, unittest.TestCase):
 
-    def test_view_return_value(self):
-        # The view returns a tuple including a copy of the given env_db and
-        # None, the latter meaning no environment has been selected.
+    def test_view_default_return_value_on_exit(self):
+        # The view configures the app so that the return value on user exit is
+        # a tuple including a copy of the given env_db and None, the latter
+        # meaning no environment has been selected.
         env_db = helpers.make_env_db()
-        new_env_db, env_data = views.env_index(
-            self.app, self.env_type_db, env_db, self.save_callable)
+        views.env_index(self.app, self.env_type_db, env_db, self.save_callable)
+        new_env_db, env_data = self.get_on_exit_return_value(self.loop)
         self.assertEqual(env_db, new_env_db)
         self.assertIsNot(env_db, new_env_db)
         self.assertIsNone(env_data)
@@ -249,10 +240,12 @@
             self.app, self.env_type_db, self.env_db, self.save_callable,
             self.env_data)
 
-    def test_view_return_value(self):
-        # The view returns a tuple including a copy of the given env_db and
-        # None, the latter meaning no environment has been selected (for now).
-        new_env_db, env_data = self.call_view()
+    def test_view_default_return_value_on_exit(self):
+        # The view configures the app so that the return value on user exit is
+        # a tuple including a copy of the given env_db and None, the latter
+        # meaning no environment has been selected (for now).
+        self.call_view()
+        new_env_db, env_data = self.get_on_exit_return_value(self.loop)
         self.assertEqual(self.env_db, new_env_db)
         self.assertIsNot(self.env_db, new_env_db)
         self.assertIsNone(env_data)