Environment list/selection view.

Environment list/selection view.

This branch implements the environment
selection Urwid view. It is possible to
start it running `make` and then
`./cli-app-demo.py`. This is still not
integrated in quickstart, but the demo
file demonstrates how to show the view
passing the required data structures
already implemented in the 
quickstart.models.envs module.

To test the branch, run `make check`.

To QA the new functionality, run the
demo application as described above, 
and play with the interface: any UX
suggestion is welcome, and will be part
of a card we already have 
("envs management: ux improvements").

Other changes:

The Urwid application is no longer an 
ObjectDict (removed from the code).
I decided to use a Python namedtuple
which seemed to me a better choice,
considering that views are not supposed 
to modify the app data structure.
This way we have both immutability and
attribute access to the object.

Implemented a TimeoutText wrapper used to
wrap the message notification widget.
This wayt he timer is restarted when a 
subsequent message is set and the previous 
has not yet been removed.

https://code.launchpad.net/~frankban/juju-quickstart/env-manage-selection-view/+merge/199484

(do not edit description out of merge proposal)

[frankban, 2013-12-18 15:43:05]

Please take a look.

[rharding, 2013-12-18 16:44:58]

Code looks good with a couple of readable enhancing notes. The UI code is a bit
dense as someone that's not looked at it before so will trust in QA.

https://codereview.appspot.com/43860044/diff/1/quickstart/cli/base.py
File quickstart/cli/base.py (right):

https://codereview.appspot.com/43860044/diff/1/quickstart/cli/base.py#newcode37
quickstart/cli/base.py:37: _Application = namedtuple(
why the _? Why call it 'App' but also call it _Application? In my experience I'd
just have App = namedtuple('App'...

https://codereview.appspot.com/43860044/diff/1/quickstart/cli/base.py#newcode152
quickstart/cli/base.py:152: # place where to add new API functions, after
changing the application
Create the App named tuple. If, in the future, we have a view that requires
additional capabilities or API access, this is the place to add those.

https://codereview.appspot.com/43860044/diff/1/quickstart/cli/ui.py
File quickstart/cli/ui.py (right):

https://codereview.appspot.com/43860044/diff/1/quickstart/cli/ui.py#newcode49
quickstart/cli/ui.py:49: None: 'selected',
I don't follow this. When None is in focus it's selected?

https://codereview.appspot.com/43860044/diff/1/quickstart/cli/views.py
File quickstart/cli/views.py (right):

https://codereview.appspot.com/43860044/diff/1/quickstart/cli/views.py#newcode32
quickstart/cli/views.py:32: A view is a callable receiving an app object (a
named tuple of functions) and
Can we capitalize the App here and in cases referencing the specific object?

[gary.poster, 2013-12-18 16:57:15]

LGTM with mostly trivial comments (discussion on possible decoys is the only
thing that ended up being interesting).

https://codereview.appspot.com/43860044/diff/1/quickstart/cli/base.py
File quickstart/cli/base.py (right):

https://codereview.appspot.com/43860044/diff/1/quickstart/cli/base.py#newcode37
quickstart/cli/base.py:37: _Application = namedtuple(
Nice improvement.

https://codereview.appspot.com/43860044/diff/1/quickstart/cli/views.py
File quickstart/cli/views.py (right):

https://codereview.appspot.com/43860044/diff/1/quickstart/cli/views.py#newcod...
quickstart/cli/views.py:140: detail_view = functools.partial(
nice :-)

I toyed with suggesting that you pass env_index to env_detail, and vice versa,
but naah for now.

https://codereview.appspot.com/43860044/diff/1/quickstart/cli/views.py#newcod...
quickstart/cli/views.py:144: envs.get_env_data(env_db, env_name)
Actually getting the data here seems dangerous to me, since each of these
include a "default" flag which is actually determined by the env_db
collectively.  As a pattern, I think it would be better to have env_name only
sorted, or possibly env_name mapping to thunks.  I haven't seen whether this
actually affects your code yet, but it feels like a bug magnet.

[Later] Eh, never mind.  This is not a long-lived data structure.  It's fine.
:-)

https://codereview.appspot.com/43860044/diff/1/quickstart/cli/views.py#newcod...
quickstart/cli/views.py:180: return env_db, None
this is a decoy, right?  No one can use these?

I worry a bit that the functional style here is a mismatch with Urwid.  These
functions are all about side effects.  However, on reflection, I feel like it's
easy to read what you have, and that increasing the functional feel is
increasing the simplicity.  We'll see how time and other people instruct us, I
guess. :-)

https://codereview.appspot.com/43860044/diff/1/quickstart/cli/views.py#newcod...
quickstart/cli/views.py:244: return env_db, None
Isn't this a decoy?  That is, no code will ever actually pay attention to these
results?  either index_view will get the mutated result in set_default, or the
urwid caller will get it via use?

[rharding, 2013-12-18 16:58:53]

LGTM 

QA Notes:

The * in front of each alternates colors. Two are red, which I at first thought
was a default indicator. 

The default is easy to miss down the row. I'ts in the () with type so I assumed
it was part of that at first. A typical thing in cli is to bracket defaults with
a []. Maybe the default row could be 

[*checkmark*] lxc (type: local)

http://www.fileformat.info/info/unicode/char/2713/index.htm

or something? 

<3 the unicode toxic type.

[gary.poster, 2013-12-18 17:07:44]

QA: looks nice and works well!  Mouse support is convenient.  Scrolling in main
content works well with cursor arrows.

I suggest a bit more space between the different parts of the help
text--particularly a space or two more before the "{BULLET} has errors" section.

The temporary messages ("ec2-west successfully set as default") might look nicer
as a horizontal line rather than as a vertical section on the right.

Thanks!

[frankban, 2013-12-19 09:32:25]

On 2013/12/18 16:58:53, rharding wrote:

> The default is easy to miss down the row. I'ts in the () with type so I
assumed
> it was part of that at first. A typical thing in cli is to bracket defaults
with
> a []. Maybe the default row could be 
> 
> [*checkmark*] lxc (type: local)
> 
> http://www.fileformat.info/info/unicode/char/2713/index.htm
> 
> or something?

Done as part of this branch, thank you for this suggestion.

> <3 the unicode toxic type.

:-)

