Closed agg23 closed 5 years ago
Hi Adam,
First of all thanks a lot for porting your fixes and refiling the merge request, your help is very much appreciated.
Next is the procedure, I don't really see why there are two separate commits instead of a single commit. The second commit is too small and the first commit can't function on it's own. If you could rebase your commit to squash them together then that would be great.
Lastly the technical details of the fix. It works well as a static fix when you're starting up Clay but it doesn't take in account that the notification daemon might crash during usage (e.x. closing your wm with clay running in a multiplexer) or that a notification starts up again later. Hence utilizing the watch_name dbus functionality instead might be a better way of doing things (pydbus doc link & gnome developer doc link)
I agree entirely with your comments. You're the first person I've met who actually cares more about commit structure than I do.
I don't know much about Dbus, but I believe I got the detection working successfully. I sheepishly admit that I could not come up with some way to test the closing functionality however, so there may still be issues.
Hi Adam,
The commit history is perfect now.
I took a look at code and there are a few things that require some improvement. For instance in the notify function you check whether the notification
attribute exists, however `*registerBusNamefunctions guarantees that it is either the channel or None. This will do nothing and make Clay crash whenever there isn't a notification server. Replacing it with an
if self.notifications is None` will fix this.
It crashes if notification server exists when it starts but exits when it closes, wrapping the self.notifications.Notify
in a exception catching GLib.Error
will probably fix this.
If the notification server doesn't exist when you start it but gets created later it will not send notifications.
I don't think that adding BASE_NAME is needed in the watch_name
function.
There are also a few style issues:
_registerBusName
) should be named _
__init__
watch_name
doesn't follow the visual indent.I would recommend that you install a linter and add it to your text editor. It really helps a lot guarding you against these issues.
EDIT: To test it, I just SIGKILL my notification server and start it up again. Here is a list of standalone servers you can use. [1] Clay (sadly) doesn't always follow this convention so I am not going to hold it against you for not doing it.
I'm fairly certain BASE_NAME
is indeed required, given the exception that is logged otherwise:
(process:11764): GLib-GIO-CRITICAL **: 08:42:11.515: g_bus_watch_name_on_connection: assertion 'g_dbus_is_name (name)' failed
The docstring for watch_name
does not have the same admissions as per bus names starting with "." as get
does, so I'm pretty sure it's safe to assume it does not have that functionality.
All of the linting stuff should be fixed, so let me know if you have further comments
I'm fairly certain BASE_NAME is indeed required, given the exception that is logged otherwise:
I just rechecked it again and you're right. The reason I thought it wasn't required was because it seemed to work fine on most of the tests that I ran, turns out the only time it doesn't work that way is when you don't have notification server started and then start one. So my bad, sorry.
The style stuff seems to be perfectly fine. The last two things minor nitpicky things are that you should probably log the exception in _register_bus_name
and put a small (unused)
next to name_owner
in the docstring.
So fix those three things and I'll merge it.
Again, thanks for your contribution.
Done
Man, can't catch break :P
I must say, I'm not liking how poorly editors seem to calculate type information (though I am quite unused to Python in general). What does your environment look like?
Making mistakes is only human after all right?
Also, I feel like discussing my text editor configuration is a bit too off-topic for this pull request. If you want to talk about that you're better off sending a message on the IRC channel.
Let's try this again.
Fixed #37: Prevent crash setting up dbus without a notification target