Merge into bzr.dev : progress-output : Code : Bazaar

Reviewer	Date Requested	Status
John A Meinel	2009-11-17	Approve on 2009-12-08
Ian Clatworthy		Approve on 2009-12-07
Vincent Ladeuil	2009-12-08	Pending
Review via email: mp+14944@code.launchpad.net

Revision history for this message

Martin Pool (mbp) wrote on 2009-11-17:

#

This conceptually relates to <https://code.edge.launchpad.net/~mbp/bzr/remove-logging/+merge/14942> but doesn't technically depend on it.

+* New API ``ui_factory.make_output_stream`` to be used for sending bulk
+ (rather than user-interaction) data to stdout. This automatically
+ coordinates with progress bars or other terminal activity, and can be
+ overridden by GUIs.
+ (Martin Pool)
+

The idea is to

* Allow us to show transport activity whenever it happens, without clobbering the output of eg log - this doesn't actually restore this behaviour, but it gets ready for it.

* Move away from Command.outf, which assumes that it's only the command layer that knows about stdout. In fact, it's generally a layer or two down, and passing what's really a global is poor.

* Also avoid code defaulting directly to sys.stdout.

* In particular, put the unicode-handling features of outf into a more reusable place.

* Give GUIs a way to capture bulk output of application-level commands to show in a window.

Command.outf still works and redirects into this.

Revision history for this message

John A Meinel (jameinel) wrote on 2009-11-17:

#

Download full text (4.7 KiB)

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

Martin Pool wrote:
> Martin Pool has proposed merging lp:~mbp/bzr/progress-output into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
>
> This conceptually relates to <https://code.edge.launchpad.net/~mbp/bzr/remove-logging/+merge/14942> but doesn't technically depend on it.
>
> +* New API ``ui_factory.make_output_stream`` to be used for sending bulk
> + (rather than user-interaction) data to stdout. This automatically
> + coordinates with progress bars or other terminal activity, and can be
> + overridden by GUIs.
> + (Martin Pool)
> +
>
> The idea is to
>
> * Allow us to show transport activity whenever it happens, without clobbering the output of eg log - this doesn't actually restore this behaviour, but it gets ready for it.
>
> * Move away from Command.outf, which assumes that it's only the command layer that knows about stdout. In fact, it's generally a layer or two down, and passing what's really a global is poor.
>
> * Also avoid code defaulting directly to sys.stdout.
>
> * In particular, put the unicode-handling features of outf into a more reusable place.
>
> * Give GUIs a way to capture bulk output of application-level commands to show in a window.
>
> Command.outf still works and redirects into this.
>

+ 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?

+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...

+ # 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(enc...

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

Martin Pool wrote:
> Martin Pool has proposed merging lp:~mbp/bzr/progress-output into lp:bzr.
> 
>     Requested reviews:
>     bzr-core (bzr-core)
> 
> 
> This conceptually relates to <https://code.edge.launchpad.net/~mbp/bzr/remove-logging/+merge/14942> but doesn't technically depend on it.
>  
> +* New API ``ui_factory.make_output_stream`` to be used for sending bulk
> +  (rather than user-interaction) data to stdout.  This automatically
> +  coordinates with progress bars or other terminal activity, and can be
> +  overridden by GUIs.
> +  (Martin Pool)
> +
> 
> The idea is to 
> 
>  * Allow us to show transport activity whenever it happens, without clobbering the output of eg log - this doesn't actually restore this behaviour, but it gets ready for it.
> 
>  * Move away from Command.outf, which assumes that it's only the command layer that knows about stdout.  In fact, it's generally a layer or two down, and passing what's really a global is poor.
> 
>  * Also avoid code defaulting directly to sys.stdout.
> 
>  * In particular, put the unicode-handling features of outf into a more reusable place.
> 
>  * Give GUIs a way to capture bulk output of application-level commands to show in a window.
> 
> Command.outf still works and redirects into this.
>

+    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?

+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...

