>>>>> "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, in some
cases the animated cursor is correctly handled, in some others is
isn't and can't be (emacs provides a bunch of ways to address the
basic issue of handling the CR, but sometimes there is really no
usable hook). The *easiest*, for an emacs user, is to be able to
set an env variable. Obviously a configuration one will not
address the need.

Other than that, what's your intent when testing None and '' ?

  review: needs_fixing