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.

[Martin Pool (mbp)]

Here's an updated version of this patch. The main functional change is to implement Vincent's suggestion that user settings of environment variables should have effect regardless of what would be autodetected.

Enhance assertIsInstance so you can pass extra explanation.

SilentUI now clearly fails when asked for input rather than looking like it would read from stdin. (I don't think that was ever used or very useful.)

The deprecated bar_type parameter to TextUI was removed.

CLIUIFactory is deprecated and most of the code is fused with TextUIFactory; there's some commentary there on why:

+# Previously, there was CLIUIFactory for dumb terminals, and TextUIFactory for
+# those on smarter terminals. However, there are actually a few independent
+# variables so simple subclassing doesn't make sense. The user may or may not
+# want progress bars, but even if they want them off they may want a rich
+# interface in other ways. And on the other hand input may be redirected from
+# a file so stdin is not a terminal, but we should still try to read input
+# from it and display progress bars etc. Therefore if we're doing a text UI
+# at all, we just use TextUIFactory and it turns some features on or off
+# depending on the detected settings and capabilities.
+#
+# We also use this factory for most blackbox tests, but typically with non-tty
+# streams and TERM=dumb. For api tests, SilentUIFactory is probably
+# appropriate.
+#
+# GUIs may actually choose to subclass TextUIFactory, so unimplemented methods
+# fall back to working through the terminal.
+#
+# SilentUIFactory used to read input from stdin, but not print anything, which
+# now seems like an unhelpful combination. So it will now just fail if asked
+# for input.
+

[Martin Pool (mbp)]

I discovered at least one failure with this change, which is

^^^^[log from bzrlib.tests.bzrdir_implementations.test_bzrdir.TestBreakLock.test_break_lock_branch(BzrDirMetaFormat1)]
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/mbp/bzr/387717-progress-bar-tty/bzrlib/tests/bzrdir_implementations/test_bzrdir.py", line 1835, in test_break_lock_branch
    master.bzrdir.break_lock()
  File "/home/mbp/bzr/387717-progress-bar-tty/bzrlib/bzrdir.py", line 125, in break_lock
    thing_to_unlock.break_lock()
  File "/home/mbp/bzr/387717-progress-bar-tty/bzrlib/branch.py", line 118, in break_lock
    self.control_files.break_lock()
  File "/home/mbp/bzr/387717-progress-bar-tty/bzrlib/lockable_files.py", line 140, in break_lock
    self._lock.break_lock()
  File "/home/mbp/bzr/387717-progress-bar-tty/bzrlib/lockdir.py", line 343, in break_lock
    if bzrlib.ui.ui_factory.get_boolean("Break %s" % lock_info):
  File "/home/mbp/bzr/387717-progress-bar-tty/bzrlib/ui/__init__.py", line 158, in get_boolean
    raise NotImplementedError(self.get_boolean)
NotImplementedError: <bound method SilentUIFactory.get_boolean of <bzrlib.ui.SilentUIFactory object at 0x7bad590>>

[Martin Pool (mbp)]

> I discovered at least one failure with this change, which is

Therefore needs-fixing.

review: Needs Fixing

[Martin Pool (mbp)]

Another attempt at this branch, originally reviewed here: https://code.edge.launchpad.net/~mbp/bzr/387717-progress-bar-tty/+merge/7669

More could be done here including more model/view separation, but this is a large enough patch for now.

[Martin Pool (mbp)]

John asked in the original review

====
@@ -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?
=====

Because there are still a few bits like doing pb.note that need to be detangled first; they can't so clearly be seen as operations only on the model of the progress task.

[Martin Pool (mbp)]

Here's the incremental patch (in two parts, to cut out a merge from trunk):

=== modified file 'NEWS'
--- NEWS 2009-06-19 08:49:14 +0000
+++ NEWS 2009-06-22 10:11:31 +0000
@@ -20,9 +20,11 @@
 * Better message in ``bzr split`` error suggesting a rich root format.
   (Neil Martinsen-Burrell, #220067)

-* 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.
+* The environment variable ``BZR_PROGRESS_BAR`` set to either ``text`` or ``none``
+ always forces progress bars either on or off respectively. Otherwise,
+ they're turned on if ``TERM`` is not ``dumb`` and stderr is a terminal.
+ bzr always uses the 'text' user interface when run as a command, so
+ ``BZR_USE_TEXT_UI`` is no longer needed.
   (Martin Pool, #339385, #387717)

 Internals
@@ -58,6 +60,12 @@
 API Changes
 ***********

+* ``CLIUIFactory`` is deprecated; use ``TextUIFactory`` instead if you
+ need to subclass or create a specific class, or better yet the existing
+ ``make_ui_for_terminal``. ``SilentUIFactory`` is now more clearly
+ defined to raise errors when asked to read input, rather than taking it
+ from stdin. (Martin Pool)
+
 * ProgressTasks now prefer to talk direct to their ProgressView not to the
   UIFactory.
   (Martin Pool)

=== modified file 'bzrlib/progress.py'
--- bzrlib/progress.py 2009-06-19 08:37:24 +0000
+++ bzrlib/progress.py 2009-06-22 10:27:55 +0000
@@ -50,12 +50,15 @@

     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
     if not isatty():
         return False
+ # 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.
     if os.environ.get('TERM') == 'dumb':
         # e.g. emacs compile window
         return False

=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py 2009-06-19 06:49:55 +0000
+++ bzrlib/tests/__init__.py 2009-06-22 09:16:26 +0000
@@ -102,6 +102,7 @@
                           TestLoader,
                           )
 from bzrlib.tests.treeshape import build_tree_contents
+from bzrlib.ui.text import TextUIFactory
 import bzrlib.version_info_formats.format_custom
 from bzrlib.workingtree import WorkingTree, WorkingTreeFormat2

@@ -702,7 +703,7 @@
             return setattr(self._cstring, name, val)

-class TestUIFactory(ui.CLIUIFactory):
+class TestUIFactory(TextUIFactory):
     """A UI Factory for testing.

     Hide the progress bar but emit note()s.
@@ -1093,11 +1094,17 @@
                          osutils.realpath(path2),
                          "apparent paths:\na = %s\nb = %s\n," % (path1, path2))

- def assertIsInstance(self, obj, kls):
- """Fail if obj is not an instance of kls"""
+ def assertIsInstance(self, obj, kls, msg=None):
+ """Fail if obj is not an instance of kls
+ ...

[Martin Pool (mbp)]

Robert did a review on irc, and I've done the points, therefore now submitting this.

review: Approve

=== modified file 'NEWS'
--- NEWS	2009-07-22 01:36:42 +0000
+++ NEWS	2009-07-22 08:35:19 +0000
@@ -30,6 +30,13 @@
 * BranchBuilder now accepts timezone to avoid test failures in countries far
   from GMT. (Vincent Ladeuil, #397716)
 
+* The environment variable ``BZR_PROGRESS_BAR`` set to either ``text`` or ``none``
+  always forces progress bars either on or off respectively.  Otherwise,
+  they're turned on if ``TERM`` is not ``dumb`` and stderr is a terminal.
+  bzr always uses the 'text' user interface when run as a command, so
+  ``BZR_USE_TEXT_UI`` is no longer needed.
+  (Martin Pool, #339385, #387717)
+
 * ``bzr commit`` no longer saves the unversioning of missing files until
   the commit has completed on the branch. This means that aborting a
   commit that found a missing file will leave the tree unedited.
@@ -74,9 +81,20 @@
 API Changes
 ***********
 
+* ``CLIUIFactory`` is deprecated; use ``TextUIFactory`` instead if you
+  need to subclass or create a specific class, or better yet the existing
+  ``make_ui_for_terminal``.  ``SilentUIFactory`` is clarified to do no
+  user interaction at all, rather than trying to read from stdin but not
+  writing any output, which would be strange if reading prompts or
+  passwords.  (Martin Pool)
+
 * New TransformPreview.commit() allows committing without a working tree.
   (Aaron Bentley)
 
+* ProgressTasks now prefer to talk direct to their ProgressView not to the
+  UIFactory. 
+  (Martin Pool)
+
 Internals
 *********
 

=== modified file 'bzrlib/progress.py'
--- bzrlib/progress.py	2009-07-01 10:54:51 +0000
+++ bzrlib/progress.py	2009-07-22 08:35:19 +0000
@@ -37,18 +37,23 @@
     )
 
 
-# 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.
     """
     isatty = getattr(f, 'isatty', None)
     if isatty is None:
         return False
     if not isatty():
         return False
+    # 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.
     if os.environ.get('TERM') == 'dumb':
         # e.g. emacs compile window
         return False
@@ -66,9 +71,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`.
         """
@@ -77,7 +89,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,
@@ -96,16 +110,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
@@ -134,7 +155,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)))
@@ -161,6 +185,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,
@@ -448,6 +473,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-07-20 04:22:47 +0000
+++ bzrlib/tests/__init__.py	2009-07-22 08:35:19 +0000
@@ -102,6 +102,7 @@
                           TestLoader,
                           )
 from bzrlib.tests.treeshape import build_tree_contents
+from bzrlib.ui.text import TextUIFactory
 import bzrlib.version_info_formats.format_custom
 from bzrlib.workingtree import WorkingTree, WorkingTreeFormat2
 
@@ -702,7 +703,7 @@
             return setattr(self._cstring, name, val)
 
 
-class TestUIFactory(ui.CLIUIFactory):
+class TestUIFactory(TextUIFactory):
     """A UI Factory for testing.
 
     Hide the progress bar but emit note()s.
@@ -1093,11 +1094,17 @@
                          osutils.realpath(path2),
                          "apparent paths:\na = %s\nb = %s\n," % (path1, path2))
 
-    def assertIsInstance(self, obj, kls):
-        """Fail if obj is not an instance of kls"""
+    def assertIsInstance(self, obj, kls, msg=None):
+        """Fail if obj is not an instance of kls
+        
+        :param msg: Supplementary message to show if the assertion fails.
+        """
         if not isinstance(obj, kls):
-            self.fail("%r is an instance of %s rather than %s" % (
-                obj, obj.__class__, kls))
+            m = "%r is an instance of %s rather than %s" % (
+                obj, obj.__class__, kls)
+            if msg:
+                m += ": " + msg
+            self.fail(m)
 
     def expectFailure(self, reason, assertion, *args, **kwargs):
         """Invoke a test, expecting it to fail for the given reason.
@@ -1325,6 +1332,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/per_branch/test_break_lock.py'
--- bzrlib/tests/per_branch/test_break_lock.py	2009-07-10 05:49:34 +0000
+++ bzrlib/tests/per_branch/test_break_lock.py	2009-07-22 08:35:20 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2006 Canonical Ltd
+# Copyright (C) 2006, 2009 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -22,6 +22,9 @@
 import bzrlib.errors as errors
 from bzrlib.tests import TestCase, TestCaseWithTransport, TestNotApplicable
 from bzrlib.tests.per_branch.test_branch import TestCaseWithBranch
+from bzrlib.ui import (
+    CannedInputUIFactory,
+    )
 
 
 class TestBreakLock(TestCaseWithBranch):
@@ -35,7 +38,6 @@
         # ours
         self.old_factory = bzrlib.ui.ui_factory
         self.addCleanup(self.restoreFactory)
-        bzrlib.ui.ui_factory = bzrlib.ui.SilentUIFactory()
 
     def restoreFactory(self):
         bzrlib.ui.ui_factory = self.old_factory
@@ -59,7 +61,7 @@
         other_instance = self.branch.repository.bzrdir.open_repository()
         if not other_instance.get_physical_lock_status():
             raise TestNotApplicable("Repository does not lock persistently.")
-        bzrlib.ui.ui_factory.stdin = StringIO("y\n")
+        bzrlib.ui.ui_factory = CannedInputUIFactory([True])
         try:
             self.unused_branch.break_lock()
         except NotImplementedError:
@@ -71,7 +73,7 @@
     def test_locked(self):
         # break_lock when locked should unlock the branch and repo
         self.branch.lock_write()
-        bzrlib.ui.ui_factory.stdin = StringIO("y\ny\n")
+        bzrlib.ui.ui_factory = CannedInputUIFactory([True, True])
         try:
             self.unused_branch.break_lock()
         except NotImplementedError:
@@ -90,7 +92,7 @@
             # this branch does not support binding.
             return
         master.lock_write()
-        bzrlib.ui.ui_factory.stdin = StringIO("y\ny\n")
+        bzrlib.ui.ui_factory = CannedInputUIFactory([True, True])
         try:
             self.unused_branch.break_lock()
         except NotImplementedError:

=== modified file 'bzrlib/tests/per_bzrdir/test_bzrdir.py'
--- bzrlib/tests/per_bzrdir/test_bzrdir.py	2009-07-10 06:45:04 +0000
+++ bzrlib/tests/per_bzrdir/test_bzrdir.py	2009-07-22 08:35:20 +0000
@@ -57,6 +57,9 @@
 from bzrlib.trace import mutter
 from bzrlib.transport import get_transport
 from bzrlib.transport.local import LocalTransport
+from bzrlib.ui import (
+    CannedInputUIFactory,
+    )
 from bzrlib.upgrade import upgrade
 from bzrlib.remote import RemoteBzrDir, RemoteRepository
 from bzrlib.repofmt import weaverepo
@@ -1774,7 +1777,6 @@
         # ours
         self.old_factory = bzrlib.ui.ui_factory
         self.addCleanup(self.restoreFactory)
-        bzrlib.ui.ui_factory = bzrlib.ui.SilentUIFactory()
 
     def restoreFactory(self):
         bzrlib.ui.ui_factory = self.old_factory
@@ -1800,7 +1802,7 @@
             return
         # only one yes needed here: it should only be unlocking
         # the repo
-        bzrlib.ui.ui_factory.stdin = StringIO("y\n")
+        bzrlib.ui.ui_factory = CannedInputUIFactory([True])
         try:
             repo.bzrdir.break_lock()
         except NotImplementedError:
@@ -1831,17 +1833,19 @@
             # two yes's : branch and repository. If the repo in this
             # dir is inappropriately accessed, 3 will be needed, and
             # we'll see that because the stream will be fully consumed
-            bzrlib.ui.ui_factory.stdin = StringIO("y\ny\ny\n")
+            bzrlib.ui.ui_factory = CannedInputUIFactory([True, True, True])
             # determine if the repository will have been locked;
             this_repo_locked = \
                 thisdir.open_repository().get_physical_lock_status()
             master.bzrdir.break_lock()
             if this_repo_locked:
                 # only two ys should have been read
-                self.assertEqual("y\n", bzrlib.ui.ui_factory.stdin.read())
+                self.assertEqual([True],
+                    bzrlib.ui.ui_factory.responses)
             else:
                 # only one y should have been read
-                self.assertEqual("y\ny\n", bzrlib.ui.ui_factory.stdin.read())
+                self.assertEqual([True, True],
+                    bzrlib.ui.ui_factory.responses)
             # we should be able to lock a newly opened branch now
             branch = master.bzrdir.open_branch()
             branch.lock_write()
@@ -1865,7 +1869,7 @@
         tree = self.make_branch_and_tree('.')
         tree.lock_write()
         # three yes's : tree, branch and repository.
-        bzrlib.ui.ui_factory.stdin = StringIO("y\ny\ny\ny\n")
+        bzrlib.ui.ui_factory = CannedInputUIFactory([True, True, True])
         try:
             tree.bzrdir.break_lock()
         except (NotImplementedError, errors.LockActive):
@@ -1875,7 +1879,8 @@
             # object.
             tree.unlock()
             return
-        self.assertEqual("y\n", bzrlib.ui.ui_factory.stdin.read())
+        self.assertEqual([True],
+                bzrlib.ui.ui_factory.responses)
         lock_tree = tree.bzrdir.open_workingtree()
         lock_tree.lock_write()
         lock_tree.unlock()

=== modified file 'bzrlib/tests/per_repository/test_break_lock.py'
--- bzrlib/tests/per_repository/test_break_lock.py	2009-03-23 14:59:43 +0000
+++ bzrlib/tests/per_repository/test_break_lock.py	2009-07-22 08:35:20 +0000
@@ -23,6 +23,9 @@
 from bzrlib.tests.per_repository.test_repository import TestCaseWithRepository
 from bzrlib.transport import get_transport
 from bzrlib.workingtree import WorkingTree
+from bzrlib.ui import (
+    CannedInputUIFactory,
+    )
 
 
 class TestBreakLock(TestCaseWithRepository):
@@ -36,8 +39,7 @@
         # ours
         self.old_factory = bzrlib.ui.ui_factory
         self.addCleanup(self.restoreFactory)
-        bzrlib.ui.ui_factory = bzrlib.ui.SilentUIFactory()
-        bzrlib.ui.ui_factory.stdin = StringIO("y\n")
+        bzrlib.ui.ui_factory = CannedInputUIFactory([True])
 
     def restoreFactory(self):
         bzrlib.ui.ui_factory = self.old_factory

=== modified file 'bzrlib/tests/per_repository_reference/test_break_lock.py'
--- bzrlib/tests/per_repository_reference/test_break_lock.py	2009-03-23 14:59:43 +0000
+++ bzrlib/tests/per_repository_reference/test_break_lock.py	2009-07-22 08:35:20 +0000
@@ -21,6 +21,9 @@
 from bzrlib.tests.per_repository_reference import (
     TestCaseWithExternalReferenceRepository,
     )
+from bzrlib.ui import (
+    CannedInputUIFactory,
+    )
 
 
 class TestBreakLock(TestCaseWithExternalReferenceRepository):
@@ -39,12 +42,8 @@
             # 'lock_write' has not taken a physical mutex out.
             repo.unlock()
             return
-        # we want a UI factory that accepts canned input for the tests:
-        # while SilentUIFactory still accepts stdin, we need to customise
-        # ours
         self.old_factory = bzrlib.ui.ui_factory
         self.addCleanup(self.restoreFactory)
-        bzrlib.ui.ui_factory = bzrlib.ui.SilentUIFactory()
-        bzrlib.ui.ui_factory.stdin = StringIO("y\n")
+        bzrlib.ui.ui_factory = CannedInputUIFactory([True])
         unused_repo.break_lock()
         self.assertRaises(errors.LockBroken, repo.unlock)

=== modified file 'bzrlib/tests/per_workingtree/test_break_lock.py'
--- bzrlib/tests/per_workingtree/test_break_lock.py	2009-07-10 07:14:02 +0000
+++ bzrlib/tests/per_workingtree/test_break_lock.py	2009-07-22 08:35:20 +0000
@@ -30,12 +30,8 @@
         super(TestBreakLock, self).setUp()
         self.unused_workingtree = self.make_branch_and_tree('.')
         self.workingtree = self.unused_workingtree.bzrdir.open_workingtree()
-        # we want a UI factory that accepts canned input for the tests:
-        # while SilentUIFactory still accepts stdin, we need to customise
-        # ours
         self.old_factory = bzrlib.ui.ui_factory
         self.addCleanup(self.restoreFactory)
-        bzrlib.ui.ui_factory = bzrlib.ui.SilentUIFactory()
 
     def restoreFactory(self):
         bzrlib.ui.ui_factory = self.old_factory
@@ -52,16 +48,14 @@
         # if the workingtree isn't locked - and the easiest way
         # to see if that happened is to lock the repo.
         self.workingtree.branch.repository.lock_write()
-        stdin = StringIO("y\n")
-        bzrlib.ui.ui_factory.stdin = stdin
+        bzrlib.ui.ui_factory = bzrlib.ui.CannedInputUIFactory([True])
         try:
             self.unused_workingtree.break_lock()
         except NotImplementedError:
             # workingtree does not support break_lock
             self.workingtree.branch.repository.unlock()
             return
-        remaining = stdin.read()
-        if remaining == 'y\n':
+        if bzrlib.ui.ui_factory.responses == [True]:
             raise TestNotApplicable("repository does not physically lock.")
         self.assertRaises(errors.LockBroken,
             self.workingtree.branch.repository.unlock)
@@ -69,7 +63,7 @@
     def test_locked(self):
         # break_lock when locked should
         self.workingtree.lock_write()
-        bzrlib.ui.ui_factory.stdin = StringIO("y\ny\ny\n")
+        bzrlib.ui.ui_factory = bzrlib.ui.CannedInputUIFactory([True, True, True])
         try:
             self.unused_workingtree.break_lock()
         except (NotImplementedError, errors.LockActive):

=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2009-07-02 08:59:16 +0000
+++ bzrlib/tests/test_config.py	2009-07-22 08:35:19 +0000
@@ -1,5 +1,4 @@
-# Copyright (C) 2005, 2006, 2008 Canonical Ltd
-#   Authors: Robert Collins <[e-mail]>
+# Copyright (C) 2005, 2006, 2008, 2009 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -1561,8 +1560,9 @@
 """))
         entered_password = 'typed-by-hand'
         stdout = tests.StringIOWrapper()
+        stderr = tests.StringIOWrapper()
         ui.ui_factory = tests.TestUIFactory(stdin=entered_password + '\n',
-                                            stdout=stdout)
+                                            stdout=stdout, stderr=stderr)
 
         # Since the password defined in the authentication config is ignored,
         # the user is prompted
@@ -1582,8 +1582,10 @@
 """))
         entered_password = 'typed-by-hand'
         stdout = tests.StringIOWrapper()
+        stderr = tests.StringIOWrapper()
         ui.ui_factory = tests.TestUIFactory(stdin=entered_password + '\n',
-                                            stdout=stdout)
+                                            stdout=stdout,
+                                            stderr=stderr)
 
         # Since the password defined in the authentication config is ignored,
         # the user is prompted

=== modified file 'bzrlib/tests/test_ftp_transport.py'
--- bzrlib/tests/test_ftp_transport.py	2009-04-10 15:58:09 +0000
+++ bzrlib/tests/test_ftp_transport.py	2009-07-22 08:35:19 +0000
@@ -90,27 +90,24 @@
         configuration.""",
         self.get_server().add_user(getpass.getuser(), self.password)
         t = self.get_transport()
-        ui.ui_factory = tests.TestUIFactory(stdin=self.password + '\n',
-                                            stdout=tests.StringIOWrapper())
+        ui.ui_factory = ui.CannedInputUIFactory([self.password])
         # Issue a request to the server to connect
         t.put_bytes('foo', 'test bytes\n')
         self.assertEqual('test bytes\n', t.get_bytes('foo'))
         # Only the password should've been read
-        self.assertEqual('', ui.ui_factory.stdin.readline())
+        ui.ui_factory.assert_all_input_consumed()
 
     def test_prompt_for_password(self):
         t = self.get_transport()
-        ui.ui_factory = tests.TestUIFactory(stdin=self.password+'\n',
-                                            stdout=tests.StringIOWrapper())
+        ui.ui_factory = ui.CannedInputUIFactory([self.password])
         # Issue a request to the server to connect
         t.has('whatever/not/existing')
         # stdin should be empty (the provided password have been consumed)
-        self.assertEqual('', ui.ui_factory.stdin.readline())
+        ui.ui_factory.assert_all_input_consumed()
 
     def test_no_prompt_for_password_when_using_auth_config(self):
         t = self.get_transport()
-        ui.ui_factory = tests.TestUIFactory(stdin='precious\n',
-                                            stdout=tests.StringIOWrapper())
+        ui.ui_factory = ui.CannedInputUIFactory([])
         # Create a config file with the right password
         conf = config.AuthenticationConfig()
         conf._get_config().update({'ftptest': {'scheme': 'ftp',
@@ -120,8 +117,6 @@
         # Issue a request to the server to connect
         t.put_bytes('foo', 'test bytes\n')
         self.assertEqual('test bytes\n', t.get_bytes('foo'))
-        # stdin should have  been left untouched
-        self.assertEqual('precious\n', ui.ui_factory.stdin.readline())
 
     def test_empty_password(self):
         # Override the default user/password from setUp
@@ -129,11 +124,9 @@
         self.password = ''
         self.get_server().add_user(self.user, self.password)
         t = self.get_transport()
-        ui.ui_factory = tests.TestUIFactory(stdin=self.password+'\n',
-                                            stdout=tests.StringIOWrapper())
+        ui.ui_factory = ui.CannedInputUIFactory([self.password])
         # Issue a request to the server to connect
         t.has('whatever/not/existing')
         # stdin should be empty (the provided password have been consumed),
         # even if the password is empty, it's followed by a newline.
-        self.assertEqual('', ui.ui_factory.stdin.readline())
-
+        ui.ui_factory.assert_all_input_consumed()

=== modified file 'bzrlib/tests/test_lockable_files.py'
--- bzrlib/tests/test_lockable_files.py	2009-05-11 07:14:58 +0000
+++ bzrlib/tests/test_lockable_files.py	2009-07-22 08:35:19 +0000
@@ -157,8 +157,7 @@
         l2 = self.get_lockable()
         orig_factory = bzrlib.ui.ui_factory
         # silent ui - no need for stdout
-        bzrlib.ui.ui_factory = bzrlib.ui.SilentUIFactory()
-        bzrlib.ui.ui_factory.stdin = StringIO("y\n")
+        bzrlib.ui.ui_factory = bzrlib.ui.CannedInputUIFactory([True])
         try:
             l2.break_lock()
         finally:

=== modified file 'bzrlib/tests/test_lockdir.py'
--- bzrlib/tests/test_lockdir.py	2009-05-08 15:58:00 +0000
+++ bzrlib/tests/test_lockdir.py	2009-07-22 08:35:19 +0000
@@ -561,9 +561,7 @@
         # do this without IO redirection to ensure it doesn't prompt.
         self.assertRaises(AssertionError, ld1.break_lock)
         orig_factory = bzrlib.ui.ui_factory
-        # silent ui - no need for stdout
-        bzrlib.ui.ui_factory = bzrlib.ui.SilentUIFactory()
-        bzrlib.ui.ui_factory.stdin = StringIO("y\n")
+        bzrlib.ui.ui_factory = bzrlib.ui.CannedInputUIFactory([True])
         try:
             ld2.break_lock()
             self.assertRaises(LockBroken, ld1.unlock)

=== modified file 'bzrlib/tests/test_pack_repository.py'
--- bzrlib/tests/test_pack_repository.py	2009-06-26 09:24:34 +0000
+++ bzrlib/tests/test_pack_repository.py	2009-07-22 08:35:20 +0000
@@ -517,8 +517,7 @@
         def restoreFactory():
             ui.ui_factory = old_factory
         self.addCleanup(restoreFactory)
-        ui.ui_factory = ui.SilentUIFactory()
-        ui.ui_factory.stdin = StringIO("y\n")
+        ui.ui_factory = ui.CannedInputUIFactory([True])
 
     def test_break_lock_breaks_physical_lock(self):
         repo = self.make_repository('.', format=self.get_format())

=== 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-07-22 08:35:20 +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_selftest.py'
--- bzrlib/tests/test_selftest.py	2009-07-20 04:22:47 +0000
+++ bzrlib/tests/test_selftest.py	2009-07-22 08:35:20 +0000
@@ -1687,8 +1687,15 @@
     def test_assert_isinstance(self):
         self.assertIsInstance(2, int)
         self.assertIsInstance(u'', basestring)
-        self.assertRaises(AssertionError, self.assertIsInstance, None, int)
+        e = self.assertRaises(AssertionError, self.assertIsInstance, None, int)
+        self.assertEquals(str(e),
+            "None is an instance of <type 'NoneType'> rather than <type 'int'>")
         self.assertRaises(AssertionError, self.assertIsInstance, 23.3, int)
+        e = self.assertRaises(AssertionError,
+            self.assertIsInstance, None, int, "it's just not")
+        self.assertEquals(str(e),
+            "None is an instance of <type 'NoneType'> rather than <type 'int'>"
+            ": it's just not")
 
     def test_assertEndsWith(self):
         self.assertEndsWith('foo', 'oo')

=== modified file 'bzrlib/tests/test_smtp_connection.py'
--- bzrlib/tests/test_smtp_connection.py	2009-03-25 18:43:31 +0000
+++ bzrlib/tests/test_smtp_connection.py	2009-07-22 08:35:20 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005, 2007 Canonical Ltd
+# Copyright (C) 2005, 2007, 2009 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -136,12 +136,9 @@
                                    smtp_factory=factory)
         self.assertIs(None, conn._smtp_password)
 
-        ui.ui_factory = tests.TestUIFactory(stdin=password + '\n',
-                                            stdout=tests.StringIOWrapper())
+        ui.ui_factory = ui.CannedInputUIFactory([password])
         conn._connect()
         self.assertEqual(password, conn._smtp_password)
-        # stdin should be empty (the provided password have been consumed)
-        self.assertEqual('', ui.ui_factory.stdin.readline())
 
     def test_smtp_password_from_auth_config(self):
         user = 'joe'

=== modified file 'bzrlib/tests/test_ui.py'
--- bzrlib/tests/test_ui.py	2009-07-02 09:09:35 +0000
+++ bzrlib/tests/test_ui.py	2009-07-22 08:35:20 +0000
@@ -31,7 +31,22 @@
 from bzrlib.symbol_versioning import (
     deprecated_in,
     )
-from bzrlib.tests.test_progress import _TTYStringIO
+from bzrlib.tests import (
+    TestCase,
+    TestUIFactory,
+    StringIOWrapper,
+    )
+from bzrlib.tests.test_progress import (
+    _NonTTYStringIO,
+    _TTYStringIO,
+    )
+from bzrlib.ui import (
+    CannedInputUIFactory,
+    CLIUIFactory,
+    SilentUIFactory,
+    UIFactory,
+    make_ui_for_terminal,
+    )
 from bzrlib.ui.text import (
     NullProgressView,
     TextProgressView,
@@ -41,20 +56,6 @@
 
 class UITests(tests.TestCase):
 
-    def test_silent_factory(self):
-        ui = _mod_ui.SilentUIFactory()
-        stdout = StringIO()
-        self.assertEqual(None,
-                         self.apply_redirected(None, stdout, stdout,
-                                               ui.get_password))
-        self.assertEqual('', stdout.getvalue())
-        self.assertEqual(None,
-                         self.apply_redirected(None, stdout, stdout,
-                                               ui.get_password,
-                                               u'Hello\u1234 %(user)s',
-                                               user=u'some\u1234'))
-        self.assertEqual('', stdout.getvalue())
-
     def test_text_factory_ascii_password(self):
         ui = tests.TestUIFactory(stdin='secret\n',
                                  stdout=tests.StringIOWrapper(),
@@ -102,21 +103,52 @@
     def test_progress_construction(self):
         """TextUIFactory constructs the right progress view.
         """
-        os.environ['BZR_PROGRESS_BAR'] = 'none'
-        self.assertIsInstance(TextUIFactory()._progress_view,
-            NullProgressView)
-
-        os.environ['BZR_PROGRESS_BAR'] = 'text'
-        self.assertIsInstance(TextUIFactory()._progress_view,
-            TextProgressView)
-
-        os.environ['BZR_PROGRESS_BAR'] = 'text'
-        self.assertIsInstance(TextUIFactory()._progress_view,
-            TextProgressView)
-
-        del os.environ['BZR_PROGRESS_BAR']
-        self.assertIsInstance(TextUIFactory()._progress_view,
-            TextProgressView)
+        for (file_class, term, pb, expected_pb_class) in (
+            # on an xterm, either use them or not as the user requests,
+            # otherwise default on
+            (_TTYStringIO, 'xterm', 'none', NullProgressView),
+            (_TTYStringIO, 'xterm', 'text', TextProgressView),
+            (_TTYStringIO, 'xterm', None, TextProgressView),
+            # on a dumb terminal, again if there's explicit configuration do
+            # it, otherwise default off
+            (_TTYStringIO, 'dumb', 'none', NullProgressView),
+            (_TTYStringIO, 'dumb', 'text', TextProgressView),
+            (_TTYStringIO, 'dumb', None, NullProgressView),
+            # on a non-tty terminal, it's null regardless of $TERM
+            (StringIO, 'xterm', None, NullProgressView),
+            (StringIO, 'dumb', None, NullProgressView),
+            # however, it can still be forced on
+            (StringIO, 'dumb', 'text', TextProgressView),
+            ):
+            os.environ['TERM'] = term
+            if pb is None:
+                if 'BZR_PROGRESS_BAR' in os.environ:
+                    del os.environ['BZR_PROGRESS_BAR']
+            else:
+                os.environ['BZR_PROGRESS_BAR'] = pb
+            stdin = file_class('')
+            stderr = file_class()
+            stdout = file_class()
+            uif = make_ui_for_terminal(stdin, stdout, stderr)
+            self.assertIsInstance(uif, TextUIFactory,
+                "TERM=%s BZR_PROGRESS_BAR=%s uif=%r" % (term, pb, uif,))
+            self.assertIsInstance(uif.make_progress_view(),
+                expected_pb_class,
+                "TERM=%s BZR_PROGRESS_BAR=%s uif=%r" % (term, pb, uif,))
+
+    def test_text_ui_non_terminal(self):
+        """Even on non-ttys, make_ui_for_terminal gives a text ui."""
+        stdin = _NonTTYStringIO('')
+        stderr = _NonTTYStringIO()
+        stdout = _NonTTYStringIO()
+        for term_type in ['dumb', None, 'xterm']:
+            if term_type is None:
+                del os.environ['TERM']
+            else:
+                os.environ['TERM'] = term_type
+            uif = make_ui_for_terminal(stdin, stdout, stderr)
+            self.assertIsInstance(uif, TextUIFactory,
+                'TERM=%r' % (term_type,))
 
     def test_progress_note(self):
         stderr = StringIO()
@@ -137,14 +169,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
@@ -173,17 +206,17 @@
         pb2.finished()
         pb1.finished()
 
-    def assert_get_bool_acceptance_of_user_input(self, factory):
-        factory.stdin = StringIO("y\n" # True
-                                 "n\n" # False
-                                 "yes with garbage\nY\n" # True
-                                 "not an answer\nno\n" # False
-                                 "I'm sure!\nyes\n" # True
-                                 "NO\n" # False
-                                 "foo\n")
-        factory.stdout = StringIO()
-        factory.stderr = StringIO()
-        # there is no output from the base factory
+    def test_text_ui_get_boolean(self):
+        stdin = StringIO("y\n" # True
+                         "n\n" # False
+                         "yes with garbage\nY\n" # True
+                         "not an answer\nno\n" # False
+                         "I'm sure!\nyes\n" # True
+                         "NO\n" # False
+                         "foo\n")
+        stdout = StringIO()
+        stderr = StringIO()
+        factory = TextUIFactory(stdin, stdout, stderr)
         self.assertEqual(True, factory.get_boolean(""))
         self.assertEqual(False, factory.get_boolean(""))
         self.assertEqual(True, factory.get_boolean(""))
@@ -194,28 +227,9 @@
         # stdin should be empty
         self.assertEqual('', factory.stdin.readline())
 
-    def test_silent_ui_getbool(self):
-        factory = _mod_ui.SilentUIFactory()
-        self.assert_get_bool_acceptance_of_user_input(factory)
-
-    def test_silent_factory_prompts_silently(self):
-        factory = _mod_ui.SilentUIFactory()
-        stdout = StringIO()
-        factory.stdin = StringIO("y\n")
-        self.assertEqual(True,
-                         self.apply_redirected(None, stdout, stdout,
-                                               factory.get_boolean, "foo"))
-        self.assertEqual("", stdout.getvalue())
-        # stdin should be empty
-        self.assertEqual('', factory.stdin.readline())
-
-    def test_text_ui_getbool(self):
-        factory = TextUIFactory(None, None, None)
-        self.assert_get_bool_acceptance_of_user_input(factory)
-
     def test_text_factory_prompt(self):
         # see <https://launchpad.net/bugs/365891>
-        factory = TextUIFactory(None, StringIO(), StringIO(), StringIO())
+        factory = TextUIFactory(StringIO(), StringIO(), StringIO())
         factory.prompt('foo %2e')
         self.assertEqual('', factory.stdout.getvalue())
         self.assertEqual('foo %2e', factory.stderr.getvalue())
@@ -223,8 +237,8 @@
     def test_text_factory_prompts_and_clears(self):
         # a get_boolean call should clear the pb before prompting
         out = _TTYStringIO()
-        factory = TextUIFactory(stdin=StringIO("yada\ny\n"),
-                                stdout=out, stderr=out)
+        os.environ['TERM'] = 'xterm'
+        factory = TextUIFactory(stdin=StringIO("yada\ny\n"), stdout=out, stderr=out)
         pb = factory.nested_progress_bar()
         pb.show_bar = False
         pb.show_spinner = False
@@ -254,17 +268,6 @@
         finally:
             pb.finished()
 
-    def test_silent_ui_getusername(self):
-        factory = _mod_ui.SilentUIFactory()
-        factory.stdin = StringIO("someuser\n\n")
-        factory.stdout = StringIO()
-        factory.stderr = StringIO()
-        self.assertEquals(None,
-            factory.get_username(u'Hello\u1234 %(host)s', host=u'some\u1234'))
-        self.assertEquals("", factory.stdout.getvalue())
-        self.assertEquals("", factory.stderr.getvalue())
-        self.assertEquals("someuser\n\n", factory.stdin.getvalue())
-
     def test_text_ui_getusername(self):
         factory = TextUIFactory(None, None, None)
         factory.stdin = StringIO("someuser\n\n")
@@ -298,63 +301,47 @@
             pb.finished()
 
 
-class TestTextProgressView(tests.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
+class CLIUITests(TestCase):
+
+    def test_cli_factory_deprecated(self):
+        uif = self.applyDeprecated(deprecated_in((1, 18, 0)),
+            CLIUIFactory,
+            StringIO(), StringIO(), StringIO())
+        self.assertIsInstance(uif, UIFactory)
+
+
+class SilentUITests(TestCase):
+
+    def test_silent_factory_get_password(self):
+        # A silent factory that can't do user interaction can't get a
+        # password.  Possibly it should raise a more specific error but it
+        # can't succeed.
+        ui = SilentUIFactory()
+        stdout = StringIO()
+        self.assertRaises(
+            NotImplementedError,
+            self.apply_redirected,
+            None, stdout, stdout, ui.get_password)
+        # and it didn't write anything out either
+        self.assertEqual('', stdout.getvalue())
+
+    def test_silent_ui_getbool(self):
+        factory = SilentUIFactory()
+        stdout = StringIO()
+        self.assertRaises(
+            NotImplementedError,
+            self.apply_redirected,
+            None, stdout, stdout, factory.get_boolean, "foo")
+
+
+class CannedInputUIFactoryTests(TestCase):
     
-    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())
+    def test_canned_input_get_input(self):
+        uif = CannedInputUIFactory([True, 'mbp', 'password'])
+        self.assertEqual(uif.get_boolean('Extra cheese?'), True)
+        self.assertEqual(uif.get_username('Enter your user name'), 'mbp')
+        self.assertEqual(uif.get_password('Password for %(host)s', host='example.com'),
+            'password')
 
 
 class TestBoolFromString(tests.TestCase):

=== modified file 'bzrlib/ui/__init__.py'
--- bzrlib/ui/__init__.py	2009-07-02 09:09:35 +0000
+++ bzrlib/ui/__init__.py	2009-07-22 08:35:20 +0000
@@ -14,18 +14,34 @@
 # along with this program; if not, write to the Free Software
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
 
-"""UI abstraction.
-
-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.
+"""Abstraction for interacting with the user.
+
+Applications can choose different types of UI, and they deal with displaying
+messages or progress to the user, and with gathering different types of input.
+
+Several levels are supported, and you can also register new factories such as
+for a GUI.
+
+UIFactory
+    Semi-abstract base class
+
+SilentUIFactory
+    Produces no output and cannot take any input; useful for programs using
+    bzrlib in batch mode or for programs such as loggerhead.
+
+CannedInputUIFactory
+    For use in testing; the input values to be returned are provided 
+    at construction.
+
+TextUIFactory
+    Standard text command-line interface, with stdin, stdout, stderr.
+    May make more or less advanced use of them, eg in drawing progress bars,
+    depending on the detected capabilities of the terminal.
+    GUIs may choose to subclass this so that unimplemented methods fall
+    back to working through the terminal.
 """
 
+
 import os
 import sys
 import warnings
@@ -41,6 +57,11 @@
     trace,
     )
 """)
+from bzrlib.symbol_versioning import (
+    deprecated_function,
+    deprecated_in,
+    deprecated_method,
+    )
 
 
 _valid_boolean_strings = dict(yes=True, no=False,
@@ -158,6 +179,14 @@
         """
         raise NotImplementedError(self.get_boolean)
 
+    def make_progress_view(self):
+        """Construct a new ProgressView object for this UI.
+
+        Application code should normally not call this but instead
+        nested_progress_bar().
+        """
+        return NullProgressView()
+
     def recommend_upgrade(self,
         current_format_name,
         basedir):
@@ -182,10 +211,9 @@
 
 
 class CLIUIFactory(UIFactory):
-    """Common behaviour for command line UI factories.
-
-    This is suitable for dumb terminals that can't repaint existing text."""
-
+    """Deprecated in favor of TextUIFactory."""
+
+    @deprecated_method(deprecated_in((1, 18, 0)))
     def __init__(self, stdin=None, stdout=None, stderr=None):
         UIFactory.__init__(self)
         self.stdin = stdin or sys.stdin
@@ -271,28 +299,51 @@
         self.stdout.write(msg + '\n')
 
 
-class SilentUIFactory(CLIUIFactory):
+class SilentUIFactory(UIFactory):
     """A UI Factory which never prints anything.
 
-    This is the default UI, if another one is never registered.
+    This is the default UI, if another one is never registered by a program
+    using bzrlib, and it's also active for example inside 'bzr serve'.
+
+    Methods that try to read from the user raise an error; methods that do
+    output do nothing.
     """
 
     def __init__(self):
-        CLIUIFactory.__init__(self)
+        UIFactory.__init__(self)
+
+    def note(self, msg):
+        pass
+
+    def get_username(self, prompt, **kwargs):
+        return None
+
+
+class CannedInputUIFactory(SilentUIFactory):
+    """A silent UI that return canned input."""
+
+    def __init__(self, responses):
+        self.responses = responses
+
+    def __repr__(self):
+        return "%s(%r)" % (self.__class__.__name__, self.responses)
+
+    def get_boolean(self, prompt):
+        return self.responses.pop(0)
 
     def get_password(self, prompt='', **kwargs):
-        return None
-
-    def get_username(self, prompt='', **kwargs):
-        return None
-
-    def prompt(self, prompt, **kwargs):
-        pass
-
-    def note(self, msg):
-        pass
-
-
+        return self.responses.pop(0)
+
+    def get_username(self, prompt, **kwargs):
+        return self.responses.pop(0)
+    
+    def assert_all_input_consumed(self):
+        if self.responses:
+            raise AssertionError("expected all input in %r to be consumed"
+                % (self,))
+
+
+@deprecated_function(deprecated_in((1, 18, 0)))
 def clear_decorator(func, *args, **kwargs):
     """Decorator that clears the term"""
     ui_factory.clear_term()
@@ -300,28 +351,27 @@
 
 
 ui_factory = SilentUIFactory()
-"""IMPORTANT: never import this symbol directly. ONLY ever access it as
-ui.ui_factory."""
+# IMPORTANT: never import this symbol directly. ONLY ever access it as
+# ui.ui_factory, so that you refer to the current value.
 
 
 def make_ui_for_terminal(stdin, stdout, stderr):
     """Construct and return a suitable UIFactory for a text mode program.
-
-    If stdout is a smart terminal, this gets a smart UIFactory with
-    progress indicators, etc.  If it's a dumb terminal, just plain text output.
     """
-    cls = None
-    isatty = getattr(stdin, 'isatty', None)
-    if isatty is None:
-        cls = CLIUIFactory
-    elif not isatty():
-        cls = CLIUIFactory
-    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
-        or cls is None):
-        from bzrlib.ui.text import TextUIFactory
-        cls = TextUIFactory
-    return cls(stdin=stdin, stdout=stdout, stderr=stderr)
+    # this is now always TextUIFactory, which in turn decides whether it
+    # should display progress bars etc
+    from bzrlib.ui.text import TextUIFactory
+    return TextUIFactory(stdin, stdout, stderr)
+
+
+class NullProgressView(object):
+    """Soak up and ignore progress information."""
+
+    def clear(self):
+        pass
+
+    def show_progress(self, task):
+        pass
+
+    def show_transport_activity(self, transport, direction, byte_count):
+        pass

=== modified file 'bzrlib/ui/text.py'
--- bzrlib/ui/text.py	2009-06-26 03:29:57 +0000
+++ bzrlib/ui/text.py	2009-07-22 08:35:20 +0000
@@ -15,7 +15,6 @@
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
 
 
-
 """Text UI, write output to the console.
 """
 
@@ -34,14 +33,16 @@
 
 """)
 
-from bzrlib.ui import CLIUIFactory
-
-
-class TextUIFactory(CLIUIFactory):
+from bzrlib.ui import (
+    UIFactory,
+    NullProgressView,
+    )
+
+
+class TextUIFactory(UIFactory):
     """A UI factory for Text user interefaces."""
 
     def __init__(self,
-                 bar_type=None,
                  stdin=None,
                  stdout=None,
                  stderr=None):
@@ -51,13 +52,14 @@
                          letting the bzrlib.progress.ProgressBar factory auto
                          select.   Deprecated.
         """
-        super(TextUIFactory, self).__init__(stdin=stdin,
-                stdout=stdout, stderr=stderr)
-        if bar_type:
-            symbol_versioning.warn(symbol_versioning.deprecated_in((1, 11, 0))
-                % "bar_type parameter")
+        super(TextUIFactory, self).__init__()
+        # TODO: there's no good reason not to pass all three streams, maybe we
+        # should deprecate the default values...
+        self.stdin = stdin
+        self.stdout = stdout
+        self.stderr = stderr
         # paints progress, network activity, etc
-        self._progress_view = self._make_progress_view()
+        self._progress_view = self.make_progress_view()
         
     def clear_term(self):
         """Prepare the terminal for output.
@@ -70,8 +72,78 @@
         # to clear it.  We might need to separately check for the case of
         self._progress_view.clear()
 
-    def _make_progress_view(self):
-        if os.environ.get('BZR_PROGRESS_BAR') in ('text', None, ''):
+    def get_boolean(self, prompt):
+        while True:
+            self.prompt(prompt + "? [y/n]: ")
+            line = self.stdin.readline().lower()
+            if line in ('y\n', 'yes\n'):
+                return True
+            elif line in ('n\n', 'no\n'):
+                return False
+            elif line in ('', None):
+                # end-of-file; possibly should raise an error here instead
+                return None
+
+    def get_non_echoed_password(self):
+        isatty = getattr(self.stdin, 'isatty', None)
+        if isatty is not None and isatty():
+            # getpass() ensure the password is not echoed and other
+            # cross-platform niceties
+            password = getpass.getpass('')
+        else:
+            # echo doesn't make sense without a terminal
+            password = self.stdin.readline()
+            if not password:
+                password = None
+            elif password[-1] == '\n':
+                password = password[:-1]
+        return password
+
+    def get_password(self, prompt='', **kwargs):
+        """Prompt the user for a password.
+
+        :param prompt: The prompt to present the user
+        :param kwargs: Arguments which will be expanded into the prompt.
+                       This lets front ends display different things if
+                       they so choose.
+        :return: The password string, return None if the user
+                 canceled the request.
+        """
+        prompt += ': '
+        self.prompt(prompt, **kwargs)
+        # There's currently no way to say 'i decline to enter a password'
+        # as opposed to 'my password is empty' -- does it matter?
+        return self.get_non_echoed_password()
+
+    def get_username(self, prompt, **kwargs):
+        """Prompt the user for a username.
+
+        :param prompt: The prompt to present the user
+        :param kwargs: Arguments which will be expanded into the prompt.
+                       This lets front ends display different things if
+                       they so choose.
+        :return: The username string, return None if the user
+                 canceled the request.
+        """
+        prompt += ': '
+        self.prompt(prompt, **kwargs)
+        username = self.stdin.readline()
+        if not username:
+            username = None
+        elif username[-1] == '\n':
+            username = username[:-1]
+        return username
+
+    def make_progress_view(self):
+        """Construct and return a new ProgressView subclass for this UI.
+        """
+        # if the user specifically requests either text or no progress bars,
+        # always do that.  otherwise, guess based on $TERM and tty presence.
+        if os.environ.get('BZR_PROGRESS_BAR') == 'text':
+            return TextProgressView(self.stderr)
+        elif os.environ.get('BZR_PROGRESS_BAR') == 'none':
+            return NullProgressView()
+        elif progress._supports_progress(self.stderr):
             return TextProgressView(self.stderr)
         else:
             return NullProgressView()
@@ -81,6 +153,19 @@
         self.clear_term()
         self.stdout.write(msg + '\n')
 
+    def prompt(self, prompt, **kwargs):
+        """Emit prompt on the CLI.
+        
+        :param kwargs: Dictionary of arguments to insert into the prompt,
+            to allow UIs to reformat the prompt.
+        """
+        if kwargs:
+            # See <https://launchpad.net/bugs/365891>
+            prompt = prompt % kwargs
+        prompt = prompt.encode(osutils.get_terminal_encoding(), 'replace')
+        self.clear_term()
+        self.stderr.write(prompt)
+
     def report_transport_activity(self, transport, byte_count, direction):
         """Called by transports as they do IO.
 
@@ -105,19 +190,6 @@
         self._progress_view.clear()
 
 
-class NullProgressView(object):
-    """Soak up and ignore progress information."""
-
-    def clear(self):
-        pass
-
-    def show_progress(self, task):
-        pass
-
-    def show_transport_activity(self, transport, direction, byte_count):
-        pass
-    
-
 class TextProgressView(object):
     """Display of progress bar and other information on a tty.