> +    def test_make_output_stream(self):
> +        # at the moment this is only implemented on text uis; i'm not sure
> +        # what it should do elsewhere
> +        try:
> +            output_stream = self.factory.make_output_stream()
> +        except NotImplementedError, e:
> +            raise tests.TestSkipped(str(e))
> +        output_stream.write('hello!')
> +
> 
> ^- Since Command._setup_outf is now relying on "make_output_stream" are
> they allowed to not implement it?

It does mean that you won't be able to run Command code under any other UIFactory.  I think probably it would still be reasonable to have a totally silent one that just can't run that kind of code.  At least I think it's ok for now.

> 
> 
> 
> +class TestTextUIOutputStream(TestCase):
> +    """Tests for output stream that synchronizes with progress bar."""
> +
> +    def test_output_clears_terminal(self):
> +        stdout = StringIO()
> +        stderr = StringIO()
> +        clear_calls = []
> +
> +        uif = TextUIFactory(None, stdout, stderr)
> +        uif.clear_term = lambda: clear_calls.append('clear')
> +
> +        stream = TextUIOutputStream(uif, uif.stdout)
> +        stream.write("Hello world!\n")
> +        stream.write("there's more...\n")
> +        stream.writelines(["1\n", "2\n", "3\n"])
> +
> +        self.assertEqual(stdout.getvalue(),
> +            "Hello world!\n"
> +            "there's more...\n"
> +            "1\n2\n3\n")
> +        self.assertEqual(['clear', 'clear', 'clear'],
> +            clear_calls)
> +
> +        stream.flush()
> +
> +
> 
> ^- This seems like a reasonable start. Though I personally am much more
> interested in what happens if you pass Unicode bytes to this stream.
> Especially in the "bzr log -p" or "bzr diff" case, which tends to mix
> Unicode bytes with raw content from files.
> 
> I realize you do have an 'encoding_type' flag...

At the moment the encoding type requires you to choose up front between either dealing only in bytes, or dealing only in unicode.  (Or byte strings that contain only ascii can be accepted for either of course.) 

I think eventually you would want to mix them as you outline below, but this is the same constraint the existing code has.

> 
> 
> +        # For whatever reason codecs.getwriter() does not advertise its
> encoding
> +        # it just returns the encoding of the wrapped file, which is
> completely
> +        # bogus. So set the attribute, so we can find the correct
> encoding later.
> +        out_stream = self._make_output_stream_explicit(encoding,
> encoding_type)
> +        if not getattr(out_stream, 'encoding', None):
> +            out_stream.encoding = encoding
> +
> 
> ^- The comment doesn't fit the action. It says that "w =
> codecs.getwriter()" *does* have an 'encoding' attribute, but it returns
> the value of the wrapped object. IOW
> 
> w = codecs.getwriter('utf-8', sys.stdout)
> w.encoding == sys.stdout.encoding
> 
> 
> rather than
> w.encoding == 'utf-8'
> 
> I may be misunderstanding something, though. And if this was the code
> that was originally present, than it makes sense to preserve the behavior.

It was copied code.  I might see if it can just be removed.

> I think the layering is nice. I'm a bit worried about having self.outf
> *and* a bunch of library functions that then grab their own codec
> wrapped stdout, and not having the code paths coordinate.

Me too.  I just wanted to take a step at a time.

> I also think this would be a reasonable time to change the api in a
> larger way. Namely, have the wrapped object have:
> 
>  self.write_bytes('+diff content')
>  self.write_unicode(u'Author:جوجو')
> 
> We could do that in addition to 'self.write' if we wanted to preserve
> compatibility.

I think that would be good; I'm not sure if it should be different methods on the same stream-like object or different objects.

> 
> If we are going to do it, then I think it would be reasonable to create
> an AbstractBaseClass of "OutputStream" that defines what apis should be
> implemented.
> 
> So I'm marking this as "needs_information" because I'd like to have a
> conversation about what you're thinking. Conceptually the code is fine,
> and should be "approve"

I'll have a look at the specific thing of out_stream.encoding and then merge this, then we can look at it more in the list thread about user interaction.