[frankban, 2013-12-19 09:33:26]

On 2013/12/18 17:07:44, gary.poster wrote:

> I suggest a bit more space between the different parts of the help
> text--particularly a space or two more before the "{BULLET} has errors"
section.

Done.

> The temporary messages ("ec2-west successfully set as default") might look
nicer
> as a horizontal line rather than as a vertical section on the right.

Added this suggestion to the UX card, thank you!

[frankban, 2013-12-19 09:36:39]

*** Submitted:

Environment list/selection view.

This branch implements the environment
selection Urwid view. It is possible to
start it running `make` and then
`./cli-app-demo.py`. This is still not
integrated in quickstart, but the demo
file demonstrates how to show the view
passing the required data structures
already implemented in the 
quickstart.models.envs module.

To test the branch, run `make check`.

To QA the new functionality, run the
demo application as described above, 
and play with the interface: any UX
suggestion is welcome, and will be part
of a card we already have 
("envs management: ux improvements").

Other changes:

The Urwid application is no longer an 
ObjectDict (removed from the code).
I decided to use a Python namedtuple
which seemed to me a better choice,
considering that views are not supposed 
to modify the app data structure.
This way we have both immutability and
attribute access to the object.

Implemented a TimeoutText wrapper used to
wrap the message notification widget.
This wayt he timer is restarted when a 
subsequent message is set and the previous 
has not yet been removed.

R=rharding, gary.poster
CC=
https://codereview.appspot.com/43860044

https://codereview.appspot.com/43860044/diff/1/quickstart/cli/base.py
File quickstart/cli/base.py (right):

https://codereview.appspot.com/43860044/diff/1/quickstart/cli/base.py#newcode37
quickstart/cli/base.py:37: _Application = namedtuple(
On 2013/12/18 16:44:58, rharding wrote:
> why the _? Why call it 'App' but also call it _Application? In my experience
I'd
> just have App = namedtuple('App'...

Done.

https://codereview.appspot.com/43860044/diff/1/quickstart/cli/base.py#newcode152
quickstart/cli/base.py:152: # place where to add new API functions, after
changing the application
On 2013/12/18 16:44:58, rharding wrote:
> Create the App named tuple. If, in the future, we have a view that requires
> additional capabilities or API access, this is the place to add those.

Done.

https://codereview.appspot.com/43860044/diff/1/quickstart/cli/ui.py
File quickstart/cli/ui.py (right):

https://codereview.appspot.com/43860044/diff/1/quickstart/cli/ui.py#newcode49
quickstart/cli/ui.py:49: None: 'selected',
On 2013/12/18 16:44:58, rharding wrote:
> I don't follow this. When None is in focus it's selected?

When a widget without a class is in focus, we apply the selected class.

https://codereview.appspot.com/43860044/diff/1/quickstart/cli/views.py
File quickstart/cli/views.py (right):

https://codereview.appspot.com/43860044/diff/1/quickstart/cli/views.py#newcode32
quickstart/cli/views.py:32: A view is a callable receiving an app object (a
named tuple of functions) and
On 2013/12/18 16:44:58, rharding wrote:
> Can we capitalize the App here and in cases referencing the specific object? 

Done.

https://codereview.appspot.com/43860044/diff/1/quickstart/cli/views.py#newcod...
quickstart/cli/views.py:180: return env_db, None
On 2013/12/18 16:57:15, gary.poster wrote:
> this is a decoy, right?  No one can use these?
> 
> I worry a bit that the functional style here is a mismatch with Urwid.  These
> functions are all about side effects.  However, on reflection, I feel like
it's
> easy to read what you have, and that increasing the functional feel is
> increasing the simplicity.  We'll see how time and other people instruct us, I
> guess. :-)

As discussed, I'll make this more explicit.

[frankban, 2013-12-19 09:37:13]

Thank you both!