+        # 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.

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.

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.

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"

review: needs_information

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

iEYEARECAAYFAksCwYwACgkQJdeBCYSNAAP66ACfTJXnb+XFWV7f3bTa4ipUKmW1
rFsAni4FckHnOxFi4n/xmE/AoaCzwHbz
=cveG
-----END PGP SIGNATURE-----

review: Needs Information

Revision history for this message

Gordon Tyler (doxxx) wrote on 2009-11-21:

#

I'd like to see this move forward since I would be making use of the updated UIFactory for my output-cleanup branch.

Revision history for this message

Martin Pool (mbp) wrote on 2009-11-27:

#

Download full text (4.3 KiB)

> + 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 re...

> +    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.

Revision history for this message

Martin Pool (mbp) wrote on 2009-11-30:

#

This has a test failure in a non_ascii diff test. I thought it had all passed before, but perhaps not.

Revision history for this message

John A Meinel (jameinel) wrote on 2009-12-02:

#

Does that mean we should be bumping it back to "Work In Progress"?

I guess my concern is that if we set it as such, then it will never get seen again...

Revision history for this message

Ian Clatworthy (ian-clatworthy) wrote on 2009-12-07:

#

This looks clean enough to merge to me. John raises a bunch of interesting questions though so please double check he's happy before landing it.

My only comment is that write() and writelines() could do with docstrings mentioning that the caller is responsible for providing newlines for each line.

review: Approve

Revision history for this message

Gordon Tyler (doxxx) wrote on 2009-12-07:

#

> My only comment is that write() and writelines() could do with docstrings
> mentioning that the caller is responsible for providing newlines for each
> line.

My first reaction was that this sounds like highly unintuitive API. But then I went and read up on Python's file object API and it seems it does the same thing. So I guess it's expected then.

Revision history for this message

Martin Pool (mbp) wrote on 2009-12-08:

#

Back to wip; in-progress bug https://bugs.edge.launchpad.net/bzr/+bug/493944

Revision history for this message

Martin Pool (mbp) wrote on 2009-12-08:

#

@john I moved the tweak for codecs.getwriter.encoding to the place it's actually constructed and made it unconditional on the assumption the comment is correct.

The failure I'm seeing now is the same as in <https://bugs.edge.launchpad.net/bzr/+bug/328007> (or specifically the test for that) and in the comments on that bug you ask for the ability to mix 'exact' and 'replace' hunks within the output stream in the way this may eventually let you do.

So this is a bit annoying, but possibly also a chance to fix that bug more cleanly.

That bug has a somewhat hackish fix based on peeking inside EncodingWriters. I'm wondering if we should actually pass in an exact encoding file stream that it can use for the diffs, which would make things incrementally cleaner.

Revision history for this message

Martin Pool (mbp) wrote on 2009-12-08:

#

Looking at that test and bug: this is a clear case where we want to mix unicode and binary output. If you have different objects you (perhaps) need to synchronize them by flushing manually; if you have one object it may still need to flush the codec writers but it could be hidden from more callers.

If it was write_bytes() vs write_unicode() it would be more obvious in the code whether people were using the right one, vs needing to work out what kind of stream they have.

unescape_for_display is another interesting case - at the moment it needs to be told the destination encoding; it could instead become a method on the stream.

Revision history for this message

Martin Pool (mbp) wrote on 2009-12-08:

#

Download full text (4.8 KiB)

Quick re-review requested; here's the incremental diff

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py 2009-11-04 22:32:13 +0000
+++ bzrlib/builtins.py 2009-12-08 09:06:02 +0000
@@ -2323,7 +2323,10 @@
             # Build the log formatter
             if log_format is None:
                 log_format = log.log_formatter_registry.get_default(b)
