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

Proposed by Martin Pool on 2010-07-17
Status: Merged
Approved by: Vincent Ladeuil on 2010-07-19
Approved revision: no longer in the source branch.
Merged at revision: 5060
Proposed branch: lp:~mbp/bzr/progress
Merge into: lp:bzr/2.2
Diff against target: 308 lines (+84/-87)
5 files modified
NEWS (+6/-0)
bzrlib/progress.py (+0/-22)
bzrlib/tests/test_progress.py (+37/-4)
bzrlib/tests/test_ui.py (+4/-48)
bzrlib/ui/text.py (+37/-13)
To merge this branch: bzr merge lp:~mbp/bzr/progress
Reviewer Review Type Date Requested Status
Vincent Ladeuil 2010-07-17 Approve on 2010-07-19
Review via email: mp+30181@code.launchpad.net

Commit message

adjust spinner and repainting of progress bar

Description of the change

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.

To post a comment you must log in.
Vincent Ladeuil (vila) wrote :

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) wrote :

sent to pqm by email

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'NEWS'
--- NEWS 2010-07-16 12:53:03 +0000
+++ NEWS 2010-07-17 16:52:42 +0000
@@ -33,6 +33,10 @@
33* Don't traceback trying to unversion children files of an already33* Don't traceback trying to unversion children files of an already
34 unversioned directory. (Vincent Ladeuil, #494221)34 unversioned directory. (Vincent Ladeuil, #494221)
3535
36* Progress bars prefer to truncate the text message rather than the
37 counters. The spinner is shown between the network transfer indicator
38 and the progress message. (Martin Pool)
39
36* Recursive binding for checkouts is now detected by bzr. A clear error40* Recursive binding for checkouts is now detected by bzr. A clear error
37 message is shown to the user. (Parth Malwankar, #405192)41 message is shown to the user. (Parth Malwankar, #405192)
3842
@@ -49,6 +53,8 @@
49API Changes53API Changes
50***********54***********
5155
56* Delete ``ProgressTask.note``, which was deprecated in 2.1.
57
52Internals58Internals
53*********59*********
5460
5561
=== 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 @@
152 own_fraction = 0.0152 own_fraction = 0.0
153 return self._parent_task._overall_completion_fraction(own_fraction)153 return self._parent_task._overall_completion_fraction(own_fraction)
154154
155 @deprecated_method(deprecated_in((2, 1, 0)))
156 def note(self, fmt_string, *args):
157 """Record a note without disrupting the progress bar.
158
159 Deprecated: use ui_factory.note() instead or bzrlib.trace. Note that
160 ui_factory.note takes just one string as the argument, not a format
161 string and arguments.
162 """
163 if args:
164 self.ui_factory.note(fmt_string % args)
165 else:
166 self.ui_factory.note(fmt_string)
167
168 def clear(self):155 def clear(self):
169 # TODO: deprecate this method; the model object shouldn't be concerned156 # TODO: deprecate this method; the model object shouldn't be concerned
170 # with whether it's shown or not. Most callers use this because they157 # with whether it's shown or not. Most callers use this because they
@@ -220,12 +207,6 @@
220 self.clear()207 self.clear()
221 self._stack.return_pb(self)208 self._stack.return_pb(self)
222209
223 def note(self, fmt_string, *args, **kwargs):
224 """Record a note without disrupting the progress bar."""
225 self.clear()
226 self.to_messages_file.write(fmt_string % args)
227 self.to_messages_file.write('\n')
228
229210
230class DummyProgress(object):211class DummyProgress(object):
231 """Progress-bar standin that does nothing.212 """Progress-bar standin that does nothing.
@@ -248,9 +229,6 @@
248 def clear(self):229 def clear(self):
249 pass230 pass
250231
251 def note(self, fmt_string, *args, **kwargs):
252 """See _BaseProgressBar.note()."""
253
254 def child_progress(self, **kwargs):232 def child_progress(self, **kwargs):
255 return DummyProgress(**kwargs)233 return DummyProgress(**kwargs)
256234
257235
=== 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 @@
58 def make_view(self):58 def make_view(self):
59 out = StringIO()59 out = StringIO()
60 view = TextProgressView(out)60 view = TextProgressView(out)
61 view._width = 8061 view._avail_width = lambda: 79
62 return out, view62 return out, view
63 63
64 def make_task(self, parent_task, view, msg, curr, total):64 def make_task(self, parent_task, view, msg, curr, total):
@@ -100,13 +100,13 @@
100 # so we're in the first half of the main task, and half way through100 # so we're in the first half of the main task, and half way through
101 # that101 # that
102 self.assertEqual(102 self.assertEqual(
103r'[####- ] reticulating splines:stage2 1/2'103'[####- ] reticulating splines:stage2 1/2 '
104 , view._render_line())104 , view._render_line())
105 # if the nested task is complete, then we're all the way through the105 # if the nested task is complete, then we're all the way through the
106 # first half of the overall work106 # first half of the overall work
107 task2.update('stage2', 2, 2)107 task2.update('stage2', 2, 2)
108 self.assertEqual(108 self.assertEqual(
109r'[#########\ ] reticulating splines:stage2 2/2'109'[#########\ ] reticulating splines:stage2 2/2 '
110 , view._render_line())110 , view._render_line())
111111
112 def test_render_progress_sub_nested(self):112 def test_render_progress_sub_nested(self):
@@ -123,5 +123,38 @@
123 # progress indication, just a label; and the bottom one is half done,123 # progress indication, just a label; and the bottom one is half done,
124 # so the overall fraction is 1/4124 # so the overall fraction is 1/4
125 self.assertEqual(125 self.assertEqual(
126 r'[####| ] a:b:c 1/2'126'[####| ] a:b:c 1/2 '
127 , view._render_line())127 , view._render_line())
128
129 def test_render_truncated(self):
130 # when the bar is too long for the terminal, we prefer not to truncate
131 # the counters because they might be interesting, and because
132 # truncating the numbers might be misleading
133 out, view = self.make_view()
134 task_a = ProgressTask(None, progress_view=view)
135 task_a.update('start_' + 'a' * 200 + '_end', 2000, 5000)
136 line = view._render_line()
137 self.assertEqual(
138'- start_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.. 2000/5000',
139 line)
140 self.assertEqual(len(line), 79)
141
142
143 def test_render_with_activity(self):
144 # if the progress view has activity, it's shown before the spinner
145 out, view = self.make_view()
146 task_a = ProgressTask(None, progress_view=view)
147 view._last_transport_msg = ' 123kB 100kB/s '
148 line = view._render_line()
149 self.assertEqual(
150' 123kB 100kB/s / ',
151 line)
152 self.assertEqual(len(line), 79)
153
154 task_a.update('start_' + 'a' * 200 + '_end', 2000, 5000)
155 view._last_transport_msg = ' 123kB 100kB/s '
156 line = view._render_line()
157 self.assertEqual(
158' 123kB 100kB/s \\ start_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.. 2000/5000',
159 line)
160 self.assertEqual(len(line), 79)
128161
=== 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 @@
100 finally:100 finally:
101 pb.finished()101 pb.finished()
102102
103 def test_progress_note(self):
104 stderr = tests.StringIOWrapper()
105 stdout = tests.StringIOWrapper()
106 ui_factory = _mod_ui_text.TextUIFactory(stdin=tests.StringIOWrapper(''),
107 stderr=stderr,
108 stdout=stdout)
109 pb = ui_factory.nested_progress_bar()
110 try:
111 result = self.applyDeprecated(deprecated_in((2, 1, 0)),
112 pb.note,
113 't')
114 self.assertEqual(None, result)
115 self.assertEqual("t\n", stdout.getvalue())
116 # Since there was no update() call, there should be no clear() call
117 self.failIf(re.search(r'^\r {10,}\r$',
118 stderr.getvalue()) is not None,
119 'We cleared the stderr without anything to put there')
120 finally:
121 pb.finished()
122
123 def test_progress_note_clears(self):
124 stderr = test_progress._TTYStringIO()
125 stdout = test_progress._TTYStringIO()
126 # so that we get a TextProgressBar
127 os.environ['TERM'] = 'xterm'
128 ui_factory = _mod_ui_text.TextUIFactory(
129 stdin=tests.StringIOWrapper(''),
130 stdout=stdout, stderr=stderr)
131 self.assertIsInstance(ui_factory._progress_view,
132 _mod_ui_text.TextProgressView)
133 pb = ui_factory.nested_progress_bar()
134 try:
135 # Create a progress update that isn't throttled
136 pb.update('x', 1, 1)
137 result = self.applyDeprecated(deprecated_in((2, 1, 0)),
138 pb.note, 't')
139 self.assertEqual(None, result)
140 self.assertEqual("t\n", stdout.getvalue())
141 # the exact contents will depend on the terminal width and we don't
142 # care about that right now - but you're probably running it on at
143 # least a 10-character wide terminal :)
144 self.assertContainsRe(stderr.getvalue(), r'\r {10,}\r$')
145 finally:
146 pb.finished()
147
148 def test_text_ui_get_boolean(self):103 def test_text_ui_get_boolean(self):
149 stdin = tests.StringIOWrapper("y\n" # True104 stdin = tests.StringIOWrapper("y\n" # True
150 "n\n" # False105 "n\n" # False
@@ -193,6 +148,7 @@
193 factory = _mod_ui_text.TextUIFactory(148 factory = _mod_ui_text.TextUIFactory(
194 stdin=tests.StringIOWrapper("yada\ny\n"),149 stdin=tests.StringIOWrapper("yada\ny\n"),
195 stdout=out, stderr=out)150 stdout=out, stderr=out)
151 factory._avail_width = lambda: 79
196 pb = factory.nested_progress_bar()152 pb = factory.nested_progress_bar()
197 pb.show_bar = False153 pb.show_bar = False
198 pb.show_spinner = False154 pb.show_spinner = False
@@ -204,9 +160,9 @@
204 factory.get_boolean,160 factory.get_boolean,
205 "what do you want"))161 "what do you want"))
206 output = out.getvalue()162 output = out.getvalue()
207 self.assertContainsRe(factory.stdout.getvalue(),163 self.assertContainsRe(output,
208 "foo *\r\r *\r*")164 "| foo *\r\r *\r*")
209 self.assertContainsRe(factory.stdout.getvalue(),165 self.assertContainsRe(output,
210 r"what do you want\? \[y/n\]: what do you want\? \[y/n\]: ")166 r"what do you want\? \[y/n\]: what do you want\? \[y/n\]: ")
211 # stdin should have been totally consumed167 # stdin should have been totally consumed
212 self.assertEqual('', factory.stdin.readline())168 self.assertEqual('', factory.stdin.readline())
213169
=== 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 @@
300 # correspond reliably to overall command progress300 # correspond reliably to overall command progress
301 self.enable_bar = False301 self.enable_bar = False
302302
303 def _avail_width(self):
304 # we need one extra space for terminals that wrap on last char
305 w = osutils.terminal_width()
306 if w is None:
307 return w
308 else:
309 return w - 1
310
303 def _show_line(self, s):311 def _show_line(self, s):
304 # sys.stderr.write("progress %r\n" % s)
305 width = osutils.terminal_width()
306 if width is not None:
307 # we need one extra space for terminals that wrap on last char
308 width = width - 1
309 s = '%-*.*s' % (width, width, s)
310 self._term_file.write('\r' + s + '\r')312 self._term_file.write('\r' + s + '\r')
311313
312 def clear(self):314 def clear(self):
@@ -348,6 +350,10 @@
348 return ''350 return ''
349351
350 def _format_task(self, task):352 def _format_task(self, task):
353 """Format task-specific parts of progress bar.
354
355 :returns: (text_part, counter_part) both unicode strings.
356 """
351 if not task.show_count:357 if not task.show_count:
352 s = ''358 s = ''
353 elif task.current_cnt is not None and task.total_cnt is not None:359 elif task.current_cnt is not None and task.total_cnt is not None:
@@ -363,21 +369,39 @@
363 t = t._parent_task369 t = t._parent_task
364 if t.msg:370 if t.msg:
365 m = t.msg + ':' + m371 m = t.msg + ':' + m
366 return m + s372 return m, s
367373
368 def _render_line(self):374 def _render_line(self):
369 bar_string = self._render_bar()375 bar_string = self._render_bar()
370 if self._last_task:376 if self._last_task:
371 task_msg = self._format_task(self._last_task)377 task_part, counter_part = self._format_task(self._last_task)
372 else:378 else:
373 task_msg = ''379 task_part = counter_part = ''
374 if self._last_task and not self._last_task.show_transport_activity:380 if self._last_task and not self._last_task.show_transport_activity:
375 trans = ''381 trans = ''
376 else:382 else:
377 trans = self._last_transport_msg383 trans = self._last_transport_msg
378 if trans:384 # the bar separates the transport activity from the message, so even
379 trans += ' | '385 # if there's no bar or spinner, we must show something if both those
380 return (bar_string + trans + task_msg)386 # fields are present
387 if (task_part or trans) and not bar_string:
388 bar_string = '| '
389 # preferentially truncate the task message if we don't have enough
390 # space
391 avail_width = self._avail_width()
392 if avail_width is not None:
393 # if terminal avail_width is unknown, don't truncate
394 current_len = len(bar_string) + len(trans) + len(task_part) + len(counter_part)
395 gap = current_len - avail_width
396 if gap > 0:
397 task_part = task_part[:-gap-2] + '..'
398 s = trans + bar_string + task_part + counter_part
399 if avail_width is not None:
400 if len(s) < avail_width:
401 s = s.ljust(avail_width)
402 elif len(s) > avail_width:
403 s = s[:avail_width]
404 return s
381405
382 def _repaint(self):406 def _repaint(self):
383 s = self._render_line()407 s = self._render_line()
@@ -439,7 +463,7 @@
439 rate = (self._bytes_since_update463 rate = (self._bytes_since_update
440 / (now - self._transport_update_time))464 / (now - self._transport_update_time))
441 # using base-10 units (see HACKING.txt).465 # using base-10 units (see HACKING.txt).
442 msg = ("%6dkB %5dkB/s" %466 msg = ("%6dkB %5dkB/s " %
443 (self._total_byte_count / 1000, int(rate) / 1000,))467 (self._total_byte_count / 1000, int(rate) / 1000,))
444 self._transport_update_time = now468 self._transport_update_time = now
445 self._last_repaint = now469 self._last_repaint = now

Subscribers

People subscribed via source and target branches