Thank you for yet another nice branch nailing yet another nail in the dispatcher coffin! I'll provide some specific comments about the code inline, though I'll ignore all the nice deletions from dispatcher.py. I think you're only hope for a *unit* test is to mock .launcher and check that the right calls have been made when update_unread_count() is called. That should be an easy test to add, and I think you should do it. What you have in the __main__ is essentially an integration test. One way to add a test for this is to add a dbus call to the TestService in gwibber/testing/service.py which would take an integer and turn around and call update_unread_count() with that value. The problem is, I can't think of a good way to verify that the test is working correctly. Is it possible to query the menus to see if they've been changed, and if so in what way? We really don't want an automated test that requires any user interaction, and without the ability to query the menus, it seems like that's all we're left with. Maybe ask on the ubuntu-devel mailing list and see how other folks who are writing similar dbusmenu applications do their testing. I just don't know enough about this stuff to give good suggestions. The only other suggestion I can give is to provide a README with instructions for how to verify this is working to a Q/A person, or another developer. Let's look at the code! I'll remove the diff hunks that look fine. === modified file 'gwibber/gwibber/main.py' --- gwibber/gwibber/main.py 2012-08-21 22:42:24 +0000 +++ gwibber/gwibber/main.py 2012-08-30 20:10:23 +0000 > @@ -46,6 +47,16 @@ > DEFAULT_SQLITE_FILE = os.path.join( > BaseDirectory.xdg_config_home, 'gwibber', 'gwibber.sqlite') > > +def refresh_callback(*ignore): > + # TODO: Once the new Dispatcher is born, this method should trigger > + # it to do a refresh. > + pass > + > +def shutdown_callback(*ignore): > + # TODO: Once the new Dispatcher is born, this method should trigger > + # it to shutdown all of gwibber-*. > + pass > + Do you expect these methods to move, or will they live permanently in main.py? > > def main(): > global log === added file 'gwibber/gwibber/utils/menus.py' --- gwibber/gwibber/utils/menus.py 1970-01-01 00:00:00 +0000 +++ gwibber/gwibber/utils/menus.py 2012-08-30 20:10:23 +0000 > @@ -0,0 +1,97 @@ > +# Copyright (C) 2012 Canonical Ltd > +# > +# This program is free software: you can redistribute it and/or modify > +# it under the terms of the GNU General Public License version 2 as > +# published by the Free Software Foundation. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see . > + > +"""This file defines the Dbusmenu behavior used in Unity.""" > + > +import subprocess > + > +from gettext import gettext as _ > + > +try: > + from gi.repository import Unity, Dbusmenu > +except: > + Unity = None > + Dbusmenu = None > + > +try: > + from gi.repository import MessagingMenu > +except: > + MessagingMenu = None > + > +class MenuManager: > + messaging = None > + launcher = None > + > + def __init__(self, refresh_callback, shutdown_callback): > + self.refresh = refresh_callback > + self.shutdown = shutdown_callback Are these attributes useful (even potentially) outside of the class? If not, it's probably better to prefix them with a single underscore. > + > + if MessagingMenu: > + self.init_messaging_menu() > + if Unity and Dbusmenu: > + self.init_dbus_menu() > + > + def init_messaging_menu(self): > + self.messaging = MessagingMenu.App(desktop_id='gwibber.desktop') > + self.messaging.register() > + > + def init_dbus_menu(self): > + self.launcher = Unity.LauncherEntry.get_for_desktop_id('gwibber.desktop') > + ql = Dbusmenu.Menuitem.new() > + > + post_menu = Dbusmenu.Menuitem.new() > + post_menu.property_set(Dbusmenu.MENUITEM_PROP_LABEL, _('Update Status')) > + post_menu.connect('item-activated', lambda *ignore: > + subprocess.Popen('gwibber-poster', shell=False)) lambda like this are pretty nasty actually. Also, I wonder about connecting this menu item to the Popen object resulting from this. The subprocess may not get called properly, or the child may not get properly waited on. What probably makes sense is to move this into a method which calls subprocess.check_output() to ensure the subcommand is run. It'll have to catch CalledProcessError in case the subprocess fails, and everything should be logged (i.e. any output and any non-zero return code). For example, what happens if gwibber-poster or gwibber-preferences doesn't exist on the system? We definitely don't want these menu items failing silently, but there's probably not much other than logging you can do. > + > + refresh_menu = Dbusmenu.Menuitem.new() > + refresh_menu.property_set(Dbusmenu.MENUITEM_PROP_LABEL, _('Refresh')) > + refresh_menu.connect('item-activated', self.refresh) > + > + preferences_menu = Dbusmenu.Menuitem.new() > + preferences_menu.property_set(Dbusmenu.MENUITEM_PROP_LABEL, _('Preferences')) > + preferences_menu.connect('item-activated', lambda *ignore: > + subprocess.Popen('gwibber-preferences', shell=False)) > + > + quit_menu = Dbusmenu.Menuitem.new() > + quit_menu.property_set(Dbusmenu.MENUITEM_PROP_LABEL, _('Quit')) > + quit_menu.connect('item-activated', self.shutdown) > + > + for menu in (post_menu, refresh_menu, preferences_menu, quit_menu): > + menu.property_set_bool(Dbusmenu.MENUITEM_PROP_VISIBLE, True) > + ql.child_append(menu) > + > + self.launcher.set_property('quicklist', ql) > + self.update_unread_count(0) > + > + def update_unread_count(self, count): > + if self.launcher: > + self.launcher.set_property('count', count) > + self.launcher.set_property('count_visible', bool(count)) > + > +# XXX This bit allows you to test this file by running it. This doesn't > +# fit very well into the larger testsuite architecture so we could > +# probably improve this somehow, but I'm not sure how. > +if __name__ == '__main__': > + def stub(*ignore): > + pass > + > + from gi.repository import GObject > + loop = GObject.MainLoop() > + menu = MenuManager(stub, stub) > + > + # If you see the number 20 on your Gwibber icon, > + # then basically the test has succeeded. > + menu.update_unread_count(20) > + loop.run()