Code review comment for lp://staging/~autopilot/autopilot/ap-qt-vis

Hi,

6 +import sys
7 +import dbus
8 +from dbus.mainloop.qt import DBusQtMainLoop
9 +from autopilot.vis.bus_enumerator import BusEnumerator
10 +from autopilot.vis.main_window import MainWindow
11 +from PyQt4 import QtGui

Please rearrange these imports - they should be like this:

import dbus
from dbus.mainloop.qt import DBusQtMainLoop
from PyQt4 import QtGui
import sys

from autopilot.vis.bus_enumerator import BusEnumerator
from autopilot.vis.main_window import MainWindow

i.e.- imports should be system first, then local, and should be ordered alphabetically. Please also fix the imports at the top of bus_enumerator.py (yes, I realise that's my code :))

142 + AP_DBUS_IFACE_STR = "com.canonical.Autopilot.Introspection"

I think this is already defined in autopilot.introspection.dbus. If it's not, it should be defined there (it's certainly used there). Let's define this in one palce (in that module) and import & use it here.

179 + # print "Updating interface list: %s, %s, %s" % (conn, obj, iface)

Please remove.

180 + dbus_object = session_bus.get_object(str(conn), str(obj))
181 + dbus_iface = dbus.Interface(dbus_object,
182 + 'com.canonical.Autopilot.Introspection')
183 + cls_name, cls_state = dbus_iface.GetState("/")[0]

These should be wrapped inside a try: ... except dbus.DBusException block. If the interface dissapears before you get to it this will throw. This will also throw if the app doesn't send a reply, or if a bunch of other things go wrong.

210 + """itemData will return a tuple with (obj, iface) details pair."""

This should be a comment, not a docstring for the method.

221 + object_details = dict(filter(lambda i: i[0] != "Children",
222 + object_details.iteritems()))

A nicer way to write this would be:

object_detail.pop("Children", None)

or maybe:

if "Children" in object_details:
    del object_details["Children"]

225 + if key == "Children":
226 + continue

I don't understand why you're doing this, given the code above...

242 + import dbus

Please remove - you're already importing this at the top of the file.

256 + if isinstance(dbus_type, dbus.Array):
257 + return ', '.join([dbus_string_rep(i) for i in dbus_type])

Please make this work for dbus.Struct as well. That will make it behave nicely with QSize, QRect, QPoint, and a few other types that Qt apps export.

Other than those issues, looks great!