Improve config error handling

- Errors are now combined if possible
- Rich text output in message boxes
- ConfigContainer errors are collected properly

qutebrowser/app.py

@@ -408,7 +408,7 @@ def _init_modules(args, crash_handler):
     objreg.register('readline-bridge', readline_bridge)
 
     log.init.debug("Initializing config...")
-    config.init(args, qApp)
+    config.init(qApp)
     save_manager.init_autosave()
 
     log.init.debug("Initializing sql...")

qutebrowser/config/config.py

@@ -24,17 +24,19 @@
 import functools
 
 from PyQt5.QtCore import pyqtSignal, pyqtSlot, QObject, QUrl
+from PyQt5.QtWidgets import QApplication, QMessageBox
 
 from qutebrowser.config import configdata, configexc, configtypes, configfiles
-from qutebrowser.utils import utils, objreg, message, log, usertypes, error
-from qutebrowser.misc import objects
+from qutebrowser.utils import utils, objreg, message, log, usertypes
+from qutebrowser.misc import objects, msgbox
 from qutebrowser.commands import cmdexc, cmdutils
 from qutebrowser.completion.models import configmodel
 
 # An easy way to access the config from other code via config.val.foo
 val = None
 instance = None
 key_instance = None
+_errbox = None
 
 # Keeping track of all change filters to validate them later.
 _change_filters = []
@@ -500,27 +502,28 @@ class ConfigContainer:
     Attributes:
         _config: The Config object.
         _prefix: The __getattr__ chain leading up to this object.
-        _confpy: If True, get values suitable for config.py and
-                 do not raise exceptions.
-        _errors: If confpy=True is given, a list of configexc.ConfigErrorDesc.
+        _configapi: If given, get values suitable for config.py and
+                    add errors to the given ConfigAPI object.
     """
 
-    def __init__(self, config, confpy=False, prefix=''):
+    def __init__(self, config, configapi=None, prefix=''):
         self._config = config
         self._prefix = prefix
-        self._confpy = confpy
-        self._errors = []
+        self._configapi = configapi
 
     def __repr__(self):
         return utils.get_repr(self, constructor=True, config=self._config,
-                              confpy=self._confpy, prefix=self._prefix)
+                              configapi=self._configapi, prefix=self._prefix)
 
     @contextlib.contextmanager
     def _handle_error(self, action, name):
         try:
             yield
         except configexc.Error as e:
-            self._errors.append(configexc.ConfigErrorDesc(action, name, e))
+            if self._configapi is None:
+                raise
+            text = "While {} '{}'".format(action, name)
+            self._configapi.errors.append(configexc.ConfigErrorDesc(text, e))
 
     def __getattr__(self, attr):
         """Get an option or a new ConfigContainer with the added prefix.
@@ -536,14 +539,17 @@ def __getattr__(self, attr):
 
         name = self._join(attr)
         if configdata.is_valid_prefix(name):
