Merge lp://staging/~autopilot/autopilot/ap-qt-vis into lp://staging/autopilot

Proposed by Christopher Lee
Status: Merged
Approved by: Thomi Richards
Approved revision: 68
Merged at revision: 54
Proposed branch: lp://staging/~autopilot/autopilot/ap-qt-vis
Merge into: lp://staging/autopilot
Diff against target: 424 lines (+342/-31)
4 files modified
autopilot/vis/__init__.py (+22/-0)
autopilot/vis/bus_enumerator.py (+91/-0)
autopilot/vis/main_window.py (+218/-0)
bin/autopilot (+11/-31)
To merge this branch: bzr merge lp://staging/~autopilot/autopilot/ap-qt-vis
Reviewer Review Type Date Requested Status
Thomi Richards (community) Approve
jenkins (community) continuous-integration Approve
Review via email: mp+120045@code.staging.launchpad.net

Commit message

Implementation of the new visualisation tool.

Description of the change

Implemented the new visualisation tool for inspecting introspectable objects.
It is a Qt GUI app that queries all available connections (that adhere to the autopilot protocol) and allows a develop to drill-down into the objects details.

To post a comment you must log in.
Revision history for this message
jenkins (martin-mrazik+qa) wrote :
review: Approve (continuous-integration)
Revision history for this message
Thomi Richards (thomir-deactivatedaccount) wrote :

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!

review: Needs Fixing
68. By Christopher Lee

Code cleanup and changes as per MP comment

Revision history for this message
jenkins (martin-mrazik+qa) wrote :
review: Approve (continuous-integration)
Revision history for this message
Thomi Richards (thomir-deactivatedaccount) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches