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 arguments, ran subprocess without checking the response, 
and failed silently in the case of error.