From 014d9d0d01e8df5a6d586ca6fb17bee23346b725 Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Tue, 12 Apr 2022 19:17:09 +0200 Subject: [PATCH 1/3] ui: introduce an optional level of indirection in make_ui The make_ui() function / coroutine returns a BaseView (i.e., a screen to display to the user). That being said, when the application calls, make_ui(), it does not come with a guarantee that the view returned will be displayed to the user immediately. One of the reason is that there are multiple await statements before the we call the ui.set_body function. Therefore, tasks running concurrently cannot reliably expect that they execute after the display is refreshed. Perhaps more importantly, when the make_ui() function takes more than .1 second to execute, we display a "Progress" screen that stays visible for at least one second. This can effectively delay a lot the moment when the view returned by make_ui() is shown to the user. A lot can happen in the meantime. As the result, the view returned by make_ui can be outdated by the time we show it on the screen. One way to work around this problem is to store in the controller a reference to the view that it returns in make_ui(). This way, the controller can modify the view and keep it up-to-date until it gets shown to the user. Unfortunately, some controllers (e.g., the storage controller) do not modify / mutate the existing view object when a modification is needed ; but instead instantiate a new view object. This patch introduces a level of indirection that can be used by these controllers. Instead of returning a view object from make_ui(), the controllers are now allowed to return a callback ; which in turn will return a view object. https://bugs.launchpad.net/subiquity/+bug/1968161 Signed-off-by: Olivier Gayot --- subiquity/client/client.py | 15 ++++++++++++--- subiquitycore/tui.py | 16 ++++++++++++---- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/subiquity/client/client.py b/subiquity/client/client.py index 47c42739..1f49d078 100644 --- a/subiquity/client/client.py +++ b/subiquity/client/client.py @@ -21,7 +21,7 @@ import os import signal import sys import traceback -from typing import Dict, List, Optional +from typing import Callable, Dict, List, Optional, Union import aiohttp @@ -486,11 +486,20 @@ class SubiquityClient(TuiApplication): log.debug("showing InstallConfirmation over %s", self.ui.body) self.add_global_overlay(InstallConfirmation(self)) - async def _start_answers_for_view(self, controller, view): + async def _start_answers_for_view( + self, controller, view: Union[BaseView, Callable[[], BaseView]]): + def noop(): + return view + + if callable(view): + deref_view = view + else: + deref_view = noop + # The view returned by make_view_for_controller is not always shown # immediately (if progress is being shown, but has not yet been shown # for a full second) so wait until it is before starting the answers. - while self.ui.body is not view: + while self.ui.body is not deref_view(): await asyncio.sleep(0.1) coro = controller.run_answers() if inspect.iscoroutine(coro): diff --git a/subiquitycore/tui.py b/subiquitycore/tui.py index d21627ce..329cf23a 100644 --- a/subiquitycore/tui.py +++ b/subiquitycore/tui.py @@ -18,6 +18,7 @@ import inspect import logging import os import signal +from typing import Callable, Optional, Union import urwid @@ -34,6 +35,7 @@ from subiquitycore.tuicontroller import Skip from subiquitycore.ui.utils import LoadingDialog from subiquitycore.ui.frame import SubiquityCoreUI from subiquitycore.utils import astart_command +from subiquitycore.view import BaseView log = logging.getLogger('subiquitycore.tui') @@ -122,7 +124,8 @@ class TuiApplication(Application): before_hook() schedule_task(_run()) - async def make_view_for_controller(self, new): + async def make_view_for_controller(self, new) \ + -> Union[BaseView, Callable[[], BaseView]]: new.context.enter("starting UI") if self.opts.screens and new.name not in self.opts.screens: raise Skip @@ -205,7 +208,8 @@ class TuiApplication(Application): async def wait_with_progress(self, awaitable): return await self._wait_with_indication(awaitable, self.show_progress) - async def _move_screen(self, increment, coro): + async def _move_screen(self, increment, coro) \ + -> Optional[Union[BaseView, Callable[[], BaseView]]]: if coro is not None: await coro old, self.cur_screen = self.cur_screen, None @@ -234,9 +238,13 @@ class TuiApplication(Application): return async def move_screen(self, increment, coro): - view = await self.wait_with_progress( + view_or_callable = await self.wait_with_progress( self._move_screen(increment, coro)) - if view is not None: + if view_or_callable is not None: + if callable(view_or_callable): + view = view_or_callable() + else: + view = view_or_callable self.ui.set_body(view) def next_screen(self, coro=None): From 414a2235e688ca62ba04a279816c0facce469338 Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Tue, 12 Apr 2022 20:39:22 +0200 Subject: [PATCH 2/3] storage: fix screen sometimes not refreshing after slow probing When we reach the storage screens on the installer, if the devices probing operation has not finished, we: * display a temporary "Probing" screen. * create an asynchronous task (a.k.a., probing task) that will eventually show the "Guide Storage" screen when the probing operation finishes. The probing task checks, when it finishes, that the screen currently visible is the "Probing" screen. This is the expectation and is true in most scenarios. But in case a different screen is visible, we skip refreshing the display. Unfortunately, sometimes, a "Progress" screen is shown for some time before the "Probing" screen appears. Consequently, we do not refresh the screen if the probing operation finishes whilst the Progress screen is visible. In order to keep the view returned by make_ui() up-to-date and make sure that the right screen is shown even if the probing operation finishes early, we use the level indirection that was implemented in make_ui. https://bugs.launchpad.net/subiquity/+bug/1968161 Signed-off-by: Olivier Gayot --- subiquity/client/controllers/filesystem.py | 25 ++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/subiquity/client/controllers/filesystem.py b/subiquity/client/controllers/filesystem.py index 5ec56c3b..5fb9a400 100644 --- a/subiquity/client/controllers/filesystem.py +++ b/subiquity/client/controllers/filesystem.py @@ -15,8 +15,10 @@ import asyncio import logging +from typing import Callable, Optional from subiquitycore.lsb_release import lsb_release +from subiquitycore.view import BaseView from subiquity.client.controller import SubiquityTuiController from subiquity.common.filesystem import gaps @@ -50,19 +52,34 @@ class FilesystemController(SubiquityTuiController, FilesystemManipulator): self.answers.setdefault('guided', False) self.answers.setdefault('guided-index', 0) self.answers.setdefault('manual', []) + self.current_view: Optional[BaseView] = None + + async def make_ui(self) -> Callable[[], BaseView]: + def get_current_view() -> BaseView: + assert self.current_view is not None + return self.current_view - async def make_ui(self): status = await self.endpoint.guided.GET() if status.status == ProbeStatus.PROBING: self.app.aio_loop.create_task(self._wait_for_probing()) - return SlowProbing(self) + self.current_view = SlowProbing(self) else: - return self.make_guided_ui(status) + self.current_view = self.make_guided_ui(status) + # NOTE: If we return a BaseView instance directly here, we have no + # guarantee that it will be displayed on the screen by the time the + # probing operation finishes. Therefore, to allow us to reliably + # replace the screen by the "Guided Storage" when the probing operation + # finishes, we add a level of indirection. + # In essence, this allows us to make modifications to the screen + # that eventually will be displayed. + # This is mostly a workaround for the issue described in LP #1968161 + return get_current_view async def _wait_for_probing(self): status = await self.endpoint.guided.GET(wait=True) + self.current_view = self.make_guided_ui(status) if isinstance(self.ui.body, SlowProbing): - self.ui.set_body(self.make_guided_ui(status)) + self.ui.set_body(self.current_view) else: log.debug("not refreshing the display. Current display is %r", self.ui.body) From cb132611c2df1f7161df1600e942f5ef18e2604d Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Wed, 13 Apr 2022 18:31:30 +0200 Subject: [PATCH 3/3] ui: get rid of unreachable else block The following commit added an unconditional return statement in the try block of _move_screen, effectively making the associated else block unreachable. a7bcc7fa add a way to wait for something with notification after 0.1s Signed-off-by: Olivier Gayot --- subiquitycore/tui.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/subiquitycore/tui.py b/subiquitycore/tui.py index 329cf23a..edd0d89a 100644 --- a/subiquitycore/tui.py +++ b/subiquitycore/tui.py @@ -234,8 +234,6 @@ class TuiApplication(Application): except Exception: self.controllers.index = cur_index raise - else: - return async def move_screen(self, increment, coro): view_or_callable = await self.wait_with_progress(