+ # Make a non-encoding output to include the diffs - bug 328007
+ unencoded_output = ui.ui_factory.make_output_stream(encoding_type='exact')
             lf = log_format(show_ids=show_ids, to_file=self.outf,
+ to_exact_file=unencoded_output,
                             show_timezone=timezone,
                             delta_format=get_verbosity_level(),
                             levels=levels,

=== modified file 'bzrlib/log.py'
--- bzrlib/log.py 2009-10-29 03:32:42 +0000
+++ bzrlib/log.py 2009-12-08 09:17:21 +0000
@@ -1291,10 +1291,13 @@
preferred_levels = 0

def __init__(self, to_file, show_ids=False, show_timezone='original',
- delta_format=None, levels=None, show_advice=False):
+ delta_format=None, levels=None, show_advice=False,
+ to_exact_file=None):
"""Create a LogFormatter.

         :param to_file: the file to output to
+ :param to_exact_file: if set, gives an output stream to which
+ non-Unicode diffs are written.
         :param show_ids: if True, revision-ids are to be displayed
         :param show_timezone: the timezone to use
         :param delta_format: the level of delta information to display
@@ -1307,7 +1310,13 @@
         self.to_file = to_file
         # 'exact' stream used to show diff, it should print content 'as is'
         # and should not try to decode/encode it to unicode to avoid bug #328007
- self.to_exact_file = getattr(to_file, 'stream', to_file)
+ if to_exact_file is not None:
+ self.to_exact_file = to_exact_file
+ else:
+ # XXX: somewhat hacky; this assumes it's a codec writer; it's better
+ # for code that expects to get diffs to pass in the exact file
+ # stream
+ self.to_exact_file = getattr(to_file, 'stream', to_file)
         self.show_ids = show_ids
         self.show_timezone = show_timezone
         if delta_format is None:
@@ -1463,9 +1472,11 @@
                                 short_status=False)
         if revision.diff is not None:
             to_file.write(indent + 'diff:\n')
+ to_file.flush()
             # Note: we explicitly don't indent the diff (relative to the
             # revision information) so that the output can be fed to patch -p0
             self.show_diff(self.to_exact_file, revision.diff, indent)
+ self.to_exact_file.flush()

def get_advice_separator(self):
"""Get the text separating the log from the closing advice."""

=== modified file 'bzrlib/ui/__init__.py'
--- bzrlib/ui/__init__.py 2009-11-16 02:56:00 +0000
+++ bzrlib/ui/__init__.py 2009-12-08 08:40:11 +0000
@@ -146,12 +146,7 @@
encoding = osutils.get_terminal_encoding()
if encoding_type i...

Quick re-review requested; here's the incremental diff

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2009-11-04 22:32:13 +0000
+++ bzrlib/builtins.py	2009-12-08 09:06:02 +0000
@@ -2323,7 +2323,10 @@
             # Build the log formatter
             if log_format is None:
                 log_format = log.log_formatter_registry.get_default(b)
+            # Make a non-encoding output to include the diffs - bug 328007
+            unencoded_output = ui.ui_factory.make_output_stream(encoding_type='exact')
             lf = log_format(show_ids=show_ids, to_file=self.outf,
+                            to_exact_file=unencoded_output,
                             show_timezone=timezone,
                             delta_format=get_verbosity_level(),
                             levels=levels,

=== modified file 'bzrlib/log.py'
--- bzrlib/log.py	2009-10-29 03:32:42 +0000
+++ bzrlib/log.py	2009-12-08 09:17:21 +0000
@@ -1291,10 +1291,13 @@
     preferred_levels = 0
 
     def __init__(self, to_file, show_ids=False, show_timezone='original',
-                 delta_format=None, levels=None, show_advice=False):
+                 delta_format=None, levels=None, show_advice=False,
+                 to_exact_file=None):
         """Create a LogFormatter.
 
         :param to_file: the file to output to
+        :param to_exact_file: if set, gives an output stream to which 
+             non-Unicode diffs are written.
         :param show_ids: if True, revision-ids are to be displayed
         :param show_timezone: the timezone to use
         :param delta_format: the level of delta information to display