-            return ConfigContainer(config=self._config, confpy=self._confpy,
+            return ConfigContainer(config=self._config,
+                                   configapi=self._configapi,
                                    prefix=name)
 
         with self._handle_error('getting', name):
-            if self._confpy:
-                return self._config.get_obj(name)
-            else:
+            if self._configapi is None:
+                # access from Python code
                 return self._config.get(name)
+            else:
+                # access from config.py
+                return self._config.get_obj(name)
 
     def __setattr__(self, attr, value):
         """Set the given option in the config."""
@@ -630,7 +636,7 @@ def register(self, update):
             instance.changed.connect(self._update_stylesheet)
 
 
-def init(args, parent=None):
+def init(parent=None):
     """Initialize the config.
 
     Args:
@@ -655,9 +661,13 @@ def init(args, parent=None):
 
     try:
         config_api = configfiles.read_config_py()
-    except configexc.ConfigFileError as e:
-        error.handle_fatal_exc(e, args=args,
-                               title="Error while loading config")
+    except configexc.ConfigFileErrors as e:
+        global _errbox
+        _errbox = msgbox.msgbox(parent=None,
+                                title="Error while reading config",
+                                text=e.to_html(),
+                                icon=QMessageBox.Warning,
+                                plain_text=False)
         config_api = None
 
     if getattr(config_api, 'load_autoconfig', True):

qutebrowser/config/configexc.py

@@ -20,7 +20,7 @@
 """Exceptions related to config parsing."""
 
 
-import traceback
+from qutebrowser.utils import jinja
 
 
 class Error(Exception):
@@ -75,49 +75,46 @@ def __init__(self, option):
         self.option = option
 
 
-class ConfigFileError(Error):
-
-    """Raised when there was an error while loading a config file."""
-
-    def __init__(self, basename, msg):
-        super().__init__("Failed to load {}: {}".format(basename, msg))
-
-
-class ConfigFileUnhandledException(ConfigFileError):
-
-    """Raised when there was an unhandled exception while loading config.py.
-
-    Needs to be raised from an exception handler.
-    """
-
-    def __init__(self, basename):
-        super().__init__(basename, "Unhandled exception\n\n{}".format(
-            traceback.format_exc()))
-
-
 class ConfigErrorDesc:
 
     """A description of an error happening while reading the config.
 
     Attributes:
-        _action: What action has been taken, e.g 'set'
-        _name: The option which was set, or the key which was bound.
-        _exception: The exception which happened.
+        text: The text to show.
+        exception: The exception which happened.
+        traceback: The formatted traceback of the exception.
     """
 
-    def __init__(self, action, name, exception):
-        self._action = action
-        self._exception = exception
-        self._name = name
-
-    def __str__(self):
-        return "While {} {}: {}".format(
-            self._action, self._name, self._exception)
+    def __init__(self, text, exception, traceback=None):
+        self.text = text
+        self.exception = exception
+        self.traceback = traceback
 
 
-class ConfigFileErrors(ConfigFileError):
+class ConfigFileErrors(Error):
 
     """Raised when multiple errors occurred inside the config."""
 
     def __init__(self, basename, errors):
-        super().__init__(basename, "\n\n".join(str(err) for err in errors))
+        super().__init__("Errors occurred while reading {}".format(basename))
+        self.basename = basename
+        self.errors = errors
+
+    def to_html(self):
+        template = jinja.environment.from_string("""
+        Errors occurred while reading {{ basename }}:
+
+        <ul>
+          {% for error in errors %}
+            <li>
+              <b>{{ error.text }}</b>: {{ error.exception }}
+              {% if error.traceback != none %}
+                <pre>
+        """.rstrip() + "\n{{ error.traceback }}" + """
+                </pre>
+              {% endif %}
+            </li>
+          {% endfor %}
+        </ul>
+        """)
+        return template.render(basename=self.basename, errors=self.errors)

qutebrowser/config/configfiles.py

@@ -110,24 +110,24 @@ class ConfigAPI:
         errors: Errors which occurred while setting options.
     """
 
-    def __init__(self, config, keyconfig, container):
+    def __init__(self, config, keyconfig):
         self._config = config
         self._keyconfig = keyconfig
-        self.val = container
         self.load_autoconfig = True
         self.errors = []
+        self.val = None  # Set when initialized
 
     @contextlib.contextmanager
     def _handle_error(self, action, name):
         try:
             yield
         except configexc.Error as e:
-            self.errors.append(configexc.ConfigErrorDesc(action, name, e))
+            text = "While {} '{}'".format(action, name)
+            self.errors.append(configexc.ConfigErrorDesc(text, e))
 
     def finalize(self):
         """Needs to get called after reading config.py is done."""
         self._config.update_mutables()
-        self.errors += self.val._errors  # pylint: disable=protected-access
 
     def get(self, name):
         with self._handle_error('getting', name):
@@ -155,32 +155,42 @@ def read_config_py(filename=None):
         if not os.path.exists(filename):
             return None
 
-    container = config.ConfigContainer(config.instance, confpy=True)
-    api = ConfigAPI(config.instance, config.key_instance, container)
+    api = ConfigAPI(config.instance, config.key_instance)
+    container = config.ConfigContainer(config.instance, configapi=api)
+    api.val = container
+
     module = types.ModuleType('config')
     module.config = api
-    module.c = api.val
+    module.c = container
     module.__file__ = filename
+
     basename = os.path.basename(filename)
 
     try:
         with open(filename, mode='rb') as f:
             source = f.read()
     except OSError as e:
-        raise configexc.ConfigFileError(basename, e.strerror)
+        text = "Error while reading {}".format(basename)
+        desc = configexc.ConfigErrorDesc(text, e)
+        raise configexc.ConfigFileErrors(basename, [desc])
 
     try:
         code = compile(source, filename, 'exec')
     except ValueError as e:
         # source contains NUL bytes
-        raise configexc.ConfigFileError(basename, str(e))
-    except SyntaxError:
-        raise configexc.ConfigFileUnhandledException(basename)
+        desc = configexc.ConfigErrorDesc("Error while compiling", e)
+        raise configexc.ConfigFileErrors(basename, [desc])
+    except SyntaxError as e:
+        desc = configexc.ConfigErrorDesc("Syntax Error", e,
+                                         traceback=traceback.format_exc())
+        raise configexc.ConfigFileErrors(basename, [desc])
 
     try:
         exec(code, module.__dict__)
-    except Exception:
-        raise configexc.ConfigFileUnhandledException(basename)
+    except Exception as e:
+        api.errors.append(configexc.ConfigErrorDesc(
+            "Unhandled exception",
+            exception=e, traceback=traceback.format_exc()))
 
     api.finalize()
     if api.errors:

qutebrowser/misc/msgbox.py

@@ -41,6 +41,7 @@ def msgbox(parent, title, text, *, icon, buttons=QMessageBox.Ok,
         A new QMessageBox.
     """
     box = QMessageBox(parent)
+    box.setAttribute(Qt.WA_DeleteOnClose)
     box.setIcon(icon)
     box.setStandardButtons(buttons)
     if on_finished is not None:

qutebrowser/utils/error.py

@@ -36,8 +36,7 @@ def _get_name(exc):
     return name
 
 
-def handle_fatal_exc(exc, args, title, *, pre_text='', post_text='',
-                     richtext=False):
+def handle_fatal_exc(exc, args, title, *, pre_text='', post_text=''):
     """Handle a fatal "expected" exception by displaying an error box.
 
     If --no-err-windows is given as argument, the text is logged to the error
@@ -49,7 +48,6 @@ def handle_fatal_exc(exc, args, title, *, pre_text='', post_text='',
         title: The title to be used for the error message.
         pre_text: The text to be displayed before the exception text.
         post_text: The text to be displayed after the exception text.
-        richtext: If given, interpret the given text as rich text.
     """
     if args.no_err_windows:
         lines = [
@@ -69,6 +67,4 @@ def handle_fatal_exc(exc, args, title, *, pre_text='', post_text='',
         if post_text:
             msg_text += '\n\n{}'.format(post_text)
         msgbox = QMessageBox(QMessageBox.Critical, title, msg_text)
-        if richtext:
-            msgbox.setTextFormat(Qt.RichText)
         msgbox.exec_()

tests/unit/config/test_config.py

@@ -777,12 +777,12 @@ def test_getattr_prefix(self, container):
         new_container = new_container.favicons
         assert new_container._prefix == 'tabs.favicons'
 
-    @pytest.mark.parametrize('confpy, expected', [
-        (True, 'rgb'),
-        (False, QColor.Rgb),
+    @pytest.mark.parametrize('configapi, expected', [
+        (object(), 'rgb'),
+        (None, QColor.Rgb),
     ])
-    def test_getattr_option(self, container, confpy, expected):
-        container._confpy = confpy
+    def test_getattr_option(self, container, configapi, expected):
+        container._configapi = configapi
         # Use an option with a to_py() so we can check the conversion.
         assert container.colors.downloads.system.fg == expected
 
@@ -877,7 +877,7 @@ def test_init(init_patch, fake_save_manager, config_tmpdir, load_autoconfig):
         config_py_lines.append('config.load_autoconfig = False')
     config_py_file.write_text('\n'.join(config_py_lines), 'utf-8', ensure=True)
 
-    config.init(args=None)
+    config.init()
 
     objreg.get('config-commands')
     assert isinstance(config.instance, config.Config)
@@ -899,4 +899,4 @@ def test_init(init_patch, fake_save_manager, config_tmpdir, load_autoconfig):
 def test_init_invalid_change_filter(init_patch):
     config.change_filter('foobar')
     with pytest.raises(configexc.NoOptionError):
-        config.init(args=None)
+        config.init()