Code review comment for ~sylvain-pineau/checkbox-ng:urwid_manifest

Revision history for this message
Maciej Kisielewski (kissiel) wrote :

URWID review:

The new urwid screen is very difficult to use.
It took me solid one minute to figure out you have to use left/right arrows and a space-bar.
For a screen with one manifest option, there's no cursor movement, giving an impression that the program is frozen.
The '-' in front of the manifest option is misleading (on other screens it means unfolded tree).
I would prefer radio buttons to be on the left of the screen (or anywhere nearby to the label) - on panoramic screens it's easy to miss (happened to me the first time I tried it).
I think tooltip in the footer is necessary to guide the operator.
I would like a shortcut to select options (like y/n), so I could quickly go through the list.
I don't get why do we need two radio buttons for boolean variables. Why not use checkboxes as we do on other screens? Simpler code, simpler UX, etc.

Remote review:
- branch is not based on a fresh master, so testing remote as-is can by crashy, but I found that it can be rebased on master cleanly
- there's no distinct interaction step on the manifest screen, breaking the remote connection on that screen rolls the session back to test selection screen (just noticing, I'm not bothered by it)
- new RemoteSA method call means API bump is necessary

Code review:
Pleasant read. Some codes below.

review: Needs Fixing

« Back to merge proposal