[merge] MVC progress bars, and indication of network traffic
Vincent Ladeuil
v.ladeuil+lp at free.fr
Wed Dec 17 09:38:42 GMT 2008
Thanks for working on that !
>>>>> "martin" == Martin Pool <mbp at canonical.com> writes:
<snip/>
>>
>> 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.
>> 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).
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.
martin> I don't think the 'dots' bar is very useful, because
martin> we don't have any good idea of the total amount of
martin> traffic and so you just get a screenful of dots.
martin> Maybe others find it so?
The only place I used it was under emacs before I begin to set
BZR_PROGRESS_BAR, so *I* don't use it anymore.
martin> Also, I find I rarely have an interactive session
martin> that can't repaint the screen.
>> Also, no tests for 'make_ui_for_terminal' ?
martin> 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.
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
<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()
<snip/>
martin> === modified file 'bzrlib/transport/__init__.py'
martin> --- bzrlib/transport/__init__.py 2008-12-01 23:20:22 +0000
martin> +++ bzrlib/transport/__init__.py 2008-12-15 01:01:23 +0000
<snip/>
martin> @@ -568,7 +581,11 @@
martin> :param relpath: The relative path to the file
martin> """
martin> - return self.get(relpath).read()
martin> + f = self.get(relpath)
martin> + try:
martin> + return f.read()
martin> + finally:
martin> + f.close()
EErk ! I'm pretty sure you just closed the http socket here (the
http transport doesn't redefine get_bytes) ! If no test is
failing there, we need one. On the other hand, I'm not sure you
need that part in your patch.
martin> @deprecated_method(one_four)
martin> def get_smart_client(self):
martin> === modified file 'bzrlib/transport/sftp.py'
martin> --- bzrlib/transport/sftp.py 2008-11-07 05:39:09 +0000
martin> +++ bzrlib/transport/sftp.py 2008-12-14 20:06:49 +0000
<snip/>
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.
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...).
I'll try that as soon as your patch land :)
<snip/>
martin> === modified file 'bzrlib/ui/text.py'
martin> --- bzrlib/ui/text.py 2007-08-15 04:33:34 +0000
martin> +++ bzrlib/ui/text.py 2008-12-16 07:56:29 +0000
<snip/>
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.
Vincent
More information about the bazaar
mailing list