@@ -1307,7 +1310,13 @@
         self.to_file = to_file
         # 'exact' stream used to show diff, it should print content 'as is'
         # and should not try to decode/encode it to unicode to avoid bug #328007
-        self.to_exact_file = getattr(to_file, 'stream', to_file)
+        if to_exact_file is not None:
+            self.to_exact_file = to_exact_file
+        else:
+            # XXX: somewhat hacky; this assumes it's a codec writer; it's better
+            # for code that expects to get diffs to pass in the exact file
+            # stream
+            self.to_exact_file = getattr(to_file, 'stream', to_file)
         self.show_ids = show_ids
         self.show_timezone = show_timezone
         if delta_format is None:
@@ -1463,9 +1472,11 @@
                                 short_status=False)
         if revision.diff is not None:
             to_file.write(indent + 'diff:\n')
+            to_file.flush()
             # Note: we explicitly don't indent the diff (relative to the
             # revision information) so that the output can be fed to patch -p0
             self.show_diff(self.to_exact_file, revision.diff, indent)
+            self.to_exact_file.flush()
 
     def get_advice_separator(self):
         """Get the text separating the log from the closing advice."""

=== modified file 'bzrlib/ui/__init__.py'
--- bzrlib/ui/__init__.py	2009-11-16 02:56:00 +0000
+++ bzrlib/ui/__init__.py	2009-12-08 08:40:11 +0000
@@ -146,12 +146,7 @@
             encoding = osutils.get_terminal_encoding()
         if encoding_type is None:
             encoding_type = 'replace'
-        # 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
         return out_stream
 
     def _make_output_stream_explicit(self, encoding, encoding_type):

=== modified file 'bzrlib/ui/text.py'
--- bzrlib/ui/text.py	2009-11-16 02:56:18 +0000
+++ bzrlib/ui/text.py	2009-12-08 09:17:34 +0000
@@ -161,6 +161,10 @@
         else:
             encoded_stdout = codecs.getwriter(encoding)(self.stdout,
                 errors=encoding_type)
+            # 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.
+            encoded_stdout.encoding = encoding
             return TextUIOutputStream(self, encoded_stdout)
 
     def note(self, msg):
@@ -400,6 +404,10 @@
     def __init__(self, ui_factory, wrapped_stream):
         self.ui_factory = ui_factory
         self.wrapped_stream = wrapped_stream
+        # this does no transcoding, but it must expose the underlying encoding
+        # because some callers need to know what can be written - see for
+        # example unescape_for_display.
+        self.encoding = wrapped_stream.encoding
 
     def flush(self):
         self.ui_factory.clear_term()

Revision history for this message

John A Meinel (jameinel) wrote on 2009-12-08:

#

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

