[merge] MVC progress bars, and indication of network traffic

[Martin Pool]

Thanks for the speedy review, kiko was just asking me on irc if this was
in yet.
 
On 16 Dec 2008, John Arbash Meinel <john at arbash-meinel.com> wrote:

> > This removes MissingProgressBarFinish which I think adds little compared
> > to just an AssertionError.

> I believe one thing it adds is that we don't get a full traceback, since
> MissingProgressBarFinish is not an actual "failure".
> 
> If anything, we should turn that code into a warning rather than raising
> an exception.
> 
> At a minimum:
> @@ -147,7 +206,7 @@
>      def return_pb(self, bar):
>          """Return bar after its been used."""
>          if bar is not self._stack[-1]:
> -            raise errors.MissingProgressBarFinish()
> +            raise AssertionError()
>          self._stack.pop()
> 
> ^- You are raising a plain Assertion which doesn't give *any*
> information about what is being asserted. So you could change this to:
> raise AssertionError('last progress bar on stack was not properly finished')

That's true, I was a bit lazy, though you do get a traceback showing
where it's from.

This is the kind of thing that tends to generate bugs complaining about
MissingProgressBarFinish, when there's actually some other problem.
"Too many concurrent requests" was a classic case of this.  On the whole
a warning is probably better, and it is possible for test cases to
observe the warning.

> > There's more we could do here but this is getting long, so I'll ask for
> > review now.  One of the main things to follow on would be to deprecate
> > or just remove the older ProgressBar classes, and also look at
> > simplifying the code which currently passes pb objects around.
> 
> At one part of the code you set "direction" but then you get rid of that
> when you "show_transport_activity". Wouldn't it be nice to know that we
> are downloading a lot during a small push? I suppose it doesn't help
> users, but it may help developers.

It's a little bit YAGNI, but I wanted to make the interface from
Transport provide it, even though I wasn't sure how to present it.  We
could show say

  123kb >>   5kB/s

with an arrow depending on whether we're most recently reading or
writing.  I was then wondering if we should show the total traffic for
both reading and writing.  In a text display I don't think it's feasible
or necessary to show both.

> The new layering seems fine, though I wonder about CLIUIFactory versus
> TextUIFactory.
> 
> If only because the scripts and manual work I've done basically involved
> manually setting ui.ui_factory = TextUIFactory(), and I was wondering
> about preserving that.
> 
> Is it that CLIUI gets a progress bar with '\r', while Text just gets a
> Dots progress bar? (Maybe I have that backwards)

Here too I wanted to stop before making the patch too big.  I think what
we want eventually is:

  * the most generic ui, with nothing implemented
    * a text-mode UI, with no assumptions about the terminal
      capabilities, used e.g. in an emacs compile buffer or 
      a cron job
      * a text-mode UI that can repaint and draw progress bars
    * various guis

I think we probably want to get rid of SilentUI; it doesn't seem to make
sense to me to have something that can't write output but does read
input from stdin.

I don't think the 'dots' bar is very useful, because we don't have any
good idea of the total amount of traffic and so you just get a screenful
of dots.  Maybe others find it so?  Also, I find I rarely have an
interactive session that can't repaint the screen.  

> Also, no tests for 'make_ui_for_terminal' ?

No, which was a bit lazy.  I'll add some in, but put that up separately.

> Anyway, I'd like to see this land early in a release cycle so we can get
> a feel for it in bzr.dev before we make a release. If you feel it is
> early enough in this release, go for it.

I think it'll be OK to do now.  It is the kind of thing where getting it
right probably needs feedback from people trying it in different cases
rather than review or unit tests.

-- 
Martin