Reloading config

[m-col]

EDIT: I appropriated this PR for general config-reloading changes. Previously it was limited to the popup commit but as these changes are a group (and would conflict when done separately) I've kept them together

Currently send_notification will try to send a notification but fail
quietly if there is no notification server running. To get the message
to the user's eyes, it can instead fall back to a transient Popup
window.

This also changes the config-reading code to use this mechanism to
inform the user of a config error, rather than creating a new Config
object just so we can stick a TextBox in it to notify them.

[ramnes]

LGTM

[m-col]

Just had a popup not go away so converting to draft until it gets resolved.

[m-col]

There was no bug, I was just being a bit dim and didn't notice the event loop warning printed to the log. Basically if the event loop isn't running yet then a timer cannot be set to hide the window, so in some instances you need to click it to hide it.

Anyway I rebased and tidied a couple things, so I've un-drafted.

[ramnes]

Maybe worth a test or two?

[m-col]

I added a test for send_popup. It would be nice to also add a test in the future when trying to restart with a faulty config, but for that proper restart handling in the tests is required #2348

[m-col]

I wasn't completely happy with the way I'd done this so I've re-written it and I think it's better. Two commits: one just adding send_popup, and not adding it to send_notification, and one using these two functions to notify the user upon config errors, instead of the current approach of overwriting the current qtile.config with the default and inserting a TextBox into its bar.

By not having send_popup used by send_notification, one can easily choose to never see popup windows (unless they get config errors).

There is one unresolved issue. We need to check that send_notification fails before falling back to send_popup, but in the case that dbus_next is available and the event loop is running, we can't know if the notification will fail due to there being no server because of the async _notify approach. Is it possible to check the presence of a server, or instead notify synchronously? cc @elParaguayo

[elParaguayo]

This is ugly and untested but something like this?

diff --git a/libqtile/utils.py b/libqtile/utils.py
index 2d6c7003..661583cd 100644
--- a/libqtile/utils.py
+++ b/libqtile/utils.py
@@ -23,6 +23,7 @@ import asyncio
 import glob
 import importlib
 import os
+import time
 import traceback
 from collections import defaultdict
 from collections.abc import Sequence
@@ -212,7 +213,11 @@ def send_notification(title, message, urgent=False, timeout=10000, id=None):
         logger.warning("Eventloop has not started. Cannot send notification.")
         return -1
     else:
-        loop.create_task(_notify(title, message, urgency, timeout, id))
+        task = loop.create_task(_notify(title, message, urgency, timeout, id))
+        while not task.done():
+            time.sleep(0.1)
+
+        return task.result()

     return id

@@ -239,11 +244,14 @@ async def _notify(title, message, urgency, timeout, id):
     if msg.message_type == MessageType.ERROR:
         logger.warning("Unable to send notification. "
                        "Is a notification server running?")
+        id = -1

     # a new bus connection is made each time a notification is sent so
     # we disconnect when the notification is done
     bus.disconnect()

+    return id
+

It's ugly as hell, though.

[m-col]

That is pretty ugly ;) With the old gi version of the send_notification, we could do this:

gi.require_version("Notify", "0.7")
from gi.repository import Notify
Notify.init("Qtile")
info = Notify.get_server_info()

and info would us information about any running notification server. This is great for knowing whether to continue with sending it. I'm slowly looking around dbus_next to see how to do the equivalent and then see where/if it makes sense to go, but any pointers would be appreciated as I've not done any dbus work.

[elParaguayo]

You can still use dbus-next to get the server info, but it would be done asynchronously. Is that really a problem though? You can check the status/try to send a notification and attach a callback to send the popup if the server's not available.

EDIT: I appreciate it's not as neat as just sending the popup if the return code is less than zero...

[m-col]

Looks like I could use utils._send_dbus_message which is actually ideal. Just gotta figure out all these weird arguments :)

[elParaguayo]

You've lost me. What can you do by using that function directly (which is still asynchronous) that you can't do by trying to send the notification and testing for success there?

