console_conf/chooser: UI tweaks

- drop the ABORT button
- introduce BACK button, goes back to the correct previous screen
- sort actions in the order of run -> recover -> install -> rest
- capitalize action title strings
- simplify current model actions screen
- user hints in the confirm screen for well known modes
  - generic hint for unknown modes

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

console_conf/controllers/chooser.py

@@ -26,6 +26,7 @@ log = logging.getLogger("console_conf.controllers.chooser")
 
 
 class RecoveryChooserBaseController(BaseController):
+
     def __init__(self, app):
         super().__init__(app)
         self.model = app.base_model
@@ -34,25 +35,57 @@ class RecoveryChooserBaseController(BaseController):
         # exit without taking any action
         self.app.exit()
 
+    def back(self):
+        self.app.prev_sceen()
+
 
 class RecoveryChooserController(RecoveryChooserBaseController):
+
+    def __init__(self, app):
+        super().__init__(app)
+        self._model_view, self._all_view = self._make_views()
+        # one of the current views
+        self._current_view = None
+
     def start_ui(self):
+        # current view is preserved, so going back comes back to the right
+        # screen
+        if self._current_view is None:
+            # right off the start, default to all-systems view
+            self._current_view = self._all_view
+            # unless we can show the current model
+            if self._model_view is not None:
+                self._current_view = self._model_view
+
+        self.ui.set_body(self._current_view)
+
+    def _make_views(self):
+        current_view = None
         if self.model.current and self.model.current.actions:
             # only when we have a current system and it has actions available
-
             more = len(self.model.systems) > 1
-            view = ChooserCurrentSystemView(self, self.model.current,
+            current_view = ChooserCurrentSystemView(self,
+                                                    self.model.current,
                                                     has_more=more)
-        else:
+
-            view = ChooserView(self, self.model.systems)
+        all_view = ChooserView(self, self.model.systems)
-        self.ui.set_body(view)
+        return current_view, all_view
 
     def select(self, system, action):
         self.model.select(system, action)
         self.app.next_screen()
 
     def more_options(self):
-        self.ui.set_body(ChooserView(self, self.model.systems))
+        self._current_view = self._all_view
+        self.ui.set_body(self._all_view)
+
+    def back(self):
+        if self._current_view == self._all_view and \
+           self._model_view is not None:
+            # back in the all-systems screen goes back to the current model
+            # screen, provided we have one
+            self._current_view = self._model_view
+            self.ui.set_body(self._current_view)
 
 
 class RecoveryChooserConfirmController(RecoveryChooserBaseController):
@@ -65,3 +98,7 @@ class RecoveryChooserConfirmController(RecoveryChooserBaseController):
         # output the choice
         self.app.respond(self.model.selection)
         self.app.exit()
+
+    def back(self):
+        self.model.unselect()
+        self.app.prev_screen()

console_conf/controllers/tests/test_chooser.py

@@ -34,14 +34,18 @@ class MockedApplication:
     opts = None
 
 
-def make_app():
+def make_app(model=None):
     app = MockedApplication()
     app.ui = mock.Mock()
+    if model is not None:
+        app.base_model = model
+    else:
         app.base_model = mock.Mock()
     app.context = Context.new(app)
     app.exit = mock.Mock()
     app.respond = mock.Mock()
     app.next_screen = mock.Mock()
+    app.prev_screen = mock.Mock()
     return app
 
 
@@ -92,12 +96,15 @@ def make_model():
 
 class TestChooserConfirmController(unittest.TestCase):
 
-    def test_abort(self):
+    def test_back(self):
         app = make_app()
         c = RecoveryChooserConfirmController(app)
-        c.cancel()
+        c.model = mock.Mock(selection='selection')
+        c.back()
         app.respond.assert_not_called()
-        app.exit.assert_called()
+        app.exit.assert_not_called()
+        app.prev_screen.assert_called()
+        c.model.unselect.assert_called()
 
     def test_confirm(self):
         app = make_app()
@@ -119,17 +126,9 @@ class TestChooserConfirmController(unittest.TestCase):
 
 class TestChooserController(unittest.TestCase):
 
-    def test_abort(self):
-        app = make_app()
-        c = RecoveryChooserController(app)
-        c.cancel()
-        app.respond.assert_not_called()
-        app.exit.assert_called()
-
     def test_select(self):
-        app = make_app()
+        app = make_app(model=make_model())
         c = RecoveryChooserController(app)
-        c.model = make_model()
 
         c.select(c.model.systems[0], c.model.systems[0].actions[0])
         exp = SelectedSystemAction(system=c.model.systems[0],
@@ -140,50 +139,84 @@ class TestChooserController(unittest.TestCase):
         app.respond.assert_not_called()
         app.exit.assert_not_called()
 
-    @mock.patch('console_conf.controllers.chooser.ChooserCurrentSystemView')
+    @mock.patch('console_conf.controllers.chooser.ChooserCurrentSystemView',
-    @mock.patch('console_conf.controllers.chooser.ChooserView')
+                return_value='current')
+    @mock.patch('console_conf.controllers.chooser.ChooserView',
+                return_value='all')
     def test_current_ui_first(self, cv, ccsv):
-        app = make_app()
+        app = make_app(model=make_model())
         c = RecoveryChooserController(app)
-        c.model = make_model()
+        c.ui.start_ui = mock.Mock()
-
-        c.start_ui()
         # current system view is constructed
         ccsv.assert_called_with(c, c.model.current, has_more=True)
-        # but the all systems view is not
+        # as well as all systems view
-        cv.assert_not_called()
+        cv.assert_called_with(c, c.model.systems)
-        ccsv.reset_mock()
+        c.start_ui()
+        c.ui.set_body.assert_called_with('current')
         # user selects more options and the view is replaced
         c.more_options()
-        # we get the all systems view now
+        c.ui.set_body.assert_called_with('all')
-        cv.assert_called_with(c, c.model.systems)
-        # and the current system view was not constructed
-        ccsv.assert_not_called()
 
-    @mock.patch('console_conf.controllers.chooser.ChooserCurrentSystemView')
+    @mock.patch('console_conf.controllers.chooser.ChooserCurrentSystemView',
-    @mock.patch('console_conf.controllers.chooser.ChooserView')
+                return_value='current')
-    def test_only_one_and_current(self, cv, ccsv):
+    @mock.patch('console_conf.controllers.chooser.ChooserView',
-        app = make_app()
+                return_value='all')
+    def test_current_current_all_there_and_back(self, cv, ccsv):
+        app = make_app(model=make_model())
         c = RecoveryChooserController(app)
-        c.model = RecoverySystemsModel.from_systems([model2_current])
+        c.ui.start_ui = mock.Mock()
+        # sanity
+        ccsv.assert_called_with(c, c.model.current, has_more=True)
+        cv.assert_called_with(c, c.model.systems)
 
         c.start_ui()
-        # current system view is constructed
+        c.ui.set_body.assert_called_with('current')
-        ccsv.assert_called_with(c, c.model.current, has_more=False)
+        # user selects more options and the view is replaced
-        # but the all systems view is not
+        c.more_options()
-        cv.assert_not_called()
+        c.ui.set_body.assert_called_with('all')
+        # go back now
+        c.back()
+        c.ui.set_body.assert_called_with('current')
+        # nothing
+        c.back()
+        c.ui.set_body.not_called()
 
-    @mock.patch('console_conf.controllers.chooser.ChooserCurrentSystemView')
+    @mock.patch('console_conf.controllers.chooser.ChooserCurrentSystemView',
-    @mock.patch('console_conf.controllers.chooser.ChooserView')
+                return_value='current')
-    def test_all_systems_first_no_current(self, cv, ccsv):
+    @mock.patch('console_conf.controllers.chooser.ChooserView',
-        app = make_app()
+                return_value='all')
+    def test_only_one_and_current(self, cv, ccsv):
+        model = RecoverySystemsModel.from_systems([model2_current])
+        app = make_app(model=model)
         c = RecoveryChooserController(app)
-        c.model = RecoverySystemsModel.from_systems([model1_non_current])
+        c.ui.start_ui = mock.Mock()
+        # both views are constructed
+        ccsv.assert_called_with(c, c.model.current, has_more=False)
+        cv.assert_called_with(c, c.model.systems)
+
+        c.start_ui()
+        c.ui.set_body.assert_called_with('current')
+        # going back does nothing
+        c.back()
+        c.ui.set_body.not_called()
+
+    @mock.patch('console_conf.controllers.chooser.ChooserCurrentSystemView',
+                return_value='current')
+    @mock.patch('console_conf.controllers.chooser.ChooserView',
+                return_value='all')
+    def test_all_systems_first_no_current(self, cv, ccsv):
+        model = RecoverySystemsModel.from_systems([model1_non_current])
+        app = make_app(model=model)
+        c = RecoveryChooserController(app)
+        c.ui.start_ui = mock.Mock()
+
         # sanity
         self.assertIsNone(c.model.current)
 
-        c.start_ui()
+        # we get the all-systems view now
-        # we get the all systems view now
         cv.assert_called()
-        # current system view is not constructed
+        # current system view is not constructed at all
         ccsv.assert_not_called()
+
+        c.start_ui()
+        c.ui.set_body.assert_called_with('all')

console_conf/ui/views/chooser.py

@@ -26,8 +26,8 @@ from urwid import (
     )
 from subiquitycore.ui.buttons import (
     danger_btn,
-    reset_btn,
     forward_btn,
+    back_btn,
     )
 from subiquitycore.ui.actionmenu import (
     Action,
@@ -47,23 +47,36 @@ from subiquitycore.view import BaseView
 log = logging.getLogger("console_conf.views.chooser")
 
 
-class ChooserCurrentSystemView(BaseView):
+class ChooserBaseView(BaseView):
     title = "Ubuntu Core"
 
-    def __init__(self, controller, current, has_more=False):
+    def __init__(self, current, scr):
-        fmt = "Select one of available actions for \"{}\" by \"{}\"{}."
+        if current is not None:
-        maybe_more = " or view all available systems" if has_more else ""
+            self.title = current.model.display_name
-        excerpt = fmt.format(current.model.display_name,
-                             current.brand.display_name,
-                             maybe_more)
 
+        super().__init__(scr)
+
+
+def by_preferred_action_type(action):
+    """Order action entries by having the 'run' mode first, then 'recover', then
+    'install', the rest is ordered alphabetically."""
+    return (action.mode != "run",
+            action.mode != "recover",
+            action.mode != "install",
+            action.title.lower())
+
+
+class ChooserCurrentSystemView(ChooserBaseView):
+    excerpt = "Select action:"
+
+    def __init__(self, controller, current, has_more=False):
         self.controller = controller
-        log.debug('current system: %s', current)
         log.debug('more systems available: %s', has_more)
+        log.debug('current system: %s', current)
 
         actions = []
-        for action in current.actions:
+        for action in sorted(current.actions, key=by_preferred_action_type):
-            actions.append(forward_btn(label=action.title,
+            actions.append(forward_btn(label=action.title.capitalize(),
                                        on_press=self._current_system_action,
                                        user_arg=(current, action)))
 
@@ -75,16 +88,11 @@ class ChooserCurrentSystemView(BaseView):
 
         lb = ListBox(actions)
 
-        buttons = [
+        super().__init__(current,
-            reset_btn("ABORT", on_press=self.abort),
+                         screen(
-        ]
-
-        super().__init__(screen(
                              lb,
-            buttons=button_pile(buttons),
-            focus_buttons=False,
                              narrow_rows=True,
-            excerpt=excerpt))
+                             excerpt=self.excerpt))
 
     def _current_system_action(self, sender, arg):
         current, action = arg
@@ -93,12 +101,11 @@ class ChooserCurrentSystemView(BaseView):
     def _more_options(self, sender):
         self.controller.more_options()
 
-    def abort(self, result):
+    def back(self, result):
-        self.controller.cancel()
+        self.controller.back()
 
 
-class ChooserView(BaseView):
+class ChooserView(ChooserBaseView):
-    title = "Ubuntu Core"
     excerpt = ("Select one of available recovery systems and a desired "
                "action to execute.")
 
@@ -123,8 +130,8 @@ class ChooserView(BaseView):
         for s in systems:
             actions = []
             log.debug('actions: %s', s.actions)
-            for act in s.actions:
+            for act in sorted(s.actions, key=by_preferred_action_type):
-                actions.append(Action(label=act.title,
+                actions.append(Action(label=act.title.capitalize(),
                                       value=act,
                                       enabled=True))
             menu = ActionMenu(actions)
@@ -133,7 +140,7 @@ class ChooserView(BaseView):
                 Text(s.label),
                 Text(s.model.display_name),
                 Text(s.brand.display_name),
-                Text("(current)" if s.current else ""),
+                Text("(installed)" if s.current else ""),
                 menu,
             ], menu)
             trows.append(srow)
@@ -144,11 +151,13 @@ class ChooserView(BaseView):
             Pile([heading_table, systems_table]),
         ]
 
-        buttons = [
+        buttons = []
-            reset_btn("ABORT", on_press=self.abort),
+        if controller.model.current is not None:
-        ]
+            # back to options of current system
+            buttons.append(back_btn("BACK", on_press=self.back))
 
-        super().__init__(screen(
+        super().__init__(controller.model.current,
+                         screen(
                              rows=rows,
                              buttons=button_pile(buttons),
                              focus_buttons=False,
@@ -157,43 +166,52 @@ class ChooserView(BaseView):
     def _system_action(self, sender, action, system):
         self.controller.select(system, action)
 
-    def abort(self, result):
+    def back(self, result):
-        self.controller.cancel()
+        self.controller.back()
 
 
-class ChooserConfirmView(BaseView):
+class ChooserConfirmView(ChooserBaseView):
-    title = "Ubuntu Core"
+
-    excerpt = ("Summary of the selected action.")
+    canned_summary = {
+        "run": "Continue running the system without any changes.",
+        "recover": ("You have requested to reboot the system into recovery "
+                    "mode."),
+        "install": ("You are about to {action} the system version {version} "
+                    "for {model} from {publisher}.\n\n"
+                    "This will remove all existing user data on the "
+                    "device.\n\n"
+                    "The system will reboot in the process."),
+    }
+    default_summary = ("You are about to execute action \"{action}\" using "
+                       "system version {version} for device {model} from "
+                       "{publisher}.\n\n"
+                       "Make sure you understand the consequences of doing "
+                       "so.")
 
     def __init__(self, controller, selection):
         self.controller = controller
 
         buttons = [
             danger_btn("CONFIRM", on_press=self.confirm),
-            reset_btn("ABORT", on_press=self.abort),
+            back_btn("BACK", on_press=self.back),
-        ]
-        using_summary = "System seed of device {} version {} from {}".format(
-                          selection.system.model.display_name,
-                          selection.system.label,
-                          selection.system.brand.display_name
-                      )
-        summary = [
-            TableRow([Text("Action:"), Color.info_error(Text(
-                selection.action.title))]),
-            TableRow([Text("Using:"), Text(using_summary)]),
         ]
+        fmt = self.canned_summary.get(selection.action.mode,
+                                      self.default_summary)
+        summary = fmt.format(action=selection.action.title,
+                             model=selection.system.model.display_name,
+                             publisher=selection.system.brand.display_name,
+                             version=selection.system.label)
         rows = [
-            Pile([Text("")]),
+            Text(summary),
-            Pile([TablePile(summary)])
         ]
-        super().__init__(screen(
+        super().__init__(controller.model.current,
+                         screen(
                              rows=rows,
                              buttons=button_pile(buttons),
-            focus_buttons=False,
+                             focus_buttons=False))
-            excerpt=self.excerpt))
-
-    def abort(self, result):
-        self.controller.cancel()
 
     def confirm(self, result):
         self.controller.confirm()
+
+    def back(self, result):
+        self.controller.back()

console_conf/ui/views/tests/test_chooser.py

@@ -0,0 +1,43 @@
+# Copyright 2020 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 as
+# published by the Free Software Foundation, either version 3 of the
+# License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY 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/>.
+import unittest
+
+from console_conf.models.systems import (
+    SystemAction,
+    )
+from console_conf.ui.views.chooser import (
+    by_preferred_action_type,
+    )
+
+
+class TestSorting(unittest.TestCase):
+
+    def test_simple(self):
+        data = [
+            SystemAction(title="b-other", mode="unknown"),
+            SystemAction(title="reinstall", mode="install"),
+            SystemAction(title="run normally", mode="run"),
+            SystemAction(title="a-other", mode="some-other"),
+            SystemAction(title="recover", mode="recover"),
+        ]
+        exp = [
+            SystemAction(title="run normally", mode="run"),
+            SystemAction(title="recover", mode="recover"),
+            SystemAction(title="reinstall", mode="install"),
+            SystemAction(title="a-other", mode="some-other"),
+            SystemAction(title="b-other", mode="unknown"),
+        ]
+        self.assertSequenceEqual(sorted(data, key=by_preferred_action_type),
+                                 exp)