Repository: qutebrowser/qutebrowser
Pull request: Add qt.environ setting in config
Any environment variables can be set using the qt.environ setting in the config.

Closes #5899 

<!-- Thanks for submitting a pull request! Please pick a descriptive title (not just "issue 12345"). If there is an open issue associated to your PR, please add a line like "Closes #12345" somewhere in the PR description (outside of this comment) -->


Problem statement:
qt.environ setting
Sometimes it can be useful to set environment variables for qutebrowser only. This can be done in `config.py` already, but having a setting would make things a bit easier (also to test via `--temp-basedir -s`.

In the config, this would be of `type: Dict` with `keytype: String` and `valtype: String`, as well as `restart: true` set (perhaps also `none_ok: true` so that `None` can be used as a value to remove a previously set variable).

Then qutebrowser would apply those in `qutebrowser/config/qtargs.py` or so, similar to how other settings (e.g. `config.val.qt.force_platform`) are already mapped to environment variables

Hints from issue discussion:
Hello @The-Compiler , can you assign me this issue? I'd like to contribute and am just starting out with open source.
@StutiV Sure, please go ahead and don't hesitate to ask if you get stuck somewhere! :+1: 
@The-Compiler can you elaborate the issue a bit? It'd be really helpful. Thank you!
So, the aim of this is to be able to set [environment variables](https://en.wikipedia.org/wiki/Environment_variable) via the qutebrowser config. For example, if you run this (on Linux/macOS, see the Wikipedia article for Windows):

```con
QT_SCALE_FACTOR=4 qutebrowser --temp-basedir
```

you should be able to start qutebrowser with a much bigger user interface.

The idea is now that this can be specified via the qutebrowser configuration instead, which you can test using something like:

```con
qutebrowser --temp-basedir -s qt.environ '{"QT_SCALE_FACTOR": "4"}'
```

(once this is actually implemented).

To implement it, roughly those steps are needed:

1) Add a new setting to `qutebrowser/config/configdata.yml` (see my notes above and e.g. the `qt.args` setting for some inspiration)
2) Handle the setting in `qutebrowser/config/qtargs.py`, probably best in `init_envvars`.
3) In `tests/unit/config/test_qtargs.py`, adjust the parametrization for the `test_env_vars` function to add a test for the new `qt.environ` setting
4) Finally, also handle the case where a value has been set to `None`/`null` in the config, so that a user can set that to *remove* an existing environment variable - for example:

```con
QT_SCALE_FACTOR=4 qutebrowser --temp-basedir -s qt.environ '{"QT_SCALE_FACTOR": null}'
```

should result in qutebrowser *not* scaling up the UI.
5) Add another test or two for that - make sure you cover both the cases where the variable is already set (so there's something to delete) and where it isn't set - you can use pytest's [monkeypatch](https://docs.pytest.org/en/stable/monkeypatch.html) to do so.
@StutiV Did you intend to unassign yourself?
Is this still being worked on by @StutiV ? I'll give implementing it a shot if not.
I have no idea, but since they unassigned themselves and didn't answer anymore, I guess not... Please go ahead!

Commit message: Add qt.environ setting in config

Any environment variables can be set using the qt.environ setting in the config.

Patch to review:
diff --git a/qutebrowser/config/configdata.yml b/qutebrowser/config/configdata.yml
index 64534276714..5788dc4abfe 100644
--- a/qutebrowser/config/configdata.yml
+++ b/qutebrowser/config/configdata.yml
@@ -156,6 +156,18 @@ qt.args:
     https://peter.sh/experiments/chromium-command-line-switches/ for a list)
     will work.
 
+qt.environ:
+  type:
+    name: Dict
+    keytype: String
+    valtype: String
+    none_ok: true
+  default: {}
+  restart: true
+  desc: >-
+    Additional environment variables to set. Set to `None` to unset an environment
+    variable.
+
 force_software_rendering:
   renamed: qt.force_software_rendering
 
diff --git a/qutebrowser/config/qtargs.py b/qutebrowser/config/qtargs.py
index 2136f7e7f40..dde8e11310e 100644
--- a/qutebrowser/config/qtargs.py
+++ b/qutebrowser/config/qtargs.py
@@ -259,3 +259,10 @@ def init_envvars() -> None:
                    if qtutils.version_check('5.14', compiled=False)
                    else 'QT_AUTO_SCREEN_SCALE_FACTOR')
         os.environ[env_var] = '1'
+
+    for var in config.val.qt.environ:
+        val = config.val.qt.environ[var]
+        if val == 'None':
+            os.environ[var] = ''
+        else:
+            os.environ[var] = val
diff --git a/tests/unit/config/test_qtargs.py b/tests/unit/config/test_qtargs.py
index b050113b4b0..4abf6913928 100644
--- a/tests/unit/config/test_qtargs.py
+++ b/tests/unit/config/test_qtargs.py
@@ -428,6 +428,39 @@ def test_env_vars(self, monkeypatch, config_stub,
 
         assert os.environ[envvar] == expected
 
+    @pytest.mark.parametrize('config_opt, config_val, init_val, envvar, expected', [
+        ('qt.environ', {'QT_SCALE_FACTOR': '2'},
+         [None], ['QT_SCALE_FACTOR'], ['2']),
+        #Test changing an environment variable
+        ('qt.environ', {'QT_SCALE_FACTOR': '4'},
+         ['2'], ['QT_SCALE_FACTOR'], ['4']),
+        ('qt.environ', {'QT_SCALE_FACTOR': '4'},
+         ['unset'], ['QT_SCALE_FACTOR'], ['4']),
+        #Test unsetting an environment variable
+        ('qt.environ', {'QT_SCALE_FACTOR': 'None'},
+         ['3'], ['QT_SCALE_FACTOR'], ['']),
+        #Test setting multiple environment variables
+        ('qt.environ', {'QT_SCALE_FACTOR': '3', 'QT_PLUGIN_PATH': '/tmp/',
+                        'QT_NEWVAR': 'newval'},
+         [None, None, None], ['QT_SCALE_FACTOR', 'QT_PLUGIN_PATH', 'QT_NEWVAR'],
+         ['3', '/tmp/', 'newval']),
+    ])
+    def test_environ_settings(self, monkeypatch, config_stub,
+                              config_opt, config_val, init_val, envvar, expected):
+        """Test extra environment settings."""
+        for (var, val) in zip(envvar, init_val):
+            if val is not None:
+                if val == 'unset':
+                    monkeypatch.delenv(var)
+                else:
+                    monkeypatch.setenv(var, val)
+
+        config_stub.set_obj(config_opt, config_val)
+        qtargs.init_envvars()
+
+        for (var, result) in zip(envvar, expected):
+            assert os.environ[var] == result
+
     @pytest.mark.parametrize('new_qt', [True, False])
     def test_highdpi(self, monkeypatch, config_stub, new_qt):
         """Test HighDPI environment variables.


Code review comments:
diff_hunk: @@ -259,3 +259,10 @@ def init_envvars() -> None:
                    if qtutils.version_check('5.14', compiled=False)
                    else 'QT_AUTO_SCREEN_SCALE_FACTOR')
         os.environ[env_var] = '1'
+
+    for var in config.val.qt.environ:
+        val = config.val.qt.environ[var]
+        if val == 'None':
path: qutebrowser/config/qtargs.py
text: @user1:
This checks against the string "None" instead of checking against the [`None` constant](https://docs.python.org/3/library/constants.html#None) from Python. Instead, this should be something like:

```suggestion
        if val is None:
```

@author:
I did this because when I start qutebrowser using the command

`qutebrowser --temp-basedir -s qt.environ '{"QT_SCALE_FACTOR": None}`

for example I noticed that the literal None is never matched in this if statement however the string 'None' is so I assumed there was some sort of preprocessing that converts the None to 'None' before it got to this point in the code. Is this a problem or am I just misunderstanding how this is supposed to work?

@user1:
I think you'll get the expected behavior via `qutebrowser --temp-basedir -s qt.environ '{"QT_SCALE_FACTOR": null}'` (`null` rather than `None`), because YAML is used to parse config options from text. This certainly should be documented better, though.

diff_hunk: @@ -259,3 +259,10 @@ def init_envvars() -> None:
                    if qtutils.version_check('5.14', compiled=False)
                    else 'QT_AUTO_SCREEN_SCALE_FACTOR')
         os.environ[env_var] = '1'
+
+    for var in config.val.qt.environ:
+        val = config.val.qt.environ[var]
+        if val == 'None':
+            os.environ[var] = ''
path: qutebrowser/config/qtargs.py
text: @user1:
Deleting an environment variable and setting it to an empty string are two different things - depending on the application reading it, they might do something different.

You can delete/unset the variable using [the `del` keyword](https://docs.python.org/3/tutorial/datastructures.html#the-del-statement) (the doc talks about lists, but the same applies to dicts as well):

```suggestion
            del os.environ[var]
```

You'll likely also need to adjust the test accordingly, or maybe even separate the test into two.

diff_hunk: @@ -428,6 +428,39 @@ def test_env_vars(self, monkeypatch, config_stub,
 
         assert os.environ[envvar] == expected
 
+    @pytest.mark.parametrize('config_opt, config_val, init_val, envvar, expected', [
+        ('qt.environ', {'QT_SCALE_FACTOR': '2'},
+         [None], ['QT_SCALE_FACTOR'], ['2']),
+        #Test changing an environment variable
+        ('qt.environ', {'QT_SCALE_FACTOR': '4'},
+         ['2'], ['QT_SCALE_FACTOR'], ['4']),
+        ('qt.environ', {'QT_SCALE_FACTOR': '4'},
+         ['unset'], ['QT_SCALE_FACTOR'], ['4']),
+        #Test unsetting an environment variable
+        ('qt.environ', {'QT_SCALE_FACTOR': 'None'},
+         ['3'], ['QT_SCALE_FACTOR'], ['']),
+        #Test setting multiple environment variables
+        ('qt.environ', {'QT_SCALE_FACTOR': '3', 'QT_PLUGIN_PATH': '/tmp/',
+                        'QT_NEWVAR': 'newval'},
+         [None, None, None], ['QT_SCALE_FACTOR', 'QT_PLUGIN_PATH', 'QT_NEWVAR'],
+         ['3', '/tmp/', 'newval']),
+    ])
+    def test_environ_settings(self, monkeypatch, config_stub,
+                              config_opt, config_val, init_val, envvar, expected):
+        """Test extra environment settings."""
+        for (var, val) in zip(envvar, init_val):
+            if val is not None:
+                if val == 'unset':
+                    monkeypatch.delenv(var)
+                else:
+                    monkeypatch.setenv(var, val)
+
+        config_stub.set_obj(config_opt, config_val)
path: tests/unit/config/test_qtargs.py
text: @user1:
```suggestion
        config_stub.val.qt.environ = config_val
```

You can then also remove it from the parametrization arguments above, since it's always `qt.environ` anyways.

diff_hunk: @@ -428,6 +428,39 @@ def test_env_vars(self, monkeypatch, config_stub,
 
         assert os.environ[envvar] == expected
 
+    @pytest.mark.parametrize('config_opt, config_val, init_val, envvar, expected', [
+        ('qt.environ', {'QT_SCALE_FACTOR': '2'},
+         [None], ['QT_SCALE_FACTOR'], ['2']),
+        #Test changing an environment variable
+        ('qt.environ', {'QT_SCALE_FACTOR': '4'},
+         ['2'], ['QT_SCALE_FACTOR'], ['4']),
+        ('qt.environ', {'QT_SCALE_FACTOR': '4'},
+         ['unset'], ['QT_SCALE_FACTOR'], ['4']),
+        #Test unsetting an environment variable
+        ('qt.environ', {'QT_SCALE_FACTOR': 'None'},
+         ['3'], ['QT_SCALE_FACTOR'], ['']),
+        #Test setting multiple environment variables
+        ('qt.environ', {'QT_SCALE_FACTOR': '3', 'QT_PLUGIN_PATH': '/tmp/',
+                        'QT_NEWVAR': 'newval'},
+         [None, None, None], ['QT_SCALE_FACTOR', 'QT_PLUGIN_PATH', 'QT_NEWVAR'],
+         ['3', '/tmp/', 'newval']),
+    ])
+    def test_environ_settings(self, monkeypatch, config_stub,
+                              config_opt, config_val, init_val, envvar, expected):
+        """Test extra environment settings."""
+        for (var, val) in zip(envvar, init_val):
path: tests/unit/config/test_qtargs.py
text: @user1:
I think it'd be more readable if `init_val` as well as `envvar` and `expected` were dictionaries as well - there's a bit more repetition, but it's clearer how the test works then. One test would then look something like:

```python
(
    {'QT_SCALE_FACTOR': 2},  # config
    {'QT_SCALE_FACTOR': None},  # initial
    {'QT_SCALE_FACTOR': 2},  # expected
)
```

(though maybe also swap config/initial around, so it can be read as three "steps": The initial value, the config being set, and the resulting value.

diff_hunk: @@ -259,3 +259,10 @@ def init_envvars() -> None:
                    if qtutils.version_check('5.14', compiled=False)
                    else 'QT_AUTO_SCREEN_SCALE_FACTOR')
         os.environ[env_var] = '1'
+
+    for var in config.val.qt.environ:
+        val = config.val.qt.environ[var]
path: qutebrowser/config/qtargs.py
text: @user1:
```suggestion
    for var, val in config.val.qt.environ.items():
```

@author:
I originally had written it the way you suggest however then I get the following error when doing the pylint test:

`qutebrowser/config/qtargs.py:263:20: E9998: qt.environ.items is no valid config option. (bad-config-option)`

so I switched it. Should I just ignore the pylint test or do I need to change something else as well?



@user1:
Oh, that's a bug in that test (written by me). Feel free to ignore it and I'll see what I can do to fix it.