From a957c5b88998f68a95d5d05a612b3a0cb0ee45a7 Mon Sep 17 00:00:00 2001 From: Maciej Borzecki Date: Tue, 7 Apr 2020 17:38:45 +0200 Subject: [PATCH] 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 --- console_conf/controllers/chooser.py | 51 ++++++- .../controllers/tests/test_chooser.py | 121 +++++++++------ console_conf/ui/views/chooser.py | 142 ++++++++++-------- console_conf/ui/views/tests/__init__.py | 0 console_conf/ui/views/tests/test_chooser.py | 43 ++++++ 5 files changed, 244 insertions(+), 113 deletions(-) create mode 100644 console_conf/ui/views/tests/__init__.py create mode 100644 console_conf/ui/views/tests/test_chooser.py diff --git a/console_conf/controllers/chooser.py b/console_conf/controllers/chooser.py index db0e76b5..8cb4e707 100644 --- a/console_conf/controllers/chooser.py +++ b/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, - has_more=more) - else: - view = ChooserView(self, self.model.systems) - self.ui.set_body(view) + current_view = ChooserCurrentSystemView(self, + self.model.current, + has_more=more) + + all_view = ChooserView(self, self.model.systems) + 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() diff --git a/console_conf/controllers/tests/test_chooser.py b/console_conf/controllers/tests/test_chooser.py index 42b9e0ab..d38e63e8 100644 --- a/console_conf/controllers/tests/test_chooser.py +++ b/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() - app.base_model = 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.ChooserView') + @mock.patch('console_conf.controllers.chooser.ChooserCurrentSystemView', + 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.start_ui() + c.ui.start_ui = mock.Mock() # current system view is constructed ccsv.assert_called_with(c, c.model.current, has_more=True) - # but the all systems view is not - cv.assert_not_called() - ccsv.reset_mock() + # as well as all systems view + cv.assert_called_with(c, c.model.systems) + 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 - cv.assert_called_with(c, c.model.systems) - # and the current system view was not constructed - ccsv.assert_not_called() + c.ui.set_body.assert_called_with('all') - @mock.patch('console_conf.controllers.chooser.ChooserCurrentSystemView') - @mock.patch('console_conf.controllers.chooser.ChooserView') - def test_only_one_and_current(self, cv, ccsv): - app = make_app() + @mock.patch('console_conf.controllers.chooser.ChooserCurrentSystemView', + return_value='current') + @mock.patch('console_conf.controllers.chooser.ChooserView', + 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 - ccsv.assert_called_with(c, c.model.current, has_more=False) - # but the all systems view is not - cv.assert_not_called() + c.ui.set_body.assert_called_with('current') + # user selects more options and the view is replaced + c.more_options() + 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.ChooserView') - def test_all_systems_first_no_current(self, cv, ccsv): - app = make_app() + @mock.patch('console_conf.controllers.chooser.ChooserCurrentSystemView', + return_value='current') + @mock.patch('console_conf.controllers.chooser.ChooserView', + 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') diff --git a/console_conf/ui/views/chooser.py b/console_conf/ui/views/chooser.py index da799035..bc416c67 100644 --- a/console_conf/ui/views/chooser.py +++ b/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): - fmt = "Select one of available actions for \"{}\" by \"{}\"{}." - maybe_more = " or view all available systems" if has_more else "" - excerpt = fmt.format(current.model.display_name, - current.brand.display_name, - maybe_more) + def __init__(self, current, scr): + if current is not None: + self.title = current.model.display_name + 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: - actions.append(forward_btn(label=action.title, + for action in sorted(current.actions, key=by_preferred_action_type): + 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 = [ - reset_btn("ABORT", on_press=self.abort), - ] - - super().__init__(screen( - lb, - buttons=button_pile(buttons), - focus_buttons=False, - narrow_rows=True, - excerpt=excerpt)) + super().__init__(current, + screen( + lb, + narrow_rows=True, + 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): - self.controller.cancel() + def back(self, result): + self.controller.back() -class ChooserView(BaseView): - title = "Ubuntu Core" +class ChooserView(ChooserBaseView): 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: - actions.append(Action(label=act.title, + for act in sorted(s.actions, key=by_preferred_action_type): + 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,56 +151,67 @@ class ChooserView(BaseView): Pile([heading_table, systems_table]), ] - buttons = [ - reset_btn("ABORT", on_press=self.abort), - ] + buttons = [] + 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( - rows=rows, - buttons=button_pile(buttons), - focus_buttons=False, - excerpt=self.excerpt)) + super().__init__(controller.model.current, + screen( + rows=rows, + buttons=button_pile(buttons), + focus_buttons=False, + excerpt=self.excerpt)) def _system_action(self, sender, action, system): self.controller.select(system, action) - def abort(self, result): - self.controller.cancel() + def back(self, result): + self.controller.back() -class ChooserConfirmView(BaseView): - title = "Ubuntu Core" - excerpt = ("Summary of the selected action.") +class ChooserConfirmView(ChooserBaseView): + + 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), - ] - 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)]), + back_btn("BACK", on_press=self.back), ] + 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("")]), - Pile([TablePile(summary)]) + Text(summary), ] - super().__init__(screen( - rows=rows, - buttons=button_pile(buttons), - focus_buttons=False, - excerpt=self.excerpt)) - - def abort(self, result): - self.controller.cancel() + super().__init__(controller.model.current, + screen( + rows=rows, + buttons=button_pile(buttons), + focus_buttons=False)) def confirm(self, result): self.controller.confirm() + + def back(self, result): + self.controller.back() diff --git a/console_conf/ui/views/tests/__init__.py b/console_conf/ui/views/tests/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/console_conf/ui/views/tests/test_chooser.py b/console_conf/ui/views/tests/test_chooser.py new file mode 100644 index 00000000..3b7b8037 --- /dev/null +++ b/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 . +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)