Fixed #37: Prevent crash setting up dbus without a notification target

[agg23]

Let's try this again.

Fixed #37: Prevent crash setting up dbus without a notification target

[ValentijnvdBeek]

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)

[agg23]

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.

[ValentijnvdBeek]

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 _*registerBusName functions 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:

• Python uses the camel_case convention
• Unused variables (_registerBusName) should be named _
• If at all possible avoid bare exception[1].
• Variables should be initialized in __init__
• Functions ideally should have docstrings[1]
• 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.

[agg23]

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

[ValentijnvdBeek]

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.

[agg23]

Done

[ValentijnvdBeek]

You forgot to re-add the BASE_NAME to watch_name, that is it.

[agg23]

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?

[ValentijnvdBeek]

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.