[m-col]

Not a big difference, just that send_notification creates a task and returns, whereas I can await _send_dbus_message and see the result directly so that I don't have to do a polling loop until the task is done :) I don't want to change the behaviour of send_notification, I just want to check it works to know whether to popup or not.

[elParaguayo]

OK - but you can only await from an async function so it still sounds like you need some extra steps.

That said, you know what you're doing so I'll stop replying unless you actually need my help!

[m-col]

I've resolved all of the groups problems, which ultimately were odd side-effects of the way groups were being persisted across reloads. I don't think it makes sense to persist groups across reloads the way they persist across restarts, as the implication of this PR is that cmd_reload_config is used for reloading the config and cmd_restart is used for updating the runtime libqtile following package/code update. Windows that are on groups that are removed are moved to the new current group so it is very clear for the user that they may need to place it to whatever group they want.

If people agree with that approach (groups are refreshed completely from the config when you reload that config), then this PR is ready to go!

[elParaguayo]

As per our chat, here's a diff to solve the delay when waiting to see if there's an available notification server:

diff --git a/libqtile/utils.py b/libqtile/utils.py
index 0c3203b4..34ff8020 100644
--- a/libqtile/utils.py
+++ b/libqtile/utils.py
@@ -264,6 +264,11 @@ def send_notification(title, message, urgent=False, timeout=10000, id=None, on_f

 async def _notify(title, message, urgency, timeout, id) -> bool:
     """Returns True upon success, False upon failure."""
+    can_notify = await _has_notification_server()
+
+    if not can_notify:
+        return False
+
     notification = ["qtile",  # Application name
                     id,  # id
                     "",  # icon
@@ -292,6 +297,27 @@ async def _notify(title, message, urgency, timeout, id) -> bool:
     return msg.message_type != MessageType.ERROR


+async def _has_notification_server():
+    """Checks for the existence of 'org.freedesktop.Notifications'."""
+    bus, msg = await _send_dbus_message(True,
+                                        MessageType.METHOD_CALL,
+                                        "org.freedesktop.DBus",
+                                        "org.freedesktop.DBus",
+                                        "/org/freedesktop/DBus",
+                                        "ListNames",
+                                        "",
+                                        [])
+
+    bus.disconnect()
+
+    if msg.message_type != MessageType.METHOD_RETURN:
+        return False
+
+    names = msg.body[0]
+
+    return "org.freedesktop.Notifications" in names
+
+
 def guess_terminal(preference=None):
     """Try to guess terminal."""
     test_terminals = []

[m-col]

Brilliant, thank you for that, I've folded it in.

[m-col]

OK, I've stripped out the popup-related stuff, so if users want feedback about config errors then they'll need a notification server running. Seems like a reasonable expectation.

[elParaguayo]

Nice. I'll see if I can test at some point this weekend. Would be great to get this one merged at last.

[elParaguayo]

Only one comment from me: When I put a bug in my config and restart, the notification I get (via Notify) says "Configuration Error - None".

This is because you're using str(error.__context__) for the notification content. __context__ seems to be empty so you may want to use str(error) instead.

[elParaguayo]

Hmm. Still some bugs here.
reload_config didn't prevent a reload with a bad config (just lost my bar contents) and I ended up with 2 of the same scratchpad on top of eachother.

The log shows the Preventing restart because of a configuration error: invalid syntax (config.py, line 311) mesage but it's followed by a TypeError: initializer for ctype 'PangoLayout *' must be a cdata pointer, not NoneType message.

[m-col]

reload_config didn't prevent a reload with a bad config (just lost my bar contents) and I ended up with 2 of the same scratchpad on top of eachother.

I was finalising resources before reloading the config and only checking when reloading, so if it didn't reload (due to a bad config) then the resources were still finalised. Fixed this now by just validating the config before doing anything.

Also rebased.

[elParaguayo]

Awesome work on this one. Thanks.