Hi all. Kamran and I are gonna doing code reviews publicly so if you're interested and spot issues we miss then feel free to chime in. -Damian

Change: https://gitweb.torproject.org/user/krkhan/arm.git/commitdiff/5fd9b5f74a86eb7...

ARM_CONTROLLER = Controller(stdscr, stickyPanels, pagePanels, menu)

What sort of persistent state will the menu require? If we can make it a popup function (like "showHelpPopup()") that would be preferable (less parameter juggling). But if you expect it to remember something between menu requests then this makes sense.

menu.draw()

Maybe rename this to "showMenu()"? The draw method is part of panel which threw me off a bit.

self._rootEntry = (entry and isinstance(entry, ParentEntry)) and entry or DEFAULT_ROOT

Interesting pattern, but it's kinda hard to read (or make sense of unless you've seen it before). Maybe something a little less compact would be clearer?

if entry and isinstance(entry, ParentEntry): self._rootEntry = entry else: self._rootEntry = DEFAULT_ROOT

titles = map(attrgetter('title'), self._rootEntry.children)

This took me a couple minutes with the interactive interpretor to make sense of too. Maybe list comprehension instead of the attrgetter would help?

titles = [menuItem.title for menuItem in self._rootEntry.children]

titlewidth = max(map(lambda title: len(title), titles)) + 2

Neat. Honestly I'm greener than I should be with lamdas. The pattern I'd use for this is... titlewidth = max([len(entry) for entry in titles]) + 2

but up to you. The later is more familiar to me but I'm not sure which can claim the best readability.

# total number of titles that can be printed in current width

Why are you giving all titles equal distance? Does this make it look better? I'd assume that would be best to have dynamic widths, leaving some entries unprinted if we run out of room (this is done all around so I can get you an example if you'd like).

I'm thrilled you're taking variable screen sizes into account!

titleformat = curses.A_NORMAL

if index == self._selection[0]: titleformat = curses.A_STANDOUT

Alternative option...

titleformat = curses.A_STANDOUT if index == self._selection[0] else curses.A_NORMAL

popup.win.addch(top, left, curses.ACS_VLINE) popup.win.addstr(top, left, entry.title.center(titlewidth), titleformat) popup.win.addch(top, left, curses.ACS_VLINE)

Noooooooo! Don't do this!

If you can help it _never_ draw to the raw curses window. If this ever attempts to draw outside the curses window it will crash. The panel class has safe proxies for everything that you should need. In this case...

popup.addch(top, left, curses.ACS_VLINE) popup.addstr(top, left, entry.title.center(titlewidth), titleformat) popup.addch(top, left, curses.ACS_VLINE)

will do the same. There's, of course, exceptions like popup.win.box() and popup.win.refresh() (those are safe).

DEFAULT_ROOT = ParentEntry(title="Root", children=(

I've been defining constants outside of classes so they can be easier to spot at the top of files. However, this is just a stylistic preference of mine - if you prefer constants to be a class attribute then that's fine.

for index in self._selection: if isinstance(entry, ParentEntry): entry = entry.children[index] else: break

if isinstance(entry, LeafEntry): entry.callback(entry)

It looks like if self._selection is an invalid path you do nothing? Maybe we should log something or raise an exception...

Your usage of named tuples and index lists will, I think, make this harder and more confusing than it needs to be. Instead I'd suggest a MenuItem class that has...

getLabel() -> text for the option isLeaf() -> boolean for if we're a leaf or not getSubmenu() -> returns a list of MenuItems below it - either an empty list or None if a leaf node (whatever makes most sense to you) getParent() -> parent MenuItem next(), prev() -> returns a MenuItem instance for the next or previous option for the menu list we're in select() -> event handler

Constructing the menu will be a little more interesting, but rendering it, navigation, and selection will all become trivial. It'll also avoid the situation where _selection is out of sync with _rootEntry (for that matter, we wouldn't need a rootEntry...).