Merge lp:~mbp/bzr/progress into lp:bzr/2.2

[Martin Pool]

adjust spinner and repainting of progress bar

Based on <https://code.edge.launchpad.net/~mbp/bzr/progress/+merge/29605>

 * spinner is now between the transport activity and the status message, so that it doesn't look like there's two spinners and one of them is stuck
 * avoid truncating the numbers at the end of the progress bar
 * delete the (deprecated since 2.1) ProgressTask.note

I think this is still safe for 2.2 since the only api change is to something already long deprecated.

[Vincent Ladeuil (vila)]

This seems to address more cases where a terminal doesn't allow using the full width which is good.
I'm still unclear about *where* exactly this is relevant but I think it's safer this way.
We may have complaints that we don't use the full available width but less than complaints about bad wrapping anyway.

review: Approve

[Martin Pool (mbp)]

sent to pqm by email

=== modified file 'NEWS'
--- NEWS	2010-07-16 12:53:03 +0000
+++ NEWS	2010-07-17 16:52:42 +0000
@@ -33,6 +33,10 @@
 * Don't traceback trying to unversion children files of an already
   unversioned directory.  (Vincent Ladeuil, #494221)
 
+* Progress bars prefer to truncate the text message rather than the
+  counters.  The spinner is shown between the network transfer indicator
+  and the progress message.  (Martin Pool)
+
 * Recursive binding for checkouts is now detected by bzr. A clear error
   message is shown to the user. (Parth Malwankar, #405192)
 
@@ -49,6 +53,8 @@
 API Changes
 ***********
 
+* Delete ``ProgressTask.note``, which was deprecated in 2.1.
+
 Internals
 *********
 
 
=== modified file 'bzrlib/progress.py'
--- bzrlib/progress.py	2010-04-30 11:03:59 +0000
+++ bzrlib/progress.py	2010-07-17 16:52:42 +0000
@@ -152,19 +152,6 @@
                 own_fraction = 0.0
             return self._parent_task._overall_completion_fraction(own_fraction)
 
-    @deprecated_method(deprecated_in((2, 1, 0)))
-    def note(self, fmt_string, *args):
-        """Record a note without disrupting the progress bar.
-        
-        Deprecated: use ui_factory.note() instead or bzrlib.trace.  Note that
-        ui_factory.note takes just one string as the argument, not a format
-        string and arguments.
-        """
-        if args:
-            self.ui_factory.note(fmt_string % args)
-        else:
-            self.ui_factory.note(fmt_string)
-
     def clear(self):
         # TODO: deprecate this method; the model object shouldn't be concerned
         # with whether it's shown or not.  Most callers use this because they
@@ -220,12 +207,6 @@
         self.clear()
         self._stack.return_pb(self)
 
-    def note(self, fmt_string, *args, **kwargs):
-        """Record a note without disrupting the progress bar."""
-        self.clear()
-        self.to_messages_file.write(fmt_string % args)
-        self.to_messages_file.write('\n')
-
 
 class DummyProgress(object):
     """Progress-bar standin that does nothing.
@@ -248,9 +229,6 @@
     def clear(self):
         pass
 
-    def note(self, fmt_string, *args, **kwargs):
-        """See _BaseProgressBar.note()."""
-
     def child_progress(self, **kwargs):
         return DummyProgress(**kwargs)
 
 
=== modified file 'bzrlib/tests/test_progress.py'
--- bzrlib/tests/test_progress.py	2010-02-17 17:11:16 +0000
+++ bzrlib/tests/test_progress.py	2010-07-17 16:52:42 +0000
@@ -58,7 +58,7 @@
     def make_view(self):
         out = StringIO()
         view = TextProgressView(out)
-        view._width = 80
+        view._avail_width = lambda: 79
         return out, view
     
     def make_task(self, parent_task, view, msg, curr, total):
@@ -100,13 +100,13 @@
         # so we're in the first half of the main task, and half way through
         # that
         self.assertEqual(
-r'[####-               ] reticulating splines:stage2 1/2'
+'[####-               ] 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'
+'[#########\          ] reticulating splines:stage2 2/2                         '
             , view._render_line())
 
     def test_render_progress_sub_nested(self):
@@ -123,5 +123,38 @@
         # 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'
+'[####|               ] a:b:c 1/2                                               '
             , view._render_line())
+
+    def test_render_truncated(self):
+        # when the bar is too long for the terminal, we prefer not to truncate
+        # the counters because they might be interesting, and because
+        # truncating the numbers might be misleading
+        out, view = self.make_view()
+        task_a = ProgressTask(None, progress_view=view)
+        task_a.update('start_' + 'a' * 200 + '_end', 2000, 5000)
+        line = view._render_line()
+        self.assertEqual(
+'- start_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.. 2000/5000',
+           line) 
+        self.assertEqual(len(line), 79)
+
+
+    def test_render_with_activity(self):
+        # if the progress view has activity, it's shown before the spinner
+        out, view = self.make_view()
+        task_a = ProgressTask(None, progress_view=view)
+        view._last_transport_msg = '   123kB   100kB/s '
+        line = view._render_line()
+        self.assertEqual(
+'   123kB   100kB/s /                                                           ',
+           line) 
+        self.assertEqual(len(line), 79)
+
+        task_a.update('start_' + 'a' * 200 + '_end', 2000, 5000)
+        view._last_transport_msg = '   123kB   100kB/s '
+        line = view._render_line()
+        self.assertEqual(
+'   123kB   100kB/s \\ start_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.. 2000/5000',
+           line) 
+        self.assertEqual(len(line), 79)
 
=== modified file 'bzrlib/tests/test_ui.py'
--- bzrlib/tests/test_ui.py	2010-06-21 22:29:38 +0000
+++ bzrlib/tests/test_ui.py	2010-07-17 16:52:42 +0000
@@ -100,51 +100,6 @@
         finally:
             pb.finished()
 
-    def test_progress_note(self):
-        stderr = tests.StringIOWrapper()
-        stdout = tests.StringIOWrapper()
-        ui_factory = _mod_ui_text.TextUIFactory(stdin=tests.StringIOWrapper(''),
-                                                stderr=stderr,
-                                                stdout=stdout)
-        pb = ui_factory.nested_progress_bar()
-        try:
-            result = self.applyDeprecated(deprecated_in((2, 1, 0)),
-                pb.note,
-                't')
-            self.assertEqual(None, result)
-            self.assertEqual("t\n", stdout.getvalue())
-            # Since there was no update() call, there should be no clear() call
-            self.failIf(re.search(r'^\r {10,}\r$',
-                                  stderr.getvalue()) is not None,
-                        'We cleared the stderr without anything to put there')
-        finally:
-            pb.finished()
-
-    def test_progress_note_clears(self):
-        stderr = test_progress._TTYStringIO()
-        stdout = test_progress._TTYStringIO()
-        # so that we get a TextProgressBar
-        os.environ['TERM'] = 'xterm'
-        ui_factory = _mod_ui_text.TextUIFactory(
-            stdin=tests.StringIOWrapper(''),
-            stdout=stdout, stderr=stderr)
-        self.assertIsInstance(ui_factory._progress_view,
-                              _mod_ui_text.TextProgressView)
-        pb = ui_factory.nested_progress_bar()
-        try:
-            # Create a progress update that isn't throttled
-            pb.update('x', 1, 1)
-            result = self.applyDeprecated(deprecated_in((2, 1, 0)),
-                pb.note, 't')
-            self.assertEqual(None, result)
-            self.assertEqual("t\n", stdout.getvalue())
-            # the exact contents will depend on the terminal width and we don't
-            # care about that right now - but you're probably running it on at
-            # least a 10-character wide terminal :)
-            self.assertContainsRe(stderr.getvalue(), r'\r {10,}\r$')
-        finally:
-            pb.finished()
-
     def test_text_ui_get_boolean(self):
         stdin = tests.StringIOWrapper("y\n" # True
                                       "n\n" # False
@@ -193,6 +148,7 @@
         factory = _mod_ui_text.TextUIFactory(
             stdin=tests.StringIOWrapper("yada\ny\n"),
             stdout=out, stderr=out)
+        factory._avail_width = lambda: 79
         pb = factory.nested_progress_bar()
         pb.show_bar = False
         pb.show_spinner = False
@@ -204,9 +160,9 @@
                                                factory.get_boolean,
                                                "what do you want"))
         output = out.getvalue()
-        self.assertContainsRe(factory.stdout.getvalue(),
-            "foo *\r\r  *\r*")
-        self.assertContainsRe(factory.stdout.getvalue(),
+        self.assertContainsRe(output,
+            "| foo *\r\r  *\r*")
+        self.assertContainsRe(output,
             r"what do you want\? \[y/n\]: what do you want\? \[y/n\]: ")
         # stdin should have been totally consumed
         self.assertEqual('', factory.stdin.readline())
 
=== modified file 'bzrlib/ui/text.py'
--- bzrlib/ui/text.py	2010-06-21 01:30:45 +0000
+++ bzrlib/ui/text.py	2010-07-17 16:52:42 +0000
@@ -300,13 +300,15 @@
         # correspond reliably to overall command progress
         self.enable_bar = False
 
+    def _avail_width(self):
+        # we need one extra space for terminals that wrap on last char
+        w = osutils.terminal_width() 
+        if w is None:
+            return w
+        else:
+            return w - 1
+
     def _show_line(self, s):
-        # sys.stderr.write("progress %r\n" % s)
-        width = osutils.terminal_width()
-        if width is not None:
-            # we need one extra space for terminals that wrap on last char
-            width = width - 1
-            s = '%-*.*s' % (width, width, s)
         self._term_file.write('\r' + s + '\r')
 
     def clear(self):
@@ -348,6 +350,10 @@
             return ''
 
     def _format_task(self, task):
+        """Format task-specific parts of progress bar.
+
+        :returns: (text_part, counter_part) both unicode strings.
+        """
         if not task.show_count:
             s = ''
         elif task.current_cnt is not None and task.total_cnt is not None:
@@ -363,21 +369,39 @@
             t = t._parent_task
             if t.msg:
                 m = t.msg + ':' + m
-        return m + s
+        return m, s
 
     def _render_line(self):
         bar_string = self._render_bar()
         if self._last_task:
-            task_msg = self._format_task(self._last_task)
+            task_part, counter_part = self._format_task(self._last_task)
         else:
-            task_msg = ''
+            task_part = counter_part = ''
         if self._last_task and not self._last_task.show_transport_activity:
             trans = ''
         else:
             trans = self._last_transport_msg
-            if trans:
-                trans += ' | '
-        return (bar_string + trans + task_msg)
+        # the bar separates the transport activity from the message, so even
+        # if there's no bar or spinner, we must show something if both those
+        # fields are present
+        if (task_part or trans) and not bar_string:
+            bar_string = '| '
+        # preferentially truncate the task message if we don't have enough
+        # space
+        avail_width = self._avail_width()
+        if avail_width is not None:
+            # if terminal avail_width is unknown, don't truncate
+            current_len = len(bar_string) + len(trans) + len(task_part) + len(counter_part)
+            gap = current_len - avail_width
+            if gap > 0:
+                task_part = task_part[:-gap-2] + '..'
+        s = trans + bar_string + task_part + counter_part
+        if avail_width is not None:
+            if len(s) < avail_width:
+                s = s.ljust(avail_width)
+            elif len(s) > avail_width:
+                s = s[:avail_width]
+        return s
 
     def _repaint(self):
         s = self._render_line()
@@ -439,7 +463,7 @@
             rate = (self._bytes_since_update
                     / (now - self._transport_update_time))
             # using base-10 units (see HACKING.txt).
-            msg = ("%6dkB %5dkB/s" %
+            msg = ("%6dkB %5dkB/s " %
                     (self._total_byte_count / 1000, int(rate) / 1000,))
             self._transport_update_time = now
             self._last_repaint = now