[tor-dev] Code Review (arm) - Initial commit for the menu code

[Damian Johnson <atagar1@xxxxxxxxx>]

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/5fd9b5f74a86eb7e641366ae1c0a1a47404cf873

> 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...).
_______________________________________________
tor-dev mailing list
tor-dev@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-dev