[merge] MVC progress bars, and indication of network traffic
Martin Pool
mbp at canonical.com
Wed Dec 24 07:14:43 GMT 2008
On 17 Dec 2008, Vincent Ladeuil <v.ladeuil+lp at free.fr> wrote:
> >> 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.
>
> martin> It's a little bit YAGNI, but I wanted to make the
> martin> interface from Transport provide it, even though I
> martin> wasn't sure how to present it. We could show say
>
> martin> 123kb >> 5kB/s
>
> martin> with an arrow depending on whether we're most
> martin> recently reading or writing. I was then wondering if
> martin> we should show the total traffic for both reading and
> martin> writing. In a text display I don't think it's
> martin> feasible or necessary to show both.
>
> As you hint below, more feedback is needed to be sure, but even
> if it's not feasible to display in a text display, GUIs can do
> that.
Right, and I wanted to let them even show multiple progress indicators
for different connections, like apt and update-manager does in Ubuntu.
>
> >> 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)
>
> martin> Here too I wanted to stop before making the patch too
> martin> big. I think what we want eventually is:
>
> martin> * the most generic ui, with nothing implemented
> martin> * a text-mode UI, with no assumptions about the terminal
> martin> capabilities, used e.g. in an emacs compile buffer or
> martin> a cron job
>
> Just for the record, I always set BZR_PROGRESS_BAR=tty under
> emacs (in shell or eshell buffers). I just checked how it behaves
> in compile buffers and there is a small glitch: it seems like the
> '\r' is interpreted as a '\n' there so you end up with:
>
> bzr branch bm:bzr.dev tagada
>
> / [========================================================] Finishing pack 5/5
>
>
> / [== ] Build phase 0/2
> - [=== ] Build phase 0/2
> \ [=== ] Build phase 0/2
> | [=== ] Build phase 0/2
> / [==== ] Build phase 0/2
>
> kind of display. That should be easily fixable with the right
> output-filter (I think I fixed things like that in the past I
> just don't remember the right emacs incantation right now).
It's true that I've removed the operation of BZR_PROGRESS_BAR. I
wondered if it would be used much so this is one data point.
I could add back support for it, at least as a compatibility thing,
though I think eventually it makes more sense to control the entire UI
style rather than just the progress bar method, and that in turn would
be better if it can go off the setting of $TERM or similar rather than
being explicitly set.
If you're seeing the bars being printed on one line after another like
that in emacs then it suggests that it just doesn't interpret \r like a
terminal does, at least without special configuration of an output
filter. I'm using the same basic approach we were previously, but just
precisely when the \r is output may be different to before and that
might be the problem: we're now emitting one both after and before the
progress bar is drawn, and that might confuse it.
It would be nice if it just worked under emacs without needing special
configuration.
>
> martin> * a text-mode UI that can repaint and draw progress bars
> martin> * various guis
>
> martin> I think we probably want to get rid of SilentUI; it
> martin> doesn't seem to make sense to me to have something
> martin> that can't write output but does read input from
> martin> stdin.
>
> I think we need some way to tell bzr to never ask for inputs
> (even for passwords) for scripting purposes from cron jobs, that
> may still be a use case for a SilentUI.
That's a useful feature but I'm not sure it quite fits in with the
selection of UI, as it's currently construed. It's possibly useful
(though sometimes difficult) to control an interactive program by
feeding it responses from stdin. For instance "echo y|bzr break-lock
foo". And, even in cases where we do have a terminal and can draw
progress bars, people might like to make the program non-interactive,
like with a --yes or --no-interactive option.
It may be that those options are best done by changing the UI to be
noninteractive, but like for --verbose or --quiet I suspect it actually
needs to control what the command does, not just its channel to the
user.
> martin> I think it'll be OK to do now. It is the kind of
> martin> thing where getting it right probably needs feedback
> martin> from people trying it in different cases rather than
> martin> review or unit tests.
>
> +1
OK, so I think there are no blocking issues from yourself or John, so
I'm going to send it to pqm.
> <snip/>
>
> martin> + def note(self, fmt_string, *args):
> martin> + """Record a note without disrupting the progress bar."""
> martin> + # XXX: shouldn't be here; put it in mutter or the ui instead
>
> I understand that.
>
> martin> + self.ui_factory.note(fmt_string % args)
> martin> +
> martin> + def clear(self):
> martin> + # XXX: shouldn't be here; put it in mutter or the ui instead
>
> But not that. Is sounds more natural to me to clear the pb when
> we're done with it. Can you explain ?
>
> martin> + self.ui_factory.clear_term()
What I meant is this: it does make sense of course for the progress bar
to know how to clear itself off. But, for application code, I think it
shouldn't generally be asking the progress bar to do so. Instead it
should say to the UI or something, "I want to print some text, make sure
the terminal's ready for that", and that might clear the pb. The idea
is that most of the application code deals with only a pb model, and it
doesn't make sense to ask the model to clear itself. (What if it's in a
GUI?) But it might not actually be an important distinction at the
moment.
> martin> @@ -266,6 +269,7 @@
> martin> # Move the start-of-buffer pointer
> martin> input_start += cur_size
> martin> # Yield the requested data
> martin> + self._report_activity(len(cur_data), 'read')
> martin> yield cur_offset, cur_data
> martin> cur_offset, cur_size = offset_iter.next()
> martin> # at this point, we've consumed as much of buffered as we can,
> martin> @@ -312,6 +316,7 @@
> martin> raise AssertionError('We must have miscalulated.'
> martin> ' We expected %d bytes, but only found %d'
> martin> % (cur_size, len(data)))
> martin> + self._report_activity(len(data), 'read')
> martin> yield cur_offset, data
> martin> cur_offset, cur_size = offset_iter.next()
>
> I'm pretty sure the following will be more accurate:
>
> === modified file 'bzrlib/transport/sftp.py'
> --- bzrlib/transport/sftp.py 2008-12-14 20:06:49 +0000
> +++ bzrlib/transport/sftp.py 2008-12-17 09:17:13 +0000
> @@ -222,6 +222,7 @@
> if len(data) != length:
> raise errors.ShortReadvError(self.relpath,
> start, length, len(data))
> + self._report_activity(length, 'read')
> if last_end is None:
> # This is the first request, just buffer it
> buffered_data = [data]
> @@ -269,7 +270,6 @@
> # Move the start-of-buffer pointer
> input_start += cur_size
> # Yield the requested data
> - self._report_activity(len(cur_data), 'read')
> yield cur_offset, cur_data
> cur_offset, cur_size = offset_iter.next()
> # at this point, we've consumed as much of buffered as we can,
> @@ -316,7 +316,6 @@
> raise AssertionError('We must have miscalulated.'
> ' We expected %d bytes, but only found %d'
> % (cur_size, len(data)))
> - self._report_activity(len(data), 'read')
> yield cur_offset, data
> cur_offset, cur_size = offset_iter.next()
>
>
> because it will report the activity just after the read
> succeeds. What you did reported activity from the buffered data.
OK, thanks.
>
> But more importantly, I think using a transport bound method is a
> very nice design choice and should apply cleanly to the
> http.response.RangeFile object (even reading the multipart
> headers and the boundaries can be reported that way...).
Thanks.
> martin> @@ -180,3 +209,23 @@
> martin> ui_factory = SilentUIFactory()
> martin> """IMPORTANT: never import this symbol directly. ONLY ever access it as
> martin> ui.ui_factory."""
> martin> +
> martin> +
> martin> +def make_ui_for_terminal(stdin, stdout, stderr):
> martin> + """Construct and return a suitable UIFactory for a text mode program.
> martin> +
> martin> + If stdout is a smart terminal, this gets a smart UIFactory with
> martin> + progress indicators, etc. If it's a dumb terminal, just plain text output.
> martin> + """
> martin> + isatty = getattr(stdin, 'isatty', None)
> martin> + if isatty is None:
> martin> + cls = CLIUIFactory
> martin> + elif not isatty():
> martin> + cls = CLIUIFactory
> martin> + elif os.environ.get('TERM') in (None, 'dumb', ''):
> martin> + # e.g. emacs compile window
> martin> + cls = CLIUIFactory
> martin> + else:
> martin> + from bzrlib.ui.text import TextUIFactory
> martin> + cls = TextUIFactory
> martin> + return cls(stdin=stdin, stdout=stdout, stderr=stderr)
>
>
> Oooh, that's where you break my setup :-) Setting
> BZR_PROGRESS_BAR is not enough anymore, if you don't respect it
> :-)
>
> I tried your patch under emacs and got tracebacks because
> CLIUIFactory doesn't define show_progress. It works fine from a
> bare terminal of course.
>
> I'm not sure what the right fix is here, either obey
> BZR_PROGRESS_BAR, or define show_progress et al in CLIUIFactory
> instead of SilentUIFactory, or even both.
I moved the stub implementations of those methods to CLIUIFactory, which
sounds reasonable, pending more reworking of the UI class hierarchy. I
tried this briefly in emacs shell mode, which sets TERM=dumb, and it
worked OK without showing a progress bar.
In fact, if I set the TERM variable, then it can draw the progress bar.
But from memory this varies a bit between different emacs subprocess
types.
--
Martin <http://launchpad.net/~mbp>
More information about the bazaar
mailing list