From 4025cbf97fc6a5acee2331b19372caa0ffa566f8 Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Mon, 20 Jun 2022 10:41:20 +0200 Subject: [PATCH 1/2] ui: avoid crashing when removing overlay that does not exist BaseView.remove_overlay() would crash with AttributeError if no overlay was found. We now add a not_found_ok parameter (defaulting to True) that makes the function silently return if the overlay could not be found. Passing not_found_ok=False and catching OverlayNotFoundError can be helpful in some scenarios to do something different if no overlay was found. Signed-off-by: Olivier Gayot --- subiquitycore/tests/test_view.py | 19 ++++++++++++++++++- subiquitycore/view.py | 27 +++++++++++++++++++++------ 2 files changed, 39 insertions(+), 7 deletions(-) diff --git a/subiquitycore/tests/test_view.py b/subiquitycore/tests/test_view.py index 68b07d07..cb0e740a 100644 --- a/subiquitycore/tests/test_view.py +++ b/subiquitycore/tests/test_view.py @@ -16,7 +16,7 @@ import urwid from subiquitycore.tests import SubiTestCase -from subiquitycore.view import BaseView +from subiquitycore.view import BaseView, OverlayNotFoundError from subiquitycore.ui.stretchy import Stretchy, StretchyOverlay from subiquitycore.ui.utils import undisabled @@ -93,3 +93,20 @@ class TestBaseView(SubiTestCase): bv.remove_overlay() self.assertTrue(c.was_closed) self.assertEqual(self.get_stretchy_chain(bv), [b, a]) + + def test_remove_overlay_not_found(self): + bv, a, b, c = self.make_view_with_overlays() + bv.remove_overlay(not_found_ok=False) + bv.remove_overlay(not_found_ok=False) + bv.remove_overlay(not_found_ok=False) + + # At this point, there is no more overlay. + with self.assertRaises(OverlayNotFoundError): + bv.remove_overlay(not_found_ok=False) + + bv.remove_overlay(not_found_ok=True) + + with self.assertRaises(OverlayNotFoundError): + bv.remove_overlay(stretchy=a, not_found_ok=False) + + bv.remove_overlay(stretchy=a, not_found_ok=True) diff --git a/subiquitycore/view.py b/subiquitycore/view.py index 0687af44..8a26ff27 100644 --- a/subiquitycore/view.py +++ b/subiquitycore/view.py @@ -38,6 +38,10 @@ from subiquitycore.ui.utils import disabled, undisabled log = logging.getLogger('subiquitycore.view') +class OverlayNotFoundError(Exception): + """ Exception to raise when trying to remove a non-existent overlay. """ + + class BaseView(WidgetWrap): def local_help(self): @@ -78,7 +82,9 @@ class BaseView(WidgetWrap): stretchy.opened() self._w = StretchyOverlay(disabled(self._w), stretchy) - def remove_overlay(self, stretchy=None): + def remove_overlay(self, stretchy=None, + *, not_found_ok=True) -> None: + """ Remove (frontmost) overlay from the view. """ if stretchy is not None: one_above = None cur = self._w @@ -94,11 +100,21 @@ class BaseView(WidgetWrap): return one_above = cur cur = undisabled(cur.bottom_w) + else: + if not not_found_ok: + raise OverlayNotFoundError else: + try: + behind_overlay = self._w.bottom_w + except AttributeError: + if not_found_ok: + return + raise OverlayNotFoundError + if isinstance(self._w, StretchyOverlay): emit_signal(self._w.stretchy, 'closed') self._w.stretchy.closed() - self._w = undisabled(self._w.bottom_w) + self._w = undisabled(behind_overlay) def cancel(self): pass @@ -106,10 +122,9 @@ class BaseView(WidgetWrap): def keypress(self, size, key): key = super().keypress(size, key) if key == 'esc': - if hasattr(self._w, 'bottom_w'): - self.remove_overlay() - return None - else: + try: + self.remove_overlay(not_found_ok=False) + except OverlayNotFoundError: self.cancel() return None return key From 163e6cfb4a8e43010a307088712a73b6630406d0 Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Wed, 7 Dec 2022 09:58:26 +0100 Subject: [PATCH 2/2] ui: make not_found_ok false by default for overlays and pass it where needed Attempting to close an overlay that does not exist is pretty much always a bug in the code. Making not_found_ok true by default will hide obvious bugs from us ; which is not a good thing. Perhaps more importantly, we might just remove the wrong overlay. Instead, we should just pass not_found_ok=True as a workaround when we know the code is buggy and don't have time to fix the bug cleanly. This is what happens for SSH keys import. If the import fails, we remove the loading animation. However for answers-based runs, we do not have a loading animation so the code bails. Let's add not_found_ok=True in this context and we can fix the code later. Signed-off-by: Olivier Gayot --- subiquity/ui/views/ssh.py | 4 +++- subiquitycore/view.py | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/subiquity/ui/views/ssh.py b/subiquity/ui/views/ssh.py index 4fce2b46..5d63a7ac 100644 --- a/subiquity/ui/views/ssh.py +++ b/subiquity/ui/views/ssh.py @@ -297,5 +297,7 @@ class SSHView(BaseView): ConfirmSSHKeys(self, ssh_data, identities)) def fetching_ssh_keys_failed(self, msg, stderr): - self.remove_overlay() + # FIXME in answers-based runs, the overlay does not exist so we pass + # not_found_ok=True. + self.remove_overlay(not_found_ok=True) self.show_stretchy_overlay(SomethingFailed(self, msg, stderr)) diff --git a/subiquitycore/view.py b/subiquitycore/view.py index 8a26ff27..bfc07314 100644 --- a/subiquitycore/view.py +++ b/subiquitycore/view.py @@ -83,7 +83,7 @@ class BaseView(WidgetWrap): self._w = StretchyOverlay(disabled(self._w), stretchy) def remove_overlay(self, stretchy=None, - *, not_found_ok=True) -> None: + *, not_found_ok=False) -> None: """ Remove (frontmost) overlay from the view. """ if stretchy is not None: one_above = None