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