Code review comment for lp:~mbp/bzr/387717-progress-bar-tty

>>>>> "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.
    >>
    >> 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).

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

Sure, as said above I hit the tiny (even smaller) red button ;-)

<snip/>
    >> 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, '')

<snip/>

    martin> I don't think I broke that, did I? They could still set =none.

Yes they could still set =none, but you don't respect =text
anymore, that's the problem ;-) *I* want progress when I'm in an
emacs shell !

IME, there are three use cases under emacs:
- interactive shells: need progress,
- shell commands: doesn't need progress,
- hidden sub-processes: should not use progress.

'hidden sub-processes' is vague on purpose, it covers all cases
where bzr is run without any way to specify any env variables, so
in *that* case, I'm glad your automatic detection select no
progress report ;-)

I managed to select between the first two with:

# If we are inside an emacs shell, tell bzr it can use a text UI
# an a tty progress bar
emacs_shell=`echo $INSIDE_EMACS | grep comint`
if [ "$emacs_shell" != "" ] ; then
    export BZR_USE_TEXT_UI=1
    export BZR_PROGRESS_BAR=text
fi

in my .bashrc.

That's hackish, but it works.

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

    martin> If the variable is not set (ie None), we should
    martin> default to showing it if the terminal can show it.
    martin> For '' it's arguable, but istr in some situations
    martin> it's hard to delete rather than clearing environment
    martin> variables, so it's better to treat '' the same as
    martin> unset. (I can't think at the moment of a plausible
    martin> specific case.)

Ok, now it's clear, I'm sure I will ask you again in couple of
months if you don't put a comment there though ;-D

I can see it being useful when people are messing around making
various attempts to find the right setup. I think *I* will
finally cleanup up such setup myself, but having bzr do the right
thing here make sense.

      Vincent