Merge lp:~mbp/bzr/387717-progress-bar-tty into lp:~bzr/bzr/trunk-old

[Martin Pool]

[Martin Pool (mbp)]

This stops progress bars being sent to stderr when it's not a file.

(That's not quite the point of bug 387717, but it's still useful.)

There are some more changes in here: ProgressTasks now talk directly to their ProgressView rather than to the UIFactory which makes them more easily testable in isolation, and may let us remove some forwarding methods in the UIFactory. (The factory still constructs the task so it can arrange to have notifications sent where ever it wants.)

Some classes whose constructor is deprecated now have comments pointing this out.

LINES, TERM and COLUMNS are set to specific values when running tests to reduce variation.

[Andrew Bennetts (spiv)]

> + # TODO: deprecate passing ui_factory

It would be good to actually do this TODO :)

It shouldn't be hard to do.

> 145 + 'TERM': 'dumb',
> 146 + 'LINES': '25',
> 147 + 'COLUMNS': '80',

(Idle thought... I wonder if the cost of constructing that dictionary every test is a measureable cost on the total test suite run, especially as we keep adding things to it?)

Part of me wonders if it would be better to just set these variables on TestTextProgressView rather than in the base case...

> + task = self.make_task(None, view, 'reticulating splines', 5, 20)

:)

> 248 + self.assertEqual(
> 249 + r'[####| ] a:b:c 1/2'
> 250 + , view._render_line())

I think adding a assertRendering method might reduce boilerplate a little, but it's not a big issue given there's only a few test methods. (And it's only moved code anyway, rather than new code.)

review: Approve

[Matthew Fuller (fullermd)]

> * Progress bars are now suppressed again when the environment variable
> + ``BZR_PROGRESS_BAR`` is set to ``none``, or when stderr is directed to
> + a terminal.
> + (Martin Pool, #339385, #387717)

They're suppressed when stderr *IS* directed to a terminal? 8-}

--
Matthew Fuller (MF4839) | <email address hidden>
Systems/Network Administrator | http://www.over-yonder.net/~fullermd/
           On the Internet, nobody can hear you scream.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Martin Pool wrote:
> Martin Pool has proposed merging lp:~mbp/bzr/387717-progress-bar-tty into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
> This stops progress bars being sent to stderr when it's not a file.
>
> (That's not quite the point of bug 387717, but it's still useful.)
>
> There are some more changes in here: ProgressTasks now talk directly to their ProgressView rather than to the UIFactory which makes them more easily testable in isolation, and may let us remove some forwarding methods in the UIFactory. (The factory still constructs the task so it can arrange to have notifications sent where ever it wants.)
>
> Some classes whose constructor is deprecated now have comments pointing this out.
>
> LINES, TERM and COLUMNS are set to specific values when running tests to reduce variation.
>

...

@@ -82,7 +91,9 @@
         self.total_cnt = None
         self.current_cnt = None
         self.msg = ''
+ # TODO: deprecate passing ui_factory
         self.ui_factory = ui_factory
+ self.progress_view = progress_view
         self.show_pct = False
         self.show_spinner = True
         self.show_eta = False,

^- Why not just deprecate it?

if ui_factory is not None:
  symbol_versioning.warn(
    'Passing ui_factory' % symbol_versioning.deprecated_in((1, 17, 0)),
    ' Pass progress_view instead')

or are there still bits that use the ui_factory without going to
progress_view?

...

+# NOTE: This is also deprecated; you should provide a ProgressView instead.
 class _BaseProgressBar(object):

Same here, just wrap __init__:

     @deprecated_method(deprecated_in((1, 17, 0)))
     def __init__(self,

Alternatively if you still need to do it from time to time, we've used a
flag like "_internal=False" and then:

if not _internal:
  warn...

So other than

1) Deprecating things with comments rather than deprecated_in()
2)
* Progress bars are now suppressed again when the environment variable
- - ``BZR_PROGRESS_BAR`` is set to ``none``.
- - (Martin Pool, #339385)
+ ``BZR_PROGRESS_BAR`` is set to ``none``, or when stderr is directed to
+ a terminal.
+ (Martin Pool, #339385, #387717)

^- And this having the wrong meaning.

I approve, but it needs fixing. I would vote "tweak" but I don't know
how to do that.

Review Approve
  review: needs_fixing

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAko7mm4ACgkQJdeBCYSNAAPRKwCeJtFuuy6f7RbFEpw7fsNUubkQ
BMgAoICd00+tFOLRsG69P3PFNSGbYVBJ
=Kpqi
-----END PGP SIGNATURE-----

review: Needs Fixing

[Vincent Ladeuil (vila)]

>>>>> "martin" == Martin Pool <email address hidden> writes:

    martin> Martin Pool has proposed merging
    martin> lp:~mbp/bzr/387717-progress-bar-tty into lp:bzr.

    martin> Requested reviews:
    martin> bzr-core (bzr-core)

    martin> This stops progress bars being sent to stderr when
    martin> it's not a file.

    martin> (That's not quite the point of bug 387717, but it's still
    martin> useful.)

    martin> There are some more changes in here: ProgressTasks now talk
    martin> directly to their ProgressView rather than to the UIFactory which
    martin> makes them more easily testable in isolation,

    martin> and may let us remove some forwarding methods in the
    martin> UIFactory.

Eerk, that's where bzr-gtk is plugged in: it uses
_progress_all_finished and _progress_updated (I know, I know,
private methods, bad vila, no sugar :).

    martin> (The factory still constructs the task so it can
    martin> arrange to have notifications sent where ever it
    martin> wants.)

At least make UIFactory use a progress_view class attribute or
define _make_progress_view in UIFactory, not in TextUIFactory and
make it public.

Additional bonus points if you can extract the reusable parts
From TextProgressView ;-D

    martin> Some classes whose constructor is deprecated now have
    martin> comments pointing this out.

Hardly discoverable I'm afraid...

I know the deprecation dance is painful, but bzr-gtk is currently
claiming compatibility with bzr 1.6 -> 1.13 and 1.15, if you
remove the forwarding methods above, that would become quite a
challenge. So at least these methods will have to be deprecated
properly (I don't really care about the classes you refer to
here).

<snip/>

    martin> === modified file 'NEWS'
    martin> --- NEWS 2009-06-17 05:05:28 +0000
    martin> +++ NEWS 2009-06-19 08:49:14 +0000
    martin> @@ -21,8 +21,9 @@
    martin> (Neil Martinsen-Burrell, #220067)

    martin> * Progress bars are now suppressed again when the environment
    martin> variable
    martin> - ``BZR_PROGRESS_BAR`` is set to ``none``.
    martin> - (Martin Pool, #339385)
    martin> + ``BZR_PROGRESS_BAR`` is set to ``none``, or when stderr is directed to

Who ate that 'not' ? :)

<snip/>

    martin> @@ -71,7 +70,8 @@
    martin> self._progress_view.clear()

    martin> def _make_progress_view(self):
    martin> - if os.environ.get('BZR_PROGRESS_BAR') in ('text', None, ''):
    martin> + if (os.environ.get('BZR_PROGRESS_BAR') in ('text', None, '')
    martin> + and progress._supports_progress(self.stderr)):
    martin> return TextProgressView(self.stderr)
    martin> else:
    martin> return NullProgressView()

Could we please stop outsmarting the user ? ;-D

Please do:

     def _make_progress_view(self):
- if (os.environ.get('BZR_PROGRESS_BAR') in ('text', None, '')
+ if os.environ.get('BZR_PROGRESS_BAR') == 'text':
+ return TextProgressView(self.stderr)
+ elif (os.environ.get('BZR_PROGRESS_BAR') in (None, '')

I assure you I dig hard, but there is no way to reliably detect
where the output is going when bzr is run under emacs, i...

review: Needs Fixing

[Martin Pool (mbp)]

2009/6/19 fullermd <email address hidden>:
>>  * Progress bars are now suppressed again when the environment variable
>> +  ``BZR_PROGRESS_BAR`` is set to ``none``, or when stderr is directed to
>> +  a terminal.
>> +  (Martin Pool, #339385, #387717)
>
> They're suppressed when stderr *IS* directed to a terminal?  8-}

Good catch, it was backwards.

--
Martin <http://launchpad.net/~mbp/>

[Martin Pool (mbp)]

2009/6/20 John A Meinel <email address hidden>:
> @@ -82,7 +91,9 @@
>         self.total_cnt = None
>         self.current_cnt = None
>         self.msg = ''
> +        # TODO: deprecate passing ui_factory
>         self.ui_factory = ui_factory
> +        self.progress_view = progress_view
>         self.show_pct = False
>         self.show_spinner = True
>         self.show_eta = False,
>
>
> ^- Why not just deprecate it?
>
> if ui_factory is not None:
>  symbol_versioning.warn(
>    'Passing ui_factory' % symbol_versioning.deprecated_in((1, 17, 0)),
>    ' Pass progress_view instead')
>
> or are there still bits that use the ui_factory without going to
> progress_view?

I though there might be some nontrivial work to deprecate it and to
look into how that would impact bzr-gtk, so I wanted to land this
first. (And from Vincent's comments it looks like that's true.)

That is the kind of change I'll put in when it's ready.
>
> ...
>
> +# NOTE: This is also deprecated; you should provide a ProgressView instead.
>  class _BaseProgressBar(object):
>
> Same here, just wrap __init__:
>
>     @deprecated_method(deprecated_in((1, 17, 0)))
>     def __init__(self,
>
> Alternatively if you still need to do it from time to time, we've used a
> flag like "_internal=False" and then:
>
> if not _internal:
>  warn...

All the public concrete constructors are deprecated. You're right I
could and maybe should add an _internal flag so that any callers other
than those subclasses will warn.

>
>
> So other than
>
> 1) Deprecating things with comments rather than deprecated_in()

My cover letter wasn't very clear: these things are already actually
deprecated, it's just that the deprecation marks the constructor not
the class and I wanted it to be more obvious.

> I approve, but it needs fixing. I would vote "tweak" but I don't know
> how to do that.

There's a bug asking for a 'tweak' status. -
https://bugs.edge.launchpad.net/launchpad-code/+bug/373078

--
Martin <http://launchpad.net/~mbp/>

[Martin Pool (mbp)]

2009/6/20 Vincent Ladeuil <email address hidden>:
> Review: Needs Fixing
>
>>>>>> "martin" == Martin Pool <email address hidden> writes:
>
>    martin> Martin Pool has proposed merging
>    martin> lp:~mbp/bzr/387717-progress-bar-tty into lp:bzr.
>
>    martin> Requested reviews:
>    martin>     bzr-core (bzr-core)
>
>    martin> This stops progress bars being sent to stderr when
>    martin> it's not a file.
>
>
>    martin> (That's not quite the point of bug 387717, but it's still
>    martin> useful.)
>
>    martin> There are some more changes in here: ProgressTasks now talk
>    martin> directly to their ProgressView rather than to the UIFactory which
>    martin> makes them more easily testable in isolation,
>
>    martin> and may let us remove some forwarding methods in the
>    martin> UIFactory.
>
> Eerk, that's where bzr-gtk is plugged in: it uses
> _progress_all_finished and _progress_updated (I know, I know,
> private methods, bad vila, no sugar :).

This is why I have some doubt about our past approach of saying
private interfaces can be changed without warning - if people need to
access something, they'll do it regardless of the name and then we
break them. Therefore I think the naming should be just as a guide to
use that particular class, and we need to make the assessment more
broadly.

At any rate I have not actually removed those methods. I'll have a
look at bzr-gtk.

>    martin> (The factory still constructs the task so it can
>    martin> arrange to have notifications sent where ever it
>    martin> wants.)
>
> At least make UIFactory use a progress_view class attribute or
> define _make_progress_view in UIFactory, not in TextUIFactory and
> make it public.
>
> Additional bonus points if you can extract the reusable parts
> >From TextProgressView ;-D

Good ideas.

Though I'll need to think about whether the public should be able to
ask the UIFactory for a ProgressView as opposed to asking it for a
progress bar already bound to a view. How the views get hooked up is
possibly part of the UI's policy...

>    martin> Some classes whose constructor is deprecated now have
>    martin> comments pointing this out.
>
> Hardly discoverable I'm afraid...
>
> I know the deprecation dance is painful, but bzr-gtk is currently
> claiming compatibility with bzr 1.6 -> 1.13 and 1.15, if you
> remove the forwarding methods above, that would become quite a
> challenge. So at least these methods will have to be deprecated
> properly (I don't really care about the classes you refer to
> here).

I didn't deprecate anything, I just added comments on classes whose
constructor is deprecated.

>    martin>      def _make_progress_view(self):
>    martin> - if os.environ.get('BZR_PROGRESS_BAR') in ('text', None, ''):
>    martin> + if (os.environ.get('BZR_PROGRESS_BAR') in ('text', None, '')
>    martin> +            and progress._supports_progress(self.stderr)):
>    martin>              return TextProgressView(self.stderr)
>    martin>          else:
>    martin>              return NullProgressView()
>
>
> Could we please stop outsmarting the user ? ;-D
>
> Please do:
>
>     def _make_progress_view(self):
> -        if (os.environ.get('BZR_P...

[Martin Pool (mbp)]

In bzr-gtk I see:

        if getattr(ui.ui_factory, "set_progress_bar_widget", None) is not None:
            # We'are using our own ui, let's tell it to use our widget.
            ui.ui_factory.set_progress_bar_widget(self.progress_widget)

so it can redirect the progress display into the relevant window, and
then it uses _progress_updated and _progress_all_finished to update
them.

So I don't think my patch will break this at all.

If we did deprecate the things I mentioned deprecating, then bzr-gtk
would need to (at least with later bzrlibs) return its own
ProgressView that points to the right progress bar. I don't think it
will be traumatic.

I was in passing thinking about letting it use the activity
notifications to update a throbber and/or to run a nested gtk main
loop.

[Vincent Ladeuil (vila)]

>>>>> "martin" == Martin Pool <email address hidden> writes:

<snip/>

    >>    martin> and may let us remove some forwarding methods in the
    >>    martin> UIFactory.
    >>
    >> Eerk, that's where bzr-gtk is plugged in: it uses
    >> _progress_all_finished and _progress_updated (I know, I know,
    >> private methods, bad vila, no sugar :).

    martin> This is why I have some doubt about our past approach
    martin> of saying private interfaces can be changed without
    martin> warning - if people need to access something, they'll
    martin> do it regardless of the name and then we break them.

Right. I think our approach about deprecation is sound.

Regarding plugins that use private methods because they couldn't
find a way to achieve something with the public ones, well, we
can't predict *all* needs. But we may encourage people to
*report* what private methods they use so that we can either make
them public or explain how to avoid using them.

<snip/>

    martin> At any rate I have not actually removed those
    martin> methods.

Sure, but you said you will :)

<snip/>

    >> At least make UIFactory use a progress_view class attribute or
    >> define _make_progress_view in UIFactory, not in TextUIFactory and
    >> make it public.
    >>
    >> Additional bonus points if you can extract the reusable parts
    >> >From TextProgressView ;-D

    martin> Good ideas.

Great.

    martin> Though I'll need to think about whether the public
    martin> should be able to ask the UIFactory for a
    martin> ProgressView as opposed to asking it for a progress
    martin> bar already bound to a view.

As I understand your design, there are no more progress bars,
there are progress tasks, a stack of them is maintained
transparently, and the whole stack uses a single report view (I
call it widget).

    martin> How the views get hooked up is possibly part of the
    martin> UI's policy...

Definitely.

There is still a problem specific to bzr-gtk (at least I couldn't
a figure a way bzrlib itself could address it).

Right now, bzr-gtk use a single UIFactory instance (and that's
the design encouraged by bzrlib). From there, the progress widget
is unique too (since UIFactory forwards to it).

Now, imagine that 'bzr viz' starts 'bzr gblame' and that each of
them use a report widget... at one point the progress will
reported by the wrong widget.

Again, I anticipate, it hasn't been encountered yet.

Just a limitation I wanted you to be aware of.

The way that could be addressed in bzr-gtk is to set the progress
widget before each call to bzrlib that requires it (as long as
the bzrlib calls are issued by objects that know, or have a way
to know, which progress widget they should use, that shouldn't be
a too heavy constraint).

And if no more progress widgets are around, bzr-gtk falls back to
creating a top-level window for that purpose.

The following threads give more details:
https://lists.canonical.com/archives/bzr-gtk/2009-June/001824.html
https://lists.canonical.com/archives/bzr-gtk/2009-June/001846.html

    >>    martin> Some classes whose constructor is deprecated now have
    >>    martin> comments pointing this out.
    >>
    >> Ha...

[Vincent Ladeuil (vila)]

>>>>> "martin" == Martin Pool <email address hidden> writes:

<snip/>

    >>    martin> and may let us remove some forwarding methods in the
    >>    martin> UIFactory.
    >>
    >> Eerk, that's where bzr-gtk is plugged in: it uses
    >> _progress_all_finished and _progress_updated (I know, I know,
    >> private methods, bad vila, no sugar :).

    martin> This is why I have some doubt about our past approach
    martin> of saying private interfaces can be changed without
    martin> warning - if people need to access something, they'll
    martin> do it regardless of the name and then we break them.

Right. I think our approach about deprecation is sound.

Regarding plugins that use private methods because they couldn't
find a way to achieve something with the public ones, well, we
can't predict *all* needs. But we may encourage people to
*report* what private methods they use so that we can either make
them public or explain how to avoid using them.

<snip/>

    martin> At any rate I have not actually removed those
    martin> methods.

Sure, but you said you will :)

<snip/>

    >> At least make UIFactory use a progress_view class attribute or
    >> define _make_progress_view in UIFactory, not in TextUIFactory and
    >> make it public.
    >>
    >> Additional bonus points if you can extract the reusable parts
    >> >From TextProgressView ;-D

    martin> Good ideas.

Great.

    martin> Though I'll need to think about whether the public
    martin> should be able to ask the UIFactory for a
    martin> ProgressView as opposed to asking it for a progress
    martin> bar already bound to a view.

As I understand your design, there are no more progress bars,
there are progress tasks, a stack of them is maintained
transparently, and the whole stack uses a single report view (I
call it widget).

    martin> How the views get hooked up is possibly part of the
    martin> UI's policy...

Definitely.

There is still a problem specific to bzr-gtk (at least I couldn't
a figure a way bzrlib itself could address it).

Right now, bzr-gtk use a single UIFactory instance (and that's
the design encouraged by bzrlib). From there, the progress widget
is unique too (since UIFactory forwards to it).

Now, imagine that 'bzr viz' starts 'bzr gblame' and that each of
them use a report widget... at one point the progress will
reported by the wrong widget.

Again, I anticipate, it hasn't been encountered yet.

Just a limitation I wanted you to be aware of.

The way that could be addressed in bzr-gtk is to set the progress
widget before each call to bzrlib that requires it (as long as
the bzrlib calls are issued by objects that know, or have a way
to know, which progress widget they should use, that shouldn't be
a too heavy constraint).

And if no more progress widgets are around, bzr-gtk falls back to
creating a top-level window for that purpose.

The following threads give more details:
https://lists.canonical.com/archives/bzr-gtk/2009-June/001824.html
https://lists.canonical.com/archives/bzr-gtk/2009-June/001846.html

    >>    martin> Some classes whose constructor is deprecated now have
    >>    martin> comments pointing this out.
    >>
    >> Ha...

[Vincent Ladeuil (vila)]

>>>>> "martin" == Martin Pool <email address hidden> writes:

    martin> In bzr-gtk I see:
    martin> if getattr(ui.ui_factory, "set_progress_bar_widget", None) is not None:
    martin> # We'are using our own ui, let's tell it to use our widget.
    martin> ui.ui_factory.set_progress_bar_widget(self.progress_widget)

    martin> so it can redirect the progress display into the relevant window, and
    martin> then it uses _progress_updated and _progress_all_finished to update
    martin> them.

That's it.

    martin> So I don't think my patch will break this at all.

Not yet :-)

    martin> If we did deprecate the things I mentioned
    martin> deprecating, then bzr-gtk would need to (at least
    martin> with later bzrlibs) return its own ProgressView that
    martin> points to the right progress bar. I don't think it
    martin> will be traumatic.

Agreed.

My point was that it will not be able (as it is now) to support
multiple bzr versions, not a big deal, but one aim of our
deprecation policy is to warn people about what part of their
toolset they should upgrade...

    martin> I was in passing thinking about letting it use the
    martin> activity notifications to update a throbber and/or to
    martin> run a nested gtk main loop.

The GtkProgressBar.update() does:

        while gtk.events_pending():
            gtk.main_iteration()

  Vincent

[Martin Pool (mbp)]

2009/6/22 Vincent Ladeuil <email address hidden>:
>    martin> If we did deprecate the things I mentioned
>    martin> deprecating, then bzr-gtk would need to (at least
>    martin> with later bzrlibs) return its own ProgressView that
>    martin> points to the right progress bar.  I don't think it
>    martin> will be traumatic.
>
> Agreed.
>
> My point was that it will not be able (as it is now) to support
> multiple bzr versions, not a big deal, but one aim of our
> deprecation policy is to warn people about what part of their
> toolset they should upgrade...

I'll try to deprecate them soon so that we can have a long warning period.

>    martin> I was in passing thinking about letting it use the
>    martin> activity notifications to update a throbber and/or to
>    martin> run a nested gtk main loop.
>
> The GtkProgressBar.update() does:
>
>        while gtk.events_pending():
>            gtk.main_iteration()
>
>  Vincent

If it does that from the transport activity notification callback too
it will probably get much more responsive when network IO is running.

--
Martin <http://launchpad.net/~mbp/>

[Martin Pool (mbp)]

> > + # TODO: deprecate passing ui_factory
>
> It would be good to actually do this TODO :)
>
> It shouldn't be hard to do.

I'll probably do it next or soon, but wanted to keep this review small.

> > 145 + 'TERM': 'dumb',
> > 146 + 'LINES': '25',
> > 147 + 'COLUMNS': '80',
>
> (Idle thought... I wonder if the cost of constructing that dictionary every
> test is a measureable cost on the total test suite run, especially as we keep
> adding things to it?)
>
> Part of me wonders if it would be better to just set these variables on
> TestTextProgressView rather than in the base case...

We have occasionally had errors in other cases, particularly in black-box tests but also in others, due to them inadvertently depending on one of these. In a sense every one of those failures is a chance to think about whether that particular case should be tested across various screen setups, but on the whole I think this is an inefficient way to discover them. Let's instead have a standard environment across all tests and then add variations where we need to.

>
> > + task = self.make_task(None, view, 'reticulating splines', 5, 20)
>
> :)
>
> > 248 + self.assertEqual(
> > 249 + r'[####| ] a:b:c 1/2'
> > 250 + , view._render_line())
>
> I think adding a assertRendering method might reduce boilerplate a little, but
> it's not a big issue given there's only a few test methods. (And it's only
> moved code anyway, rather than new code.)

Right.

=== modified file 'NEWS'
--- NEWS	2009-06-20 01:53:18 +0000
+++ NEWS	2009-06-20 08:35:18 +0000
@@ -39,8 +39,9 @@
   (Ian Clatworthy)
 
 * Progress bars are now suppressed again when the environment variable
-  ``BZR_PROGRESS_BAR`` is set to ``none``.
-  (Martin Pool, #339385)
+  ``BZR_PROGRESS_BAR`` is set to ``none``, or when stderr is directed to
+  a terminal.
+  (Martin Pool, #339385, #387717)
 
 * Unshelve works correctly when multiple zero-length files are present on
   the shelf. (Aaron Bentley, #363444)
@@ -94,6 +95,10 @@
 API Changes
 ***********
 
+* ProgressTasks now prefer to talk direct to their ProgressView not to the
+  UIFactory. 
+  (Martin Pool)
+
 * Removed overspecific error class ``InvalidProgressBarType``.
   (Martin Pool)
 

=== modified file 'bzrlib/progress.py'
--- bzrlib/progress.py	2009-06-10 03:56:49 +0000
+++ bzrlib/progress.py	2009-06-20 08:35:18 +0000
@@ -42,13 +42,15 @@
     )
 
 
-# XXX: deprecated; can be removed when the ProgressBar factory is removed
 def _supports_progress(f):
-    """Detect if we can use pretty progress bars on the output stream f.
+    """Detect if we can use pretty progress bars on file F.
 
     If this returns true we expect that a human may be looking at that
     output, and that we can repaint a line to update it.
+
+    This doesn't check the policy for whether we *should* use them.
     """
+    # XXX: This is a bit redundant with make_ui_for_terminal.
     isatty = getattr(f, 'isatty', None)
     if isatty is None:
         return False
@@ -71,9 +73,16 @@
     will not necessarily respect all these fields.
     """
 
-    def __init__(self, parent_task=None, ui_factory=None):
+    def __init__(self, parent_task=None, ui_factory=None, progress_view=None):
         """Construct a new progress task.
 
+        :param parent_task: Enclosing ProgressTask or None.
+
+        :param progress_view: ProgressView to display this ProgressTask.
+
+        :param ui_factory: The UI factory that will display updates; 
+            deprecated in favor of passing progress_view directly.
+
         Normally you should not call this directly but rather through
         `ui_factory.nested_progress_bar`.
         """
@@ -82,7 +91,9 @@
         self.total_cnt = None
         self.current_cnt = None
         self.msg = ''
+        # TODO: deprecate passing ui_factory
         self.ui_factory = ui_factory
+        self.progress_view = progress_view
         self.show_pct = False
         self.show_spinner = True
         self.show_eta = False,
@@ -101,16 +112,23 @@
         self.current_cnt = current_cnt
         if total_cnt:
             self.total_cnt = total_cnt
-        self.ui_factory._progress_updated(self)
+        if self.progress_view:
+            self.progress_view.show_progress(self)
+        else:
+            self.ui_factory._progress_updated(self)
 
     def tick(self):
         self.update(self.msg)
 
     def finished(self):
-        self.ui_factory._progress_finished(self)
+        if self.progress_view:
+            self.progress_view.task_finished(self)
+        else:
+            self.ui_factory._progress_finished(self)
 
     def make_sub_task(self):
-        return ProgressTask(self, self.ui_factory)
+        return ProgressTask(self, ui_factory=self.ui_factory,
+            progress_view=self.progress_view)
 
     def _overall_completion_fraction(self, child_fraction=0.0):
         """Return fractional completion of this task and its parents
@@ -139,7 +157,10 @@
 
     def clear(self):
         # XXX: shouldn't be here; put it in mutter or the ui instead
-        self.ui_factory.clear_term()
+        if self.progress_view:
+            self.progress_view.clear()
+        else:
+            self.ui_factory.clear_term()
 
 
 @deprecated_function(deprecated_in((1, 16, 0)))
@@ -166,6 +187,7 @@
         return _progress_bar_types[requested_bar_type](to_file=to_file, **kwargs)
 
 
+# NOTE: This is also deprecated; you should provide a ProgressView instead.
 class _BaseProgressBar(object):
 
     def __init__(self,
@@ -453,6 +475,8 @@
         #self.to_file.flush()
 
 
+
+# DEPRECATED
 class ChildProgress(_BaseProgressBar):
     """A progress indicator that pushes its data to the parent"""
 

=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2009-06-10 18:58:02 +0000
+++ bzrlib/tests/__init__.py	2009-06-20 08:35:18 +0000
@@ -1325,6 +1325,13 @@
             'BZR_PROGRESS_BAR': None,
             'BZR_LOG': None,
             'BZR_PLUGIN_PATH': None,
+            # Make sure that any text ui tests are consistent regardless of
+            # the environment the test case is run in; you may want tests that
+            # test other combinations.  'dumb' is a reasonable guess for tests
+            # going to a pipe or a StringIO.
+            'TERM': 'dumb',
+            'LINES': '25',
+            'COLUMNS': '80',
             # SSH Agent
             'SSH_AUTH_SOCK': None,
             # Proxies

=== modified file 'bzrlib/tests/test_progress.py'
--- bzrlib/tests/test_progress.py	2009-06-10 03:56:49 +0000
+++ bzrlib/tests/test_progress.py	2009-06-20 08:35:19 +0000
@@ -14,14 +14,21 @@
 # along with this program; if not, write to the Free Software
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
 
+
 import os
 from StringIO import StringIO
 
 from bzrlib import errors
-from bzrlib.tests import TestCase
+from bzrlib.progress import (
+    ProgressTask,
+    )
 from bzrlib.symbol_versioning import (
     deprecated_in,
     )
+from bzrlib.tests import TestCase
+from bzrlib.ui.text import (
+    TextProgressView,
+    )
 
 
 class _TTYStringIO(StringIO):
@@ -36,3 +43,72 @@
 
     def isatty(self):
         return False
+
+
+class TestTextProgressView(TestCase):
+    """Tests for text display of progress bars.
+
+    These try to exercise the progressview independently of its construction,
+    which is arranged by the TextUIFactory.
+    """
+    # The ProgressTask now connects directly to the ProgressView, so we can
+    # check them independently of the factory or of the determination of what
+    # view to use.
+    
+    def make_view(self):
+        out = StringIO()
+        view = TextProgressView(out)
+        view._width = 80
+        return out, view
+    
+    def make_task(self, parent_task, view, msg, curr, total):
+        # would normally be done by UIFactory; is done here so that we don't
+        # have to have one.
+        task = ProgressTask(parent_task, progress_view=view)
+        task.msg = msg
+        task.current_cnt = curr
+        task.total_cnt = total
+        return task
+
+    def test_render_progress_easy(self):
+        """Just one task and one quarter done"""
+        out, view = self.make_view()
+        task = self.make_task(None, view, 'reticulating splines', 5, 20)
+        view.show_progress(task)
+        self.assertEqual(
+'\r[####/               ] reticulating splines 5/20                               \r'
+            , out.getvalue())
+
+    def test_render_progress_nested(self):
+        """Tasks proportionally contribute to overall progress"""
+        out, view = self.make_view()
+        task = self.make_task(None, view, 'reticulating splines', 0, 2)
+        task2 = self.make_task(task, view, 'stage2', 1, 2)
+        view.show_progress(task2)
+        # so we're in the first half of the main task, and half way through
+        # that
+        self.assertEqual(
+r'[####-               ] reticulating splines:stage2 1/2'
+            , view._render_line())
+        # if the nested task is complete, then we're all the way through the
+        # first half of the overall work
+        task2.update('stage2', 2, 2)
+        self.assertEqual(
+r'[#########\          ] reticulating splines:stage2 2/2'
+            , view._render_line())
+
+    def test_render_progress_sub_nested(self):
+        """Intermediate tasks don't mess up calculation."""
+        out, view = self.make_view()
+        task_a = ProgressTask(None, progress_view=view)
+        task_a.update('a', 0, 2)
+        task_b = ProgressTask(task_a, progress_view=view)
+        task_b.update('b')
+        task_c = ProgressTask(task_b, progress_view=view)
+        task_c.update('c', 1, 2)
+        # the top-level task is in its first half; the middle one has no
+        # progress indication, just a label; and the bottom one is half done,
+        # so the overall fraction is 1/4
+        self.assertEqual(
+            r'[####|               ] a:b:c 1/2'
+            , view._render_line())

=== modified file 'bzrlib/tests/test_ui.py'
--- bzrlib/tests/test_ui.py	2009-06-17 05:16:48 +0000
+++ bzrlib/tests/test_ui.py	2009-06-20 08:35:19 +0000
@@ -25,6 +25,9 @@
 
 import bzrlib
 import bzrlib.errors as errors
+from bzrlib.progress import (
+    ProgressTask,
+    )
 from bzrlib.symbol_versioning import (
     deprecated_in,
     )
@@ -33,7 +36,10 @@
     TestUIFactory,
     StringIOWrapper,
     )
-from bzrlib.tests.test_progress import _TTYStringIO
+from bzrlib.tests.test_progress import (
+    _NonTTYStringIO,
+    _TTYStringIO,
+    )
 from bzrlib.ui import (
     CLIUIFactory,
     SilentUIFactory,
@@ -107,22 +113,35 @@
     def test_progress_construction(self):
         """TextUIFactory constructs the right progress view.
         """
+        err = _TTYStringIO()
+        os.environ['TERM'] = 'xterm'
         os.environ['BZR_PROGRESS_BAR'] = 'none'
-        self.assertIsInstance(TextUIFactory()._progress_view,
+        self.assertIsInstance(TextUIFactory(stderr=err)._progress_view,
             NullProgressView)
 
         os.environ['BZR_PROGRESS_BAR'] = 'text'
-        self.assertIsInstance(TextUIFactory()._progress_view,
+        self.assertIsInstance(TextUIFactory(stderr=err)._progress_view,
             TextProgressView)
 
         os.environ['BZR_PROGRESS_BAR'] = 'text'
-        self.assertIsInstance(TextUIFactory()._progress_view,
+        self.assertIsInstance(TextUIFactory(stderr=err)._progress_view,
             TextProgressView)
 
         del os.environ['BZR_PROGRESS_BAR']
-        self.assertIsInstance(TextUIFactory()._progress_view,
+        self.assertIsInstance(TextUIFactory(stderr=err)._progress_view,
             TextProgressView)
 
+        # if not a tty, no progress bars
+        self.assertIsInstance(
+            TextUIFactory(stderr=_NonTTYStringIO())._progress_view,
+            NullProgressView)
+
+        # if a tty but dumb, no progress bars
+        os.environ['TERM'] = 'dumb'
+        self.assertIsInstance(
+            TextUIFactory(stderr=_TTYStringIO())._progress_view,
+            NullProgressView)
+
     def test_progress_note(self):
         stderr = StringIO()
         stdout = StringIO()
@@ -142,14 +161,15 @@
             pb.finished()
 
     def test_progress_note_clears(self):
-        stderr = StringIO()
-        stdout = StringIO()
-        # The PQM redirects the output to a file, so it
-        # defaults to creating a Dots progress bar. we
-        # need to force it to believe we are a TTY
+        stderr = _TTYStringIO()
+        stdout = _TTYStringIO()
+        # so that we get a TextProgressBar
+        os.environ['TERM'] = 'xterm'
         ui_factory = TextUIFactory(
             stdin=StringIO(''),
             stdout=stdout, stderr=stderr)
+        self.assertIsInstance(ui_factory._progress_view,
+            TextProgressView)
         pb = ui_factory.nested_progress_bar()
         try:
             # Create a progress update that isn't throttled
@@ -222,6 +242,7 @@
     def test_text_factory_prompts_and_clears(self):
         # a get_boolean call should clear the pb before prompting
         out = _TTYStringIO()
+        os.environ['TERM'] = 'xterm'
         factory = TextUIFactory(stdin=StringIO("yada\ny\n"), stdout=out, stderr=out)
         pb = factory.nested_progress_bar()
         pb.show_bar = False
@@ -295,61 +316,3 @@
             pb.finished()
 
 
-class TestTextProgressView(TestCase):
-    """Tests for text display of progress bars.
-    """
-    # XXX: These might be a bit easier to write if the rendering and
-    # state-maintaining parts of TextProgressView were more separate, and if
-    # the progress task called back directly to its own view not to the ui
-    # factory. -- mbp 20090312
-    
-    def _make_factory(self):
-        out = StringIO()
-        uif = TextUIFactory(stderr=out)
-        uif._progress_view._width = 80
-        return out, uif
-
-    def test_render_progress_easy(self):
-        """Just one task and one quarter done"""
-        out, uif = self._make_factory()
-        task = uif.nested_progress_bar()
-        task.update('reticulating splines', 5, 20)
-        self.assertEqual(
-'\r[####/               ] reticulating splines 5/20                               \r'
-            , out.getvalue())
-
-    def test_render_progress_nested(self):
-        """Tasks proportionally contribute to overall progress"""
-        out, uif = self._make_factory()
-        task = uif.nested_progress_bar()
-        task.update('reticulating splines', 0, 2)
-        task2 = uif.nested_progress_bar()
-        task2.update('stage2', 1, 2)
-        # so we're in the first half of the main task, and half way through
-        # that
-        self.assertEqual(
-r'[####\               ] reticulating splines:stage2 1/2'
-            , uif._progress_view._render_line())
-        # if the nested task is complete, then we're all the way through the
-        # first half of the overall work
-        task2.update('stage2', 2, 2)
-        self.assertEqual(
-r'[#########|          ] reticulating splines:stage2 2/2'
-            , uif._progress_view._render_line())
-
-    def test_render_progress_sub_nested(self):
-        """Intermediate tasks don't mess up calculation."""
-        out, uif = self._make_factory()
-        task_a = uif.nested_progress_bar()
-        task_a.update('a', 0, 2)
-        task_b = uif.nested_progress_bar()
-        task_b.update('b')
-        task_c = uif.nested_progress_bar()
-        task_c.update('c', 1, 2)
-        # the top-level task is in its first half; the middle one has no
-        # progress indication, just a label; and the bottom one is half done,
-        # so the overall fraction is 1/4
-        self.assertEqual(
-            r'[####|               ] a:b:c 1/2'
-            , uif._progress_view._render_line())
-

=== modified file 'bzrlib/ui/__init__.py'
--- bzrlib/ui/__init__.py	2009-06-10 03:56:49 +0000
+++ bzrlib/ui/__init__.py	2009-06-20 08:35:19 +0000
@@ -19,11 +19,21 @@
 This tells the library how to display things to the user.  Through this
 layer different applications can choose the style of UI.
 
-At the moment this layer is almost trivial: the application can just
-choose the style of progress bar.
-
-Set the ui_factory member to define the behaviour.  The default
-displays no output.
+Several levels are supported, and you can also register new factories such as
+for a GUI.
+
+UIFactory
+    Abstract base class
+
+CLIUIFactory
+    Basic text output, and can read input from the user.
+
+TextUIFactory
+    Can also repaint the screen to draw progress bars.
+
+SilentUIFactory
+    Produces no output and cannot take any input; useful for programs using
+    bzrlib in batch mode or for programs such as loggerhead.
 """
 
 import os
@@ -279,11 +289,15 @@
         cls = CLIUIFactory
     elif not isatty():
         cls = CLIUIFactory
+    # The following case also handles Win32 - on that platform $TERM is
+    # typically never set, so the case None is treated as a smart terminal,
+    # not dumb.  <https://bugs.launchpad.net/bugs/334808>  win32 files do have
+    # isatty methods that return true.
     elif os.environ.get('TERM') in ('dumb', ''):
         # e.g. emacs compile window
         cls = CLIUIFactory
     # User may know better, otherwise default to TextUIFactory
-    if (   os.environ.get('BZR_USE_TEXT_UI', None) is not None
+    if (os.environ.get('BZR_USE_TEXT_UI', None) is not None
         or cls is None):
         from bzrlib.ui.text import TextUIFactory
         cls = TextUIFactory

=== modified file 'bzrlib/ui/text.py'
--- bzrlib/ui/text.py	2009-06-17 05:03:18 +0000
+++ bzrlib/ui/text.py	2009-06-20 08:35:19 +0000
@@ -15,7 +15,6 @@
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
 
 
-
 """Text UI, write output to the console.
 """
 
@@ -71,7 +70,8 @@
         self._progress_view.clear()
 
     def _make_progress_view(self):
-        if os.environ.get('BZR_PROGRESS_BAR') in ('text', None, ''):
+        if (os.environ.get('BZR_PROGRESS_BAR') in ('text', None, '')
+            and progress._supports_progress(self.stderr)):
             return TextProgressView(self.stderr)
         else:
             return NullProgressView()