Code Review: or-applet - tor-dev

27 May 2014


      Hi Yawning, I just took a quick peek at your or-applet
(https://github.com/Yawning/or-applet). Nice work! Added it to Stem's
examples page.
Just a few thoughts. Nothing very substantial...
...
def _format_status(status):
    if status == CircStatus.LAUNCHED:
        return 'LAUNCHED (circuit ID assigned to new circuit)'
    elif status == CircStatus.BUILT:
        return 'BUILT (all hops finished, can now accept streams)'
    elif status == CircStatus.EXTENDED:
        return 'EXTENDED (one more hop has been completed)'
    elif status == CircStatus.FAILED:
        return 'FAILED (circuit closed (was not built))'
    elif status == CircStatus.CLOSED:
        return 'CLOSED (circuit closed (was built))'
    return str(status)
Very minor thing but I'd probably opt for the following...
FORMAT_STATUSES = {
    CircStatus.LAUNCHED: 'LAUNCHED (circuit ID assigned to new circuit)',
    CircStatus.BUILT: 'BUILT (all hops finished, can now accept streams)',
    CircStatus.EXTENDED: 'EXTENDED (one more hop has been completed)',
    CircStatus.FAILED: 'FAILED (circuit closed (was not built))',
    CircStatus.CLOSED: 'CLOSED (circuit closed (was built))',
  }
def _format_status(status):
    return FORMAT_STATUSES.get(status, str(status))
Ditto for the rest of the status_icon.py functions.
...
def _build_dynamic_menu(self):
    circuits = self._ctl.get_circuits()
    if circuits == None:
        item = Gtk.MenuItem('No circuits established')
        item.set_sensitive(False)
        self._menu.append(item)
        return
    streams = self._ctl.get_streams()
The get_circuits() method will never return None as you're using it
here. It'll either return a list or raise a stem.ControllerError. I
think what you actually want is...
circuits = self._ctl.get_circuits(None)
That way it'll return None rather than raise an exception if it fails.
...
our_streams.append('[' + stream.id + ']: ' + stream.target)
In python string concatenation is discouraged unless it gives you
readability benefits. It's slightly more performant to use format() or
the following...
our_streams.append('[%s]: %s' % (stream.id, stream.target))
That said, really doesn't matter. :)
...
if len(our_streams) == 0:
  ...
The boolean value of an empty list is False, so this is usually done as...
if our_streams:
    ...
...
if self._control == None:
Minor thing but None comparison is the one time it's suggested that
you use the 'is' keyword (ie, 'if self._control is None:').
...
class PopupMenu:
It's generally a good idea to always extend object...
class PopupMenu(object):
...
def is_newnym_available(self):
    if self._control == None:
        return False
    return self._control.is_newnym_available()
Hmmm, a goal I had for stem Controller objects was that they could be
gracefully disconnected and reconnected. The Controller's
is_newnym_available() already checks is_alive() and returns False if
you're disconnected from tor.
Ideally self._control should never be None. However, if you've never
had an *initial* tor connection then I'm not sure that it's possible
to instantiate a Controller. If so then that's a stem issue I'll need
to think about...
Cheers! -Damian