Merge lp:~robru/gwibber/dbusmenu into lp:~barry/gwibber/py3

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 <http://www.gnu.org/licenses/>.
> +
> +"""This file defines the Dbusmenu behavior used in Unity."""
> +
> +import subprocess
> +
> +from gettext import gettext as _
> +
> +try:
> + from gi.repository import Unity, D...

On 12-08-30 04:05 PM, Barry Warsaw wrote:>> +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?

I knew you wouldn't like these ;-)

They are not expected to do anything. I expect to delete them once we
have a working Dispatcher, and then we can just pass the appropriate
dispatcher methods directly into the MenuManager constructor. The only
reason that I did two separate ones instead of passing in the same one
twice, was that I thought that perhaps the shutdown callback may not
even need the dispatcher at all, if our dispatcher is simple enough.
perhaps shutdown_callback could be replaced with a lambda that just
calls mainloop.quit() or something. I'm not really sure, I was hoping
you'd be able to integrate this better.

 >> + 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.

They won't be, because anything that needs them will have equal access
to them from the place that they're passed in from (eg, if something
needs them, they can get it directly from main.py rather than from
MenuManager).

 >> + 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.

I hate to say this barry, but I'm not very familiar with either
subprocess, OR with the logger, but you seem to have strong opinions
about both. Is there any way that you could clean this part up to your
standards?

I agree that silent lambdas launching processes and ignoring output is
ugly, but I just don't know any other way. In my defence, this is almost
identical to what it was doing before, except instead of lambdas it
wasted a bunch of space defining instance methods, that identically
ignored their argument...

On Aug 30, 2012, at 09:28 PM, Robert Bruce Park wrote:

>I hate to say this barry, but I'm not very familiar with either
>subprocess, OR with the logger, but you seem to have strong opinions
>about both. Is there any way that you could clean this part up to your
>standards?

No worries, I'll clean this up when I merge the branch.

-Barry