Martin Pool wrote:
> Quick re-review requested; here's the incremental diff
>
> === modified file 'bzrlib/builtins.py'
> --- bzrlib/builtins.py 2009-11-04 22:32:13 +0000
> +++ bzrlib/builtins.py 2009-12-08 09:06:02 +0000
> @@ -2323,7 +2323,10 @@
> # Build the log formatter
> if log_format is None:
> log_format = log.log_formatter_registry.get_default(b)
> + # Make a non-encoding output to include the diffs - bug 328007
> + unencoded_output = ui.ui_factory.make_output_stream(encoding_type='exact')
> lf = log_format(show_ids=show_ids, to_file=self.outf,
> + to_exact_file=unencoded_output,
> show_timezone=timezone,
> delta_format=get_verbosity_level(),
> levels=levels,
>
> === modified file 'bzrlib/log.py'
> --- bzrlib/log.py 2009-10-29 03:32:42 +0000
> +++ bzrlib/log.py 2009-12-08 09:17:21 +0000
> @@ -1291,10 +1291,13 @@
> preferred_levels = 0
>
> def __init__(self, to_file, show_ids=False, show_timezone='original',
> - delta_format=None, levels=None, show_advice=False):
> + delta_format=None, levels=None, show_advice=False,
> + to_exact_file=None):
> """Create a LogFormatter.
>

Having to explicitly flush one before writing to the other is a bit
strange, and certainly the "write_bytes()" versus "write_unicode()"
makes that cleaner (IMO).

But this is a good incremental improvement over where we are at today.

review: approve
merge: approve

John
=:->

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

iEYEARECAAYFAkselr8ACgkQJdeBCYSNAAO8XwCePFXY9tLoAFigiZxWw4VBHipv
dlEAniXiLRPa3I6VUSCKvZVqAG68/1Fa
=VhDt
-----END PGP SIGNATURE-----

review: Approve

Bazaar

Merge lp:~mbp/bzr/progress-output into lp:bzr

Commit message

Description of the change

Preview Diff

Subscribers

 === modified file 'NEWS'
 --- NEWS	2009-12-07 22:32:56 +0000
 +++ NEWS	2009-12-09 05:49:10 +0000
@@ -241,6 +241,12 @@
  * Include Japanese translations for documentation (Inada Naoki)
++* New API ``ui_factory.make_output_stream`` to be used for sending bulk
++  (rather than user-interaction) data to stdout.  This automatically
++  coordinates with progress bars or other terminal activity, and can be
++  overridden by GUIs.
++  (Martin Pool, 493944)
++
  Internals
  *********
 === modified file 'bzrlib/builtins.py'
 --- bzrlib/builtins.py	2009-12-08 17:30:11 +0000
 +++ bzrlib/builtins.py	2009-12-09 05:49:10 +0000
@@ -2349,7 +2349,10 @@
              # Build the log formatter
              if log_format is None:
                  log_format = log.log_formatter_registry.get_default(b)
++            # Make a non-encoding output to include the diffs - bug 328007
++            unencoded_output = ui.ui_factory.make_output_stream(encoding_type='exact')
              lf = log_format(show_ids=show_ids, to_file=self.outf,
++                            to_exact_file=unencoded_output,
                              show_timezone=timezone,
                              delta_format=get_verbosity_level(),
                              levels=levels,
 === modified file 'bzrlib/cmd_version_info.py'
 --- bzrlib/cmd_version_info.py	2009-03-23 14:59:43 +0000
 +++ bzrlib/cmd_version_info.py	2009-12-09 05:49:10 +0000
@@ -1,4 +1,4 @@
--# Copyright (C) 2005, 2006 Canonical Ltd
++# Copyright (C) 2005, 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
@@ -17,14 +17,17 @@
  """Commands for generating snapshot information about a bzr tree."""
  from bzrlib.lazy_import import lazy_import
++
  lazy_import(globals(), """
  from bzrlib import (
      branch,
      errors,
++    ui,
++    version_info_formats,
      workingtree,
--    version_info_formats,
+     )
  """)
++
  from bzrlib.commands import Command
  from bzrlib.option import Option, RegistryOption
@@ -113,4 +116,4 @@
                  include_revision_history=include_history,
                  include_file_revisions=include_file_revisions,
                  template=template)
--        builder.generate(self.outf)
++        builder.generate(ui.ui_factory.make_output_stream())
 === modified file 'bzrlib/commands.py'
 --- bzrlib/commands.py	2009-12-02 07:56:16 +0000
 +++ bzrlib/commands.py	2009-12-09 05:49:10 +0000
@@ -1,4 +1,4 @@
--# Copyright (C) 2006, 2008 Canonical Ltd
++# Copyright (C) 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
@@ -45,6 +45,7 @@
      option,
      osutils,
      trace,
++    ui,
      win32utils,
+     )
  """)
@@ -595,26 +596,8 @@
      def _setup_outf(self):
          """Return a file linked to stdout, which has proper encoding."""
--        # Originally I was using self.stdout, but that looks
--        # *way* too much like sys.stdout
--        if self.encoding_type == 'exact':
--            # force sys.stdout to be binary stream on win32
--            if sys.platform == 'win32':
--                fileno = getattr(sys.stdout, 'fileno', None)
--                if fileno:
--                    import msvcrt
--                    msvcrt.setmode(fileno(), os.O_BINARY)
--            self.outf = sys.stdout
--            return
--
--        output_encoding = osutils.get_terminal_encoding()
--
--        self.outf = codecs.getwriter(output_encoding)(sys.stdout,
--                        errors=self.encoding_type)
--        # 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.
--        self.outf.encoding = output_encoding
++        self.outf = ui.ui_factory.make_output_stream(
++            encoding_type=self.encoding_type)
      def run_argv_aliases(self, argv, alias_argv=None):
          """Parse the command line and run with extra aliases in alias_argv."""
 === modified file 'bzrlib/log.py'
 --- bzrlib/log.py	2009-12-04 22:13:52 +0000
 +++ bzrlib/log.py	2009-12-09 05:49:10 +0000
@@ -1293,10 +1293,13 @@
      preferred_levels = 0
      def __init__(self, to_file, show_ids=False, show_timezone='original',
--            delta_format=None, levels=None, show_advice=False):
++            delta_format=None, levels=None, show_advice=False,
++            to_exact_file=None):
          """Create a LogFormatter.
          :param to_file: the file to output to
++        :param to_exact_file: if set, gives an output stream to which
++             non-Unicode diffs are written.
          :param show_ids: if True, revision-ids are to be displayed
          :param show_timezone: the timezone to use
          :param delta_format: the level of delta information to display
@@ -1309,7 +1312,13 @@
          self.to_file = to_file
          # 'exact' stream used to show diff, it should print content 'as is'
          # and should not try to decode/encode it to unicode to avoid bug #328007
--        self.to_exact_file = getattr(to_file, 'stream', to_file)
++        if to_exact_file is not None:
++            self.to_exact_file = to_exact_file
++        else:
++            # XXX: somewhat hacky; this assumes it's a codec writer; it's better
++            # for code that expects to get diffs to pass in the exact file
++            # stream
++            self.to_exact_file = getattr(to_file, 'stream', to_file)
          self.show_ids = show_ids
          self.show_timezone = show_timezone
          if delta_format is None:
@@ -1494,9 +1503,11 @@
                                  short_status=False)
          if revision.diff is not None:
              to_file.write(indent + 'diff:\n')
++            to_file.flush()
              # Note: we explicitly don't indent the diff (relative to the
              # revision information) so that the output can be fed to patch -p0
              self.show_diff(self.to_exact_file, revision.diff, indent)
++            self.to_exact_file.flush()
      def get_advice_separator(self):
          """Get the text separating the log from the closing advice."""
 === modified file 'bzrlib/tests/per_uifactory/__init__.py'
 --- bzrlib/tests/per_uifactory/__init__.py	2009-09-23 06:29:46 +0000
 +++ bzrlib/tests/per_uifactory/__init__.py	2009-12-09 05:49:10 +0000
@@ -74,6 +74,15 @@
          self.factory.show_warning(msg)
          self._check_show_warning(msg)
++    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!')
++
  class TestTextUIFactory(tests.TestCase, UIFactoryTestMixin):
 === modified file 'bzrlib/tests/test_ui.py'
 --- bzrlib/tests/test_ui.py	2009-10-15 20:04:37 +0000
 +++ bzrlib/tests/test_ui.py	2009-12-09 05:49:10 +0000
@@ -50,6 +50,7 @@
      NullProgressView,
      TextProgressView,
      TextUIFactory,
++    TextUIOutputStream,
+     )
@@ -253,6 +254,32 @@
              pb.finished()
++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()
++
++
  class UITests(tests.TestCase):
      def test_progress_construction(self):
 === modified file 'bzrlib/ui/__init__.py'
 --- bzrlib/ui/__init__.py	2009-12-04 10:24:28 +0000
 +++ bzrlib/ui/__init__.py	2009-12-09 05:49:10 +0000
@@ -125,6 +125,34 @@
          """
          raise NotImplementedError(self.get_password)
++    def make_output_stream(self, encoding=None, encoding_type=None):
++        """Get a stream for sending out bulk text data.
++
++        This is used for commands that produce bulk text, such as log or diff
++        output, as opposed to user interaction.  This should work even for
++        non-interactive user interfaces.  Typically this goes to a decorated
++        version of stdout, but in a GUI it might be appropriate to send it to a
++        window displaying the text.
++
++        :param encoding: Unicode encoding for output; default is the
++            terminal encoding, which may be different from the user encoding.
++            (See get_terminal_encoding.)
++
++        :param encoding_type: How to handle encoding errors:
++            replace/strict/escape/exact.  Default is replace.
++        """
++        # XXX: is the caller supposed to close the resulting object?
++        if encoding is None:
++            encoding = osutils.get_terminal_encoding()
++        if encoding_type is None:
++            encoding_type = 'replace'
++        out_stream = self._make_output_stream_explicit(encoding, encoding_type)
++        return out_stream
++
++    def _make_output_stream_explicit(self, encoding, encoding_type):
++        raise NotImplementedError("%s doesn't support make_output_stream"
++            % (self.__class__.__name__))
++
      def nested_progress_bar(self):
          """Return a nested progress bar.
 === modified file 'bzrlib/ui/text.py'
 --- bzrlib/ui/text.py	2009-12-07 10:38:09 +0000
 +++ bzrlib/ui/text.py	2009-12-09 05:49:10 +0000
@@ -18,6 +18,7 @@
  """Text UI, write output to the console.
  """
++import codecs
  import getpass
  import os
  import sys
@@ -146,6 +147,26 @@
          else:
              return NullProgressView()
++    def _make_output_stream_explicit(self, encoding, encoding_type):
++        if encoding_type == 'exact':
++            # force sys.stdout to be binary stream on win32;
++            # NB: this leaves the file set in that mode; may cause problems if
++            # one process tries to do binary and then text output
++            if sys.platform == 'win32':
++                fileno = getattr(self.stdout, 'fileno', None)
++                if fileno:
++                    import msvcrt
++                    msvcrt.setmode(fileno(), os.O_BINARY)
++            return TextUIOutputStream(self, self.stdout)
++        else:
++            encoded_stdout = codecs.getwriter(encoding)(self.stdout,
++                errors=encoding_type)
++            # 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.
++            encoded_stdout.encoding = encoding
++            return TextUIOutputStream(self, encoded_stdout)
++
      def note(self, msg):
          """Write an already-formatted message, clearing the progress bar if necessary."""
          self.clear_term()
@@ -367,3 +388,37 @@
              self._bytes_since_update = 0
              self._last_transport_msg = msg
              self._repaint()
++
++
++class TextUIOutputStream(object):
++    """Decorates an output stream so that the terminal is cleared before writing.
++
++    This is supposed to ensure that the progress bar does not conflict with bulk
++    text output.
++    """
++    # XXX: this does not handle the case of writing part of a line, then doing
++    # progress bar output: the progress bar will probably write over it.
++    # one option is just to buffer that text until we have a full line;
++    # another is to save and restore it
++
++    # XXX: might need to wrap more methods
++
++    def __init__(self, ui_factory, wrapped_stream):
++        self.ui_factory = ui_factory
++        self.wrapped_stream = wrapped_stream
++        # this does no transcoding, but it must expose the underlying encoding
++        # because some callers need to know what can be written - see for
++        # example unescape_for_display.
++        self.encoding = getattr(wrapped_stream, 'encoding', None)
++
++    def flush(self):
++        self.ui_factory.clear_term()
++        self.wrapped_stream.flush()
++
++    def write(self, to_write):
++        self.ui_factory.clear_term()
++        self.wrapped_stream.write(to_write)
++
++    def writelines(self, lines):
++        self.ui_factory.clear_term()
++        self.wrapped_stream.writelines(lines)