Merge lp://staging/~erigami/dockmanager/workrave-helper into lp://staging/dockmanager

Please add the "DBusName" key to the info file - by using it, the script will only be run when that particular name is live on the bus (saves memory)

I've added the DBusName key to the info file.

* please add a license header to your code like all other helpers have.
* remove all debugging and dead code. Of course comments are welcome and should stay.
* use "print ..." for important debug outputs.
* for resetting ui make use of self.reset_tooltip(), self.reset_badge(), self.reset_icon(),
  ...

Made fixes requested by Rico.

@Erigami: Thanks for cleaning this up.

I haven't tested it since I don't use Workrave, but I think there a some problems.

What is the reason for these two classes WorkraveWatcher and WorkraveItem? Why not merging them together?

This is most likely generating a leak every time the DockItem appears/changes:
    gobject.timeout_add(5000, self.__poll_workrave)
    gobject.timeout_add(1000, self.__update_ui)
You need to remove these timers if they aren't needed anymore and prevent adding them multiple times.

> What is the reason for these two classes WorkraveWatcher and WorkraveItem? Why
> not merging them together?

The WorkraveItem encapsulates the UI goo for the dock item - ideally I'd just use a DockItem, but it's missing set_message(). The Watcher receives callbacks from timers and updates the UI.

> This is most likely generating a leak every time the DockItem appears/changes:
> gobject.timeout_add(5000, self.__poll_workrave)
> gobject.timeout_add(1000, self.__update_ui)
> You need to remove these timers if they aren't needed anymore and prevent
> adding them multiple times.

I don't think they cause a memory leak, since on_service_change() is called once on startup. If there are any changes in the Workrave interface, then DockManager should stop the helper, right?

Is there something that I'm missing?

Looks good to me.

> > This is most likely generating a leak every time the DockItem
> appears/changes:
> > gobject.timeout_add(5000, self.__poll_workrave)
> > gobject.timeout_add(1000, self.__update_ui)
> > You need to remove these timers if they aren't needed anymore and prevent
> > adding them multiple times.
>
> I don't think they cause a memory leak, since on_service_change() is called
> once on startup. If there are any changes in the Workrave interface, then
> DockManager should stop the helper, right?
>
> Is there something that I'm missing?

Docky doesn't kill the helpers when their DBusName disappears. Rico probably meant a race if Workrave disappears from the bus and reappears in less than 1 second (or 5 in the other callback). You should remove the timers using "glib.source_remove(timer_id)".

> > I don't think they cause a memory leak, since on_service_change() is called
> > once on startup. If there are any changes in the Workrave interface, then
> > DockManager should stop the helper, right?
...
> Docky doesn't kill the helpers when their DBusName disappears. Rico probably
> meant a race if Workrave disappears from the bus and reappears in less than 1
> second (or 5 in the other callback). You should remove the timers using
> "glib.source_remove(timer_id)".

When I run DockManager from the console (and it's talking to AWN), it emits a message on the console telling me that the process has been killed.

The callbacks also return False when the connected flag is set to false, so they deregister themselves on the poll after the Workrave object disappears from dbus.

The point is that not every implementation that uses DockManager (read: Docky) makes use of the daemon and behaves exactly the same.

So to be super safe, you need to assume that some implementations will not kill/restart the script and thus your code will wind up leaking.

Take a look at other helpers that use timers to see how to properly remove the stale timers.

I'll take Michal's suggestion and explicitly deregister timers with glib.source_remove().

Is there any doc on the lifecycle I should expect a helper to have? I've run against Docky and AWN/DockManager, but it sounds like there's more going on than I've seen.

Eventually, everything will most likely use the DockManager daemon. The reason Docky does not use it (currently) is because this all started with Docky's helpers. We then decided to generalize it so it could be shared with AWN (and anything else that wants to use it). So AWN's implementation has from the start used the daemon while Docky's is using its original, custom implementation.

So you are already aware of everything. From a program perspective just consider your helper as a daemon that runs all the time. If your code works properly for that, it will work in every case.

Thanks Robert.

The timers are now cleared explicitly.

Bump.

@Erigami: If you gave it a spin with docky then I think we are good here.

> @Erigami: If you gave it a spin with docky then I think we are good here.

Will do. Thanks.

I gave it a go with docky, and it works as expected.