Bazaar

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

  1. progress
  2. Merge into 2.2
Proposed by Martin Pool
Status: Merged
Approved by: Vincent Ladeuil
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 Approve
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.
Revision history for this message
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
Revision history for this message
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
1=== modified file 'NEWS'
2--- NEWS 2010-07-16 12:53:03 +0000
3+++ NEWS 2010-07-17 16:52:42 +0000
4@@ -33,6 +33,10 @@
5 * Don't traceback trying to unversion children files of an already
6 unversioned directory. (Vincent Ladeuil, #494221)
7
8+* Progress bars prefer to truncate the text message rather than the
9+ counters. The spinner is shown between the network transfer indicator
10+ and the progress message. (Martin Pool)
11+
12 * Recursive binding for checkouts is now detected by bzr. A clear error
13 message is shown to the user. (Parth Malwankar, #405192)
14
15@@ -49,6 +53,8 @@
16 API Changes
17 ***********
18
19+* Delete ``ProgressTask.note``, which was deprecated in 2.1.
20+
21 Internals
22 *********
23
24
25=== modified file 'bzrlib/progress.py'
26--- bzrlib/progress.py 2010-04-30 11:03:59 +0000
27+++ bzrlib/progress.py 2010-07-17 16:52:42 +0000
28@@ -152,19 +152,6 @@
29 own_fraction = 0.0
30 return self._parent_task._overall_completion_fraction(own_fraction)
31
32- @deprecated_method(deprecated_in((2, 1, 0)))
33- def note(self, fmt_string, *args):
34- """Record a note without disrupting the progress bar.
35-
36- Deprecated: use ui_factory.note() instead or bzrlib.trace. Note that
37- ui_factory.note takes just one string as the argument, not a format
38- string and arguments.
39- """
40- if args:
41- self.ui_factory.note(fmt_string % args)
42- else:
43- self.ui_factory.note(fmt_string)
44-
45 def clear(self):
46 # TODO: deprecate this method; the model object shouldn't be concerned
47 # with whether it's shown or not. Most callers use this because they
48@@ -220,12 +207,6 @@
49 self.clear()
50 self._stack.return_pb(self)
51
52- def note(self, fmt_string, *args, **kwargs):
53- """Record a note without disrupting the progress bar."""
54- self.clear()
55- self.to_messages_file.write(fmt_string % args)
56- self.to_messages_file.write('\n')
57-
58
59 class DummyProgress(object):
60 """Progress-bar standin that does nothing.
61@@ -248,9 +229,6 @@
62 def clear(self):
63 pass
64
65- def note(self, fmt_string, *args, **kwargs):
66- """See _BaseProgressBar.note()."""
67-
68 def child_progress(self, **kwargs):
69 return DummyProgress(**kwargs)
70
71
72=== modified file 'bzrlib/tests/test_progress.py'
73--- bzrlib/tests/test_progress.py 2010-02-17 17:11:16 +0000
74+++ bzrlib/tests/test_progress.py 2010-07-17 16:52:42 +0000
75@@ -58,7 +58,7 @@
76 def make_view(self):
77 out = StringIO()
78 view = TextProgressView(out)
79- view._width = 80
80+ view._avail_width = lambda: 79
81 return out, view
82
83 def make_task(self, parent_task, view, msg, curr, total):
84@@ -100,13 +100,13 @@
85 # so we're in the first half of the main task, and half way through
86 # that
87 self.assertEqual(
88-r'[####- ] reticulating splines:stage2 1/2'
89+'[####- ] reticulating splines:stage2 1/2 '
90 , view._render_line())
91 # if the nested task is complete, then we're all the way through the
92 # first half of the overall work
93 task2.update('stage2', 2, 2)
94 self.assertEqual(
95-r'[#########\ ] reticulating splines:stage2 2/2'
96+'[#########\ ] reticulating splines:stage2 2/2 '
97 , view._render_line())
98
99 def test_render_progress_sub_nested(self):
100@@ -123,5 +123,38 @@
101 # progress indication, just a label; and the bottom one is half done,
102 # so the overall fraction is 1/4
103 self.assertEqual(
104- r'[####| ] a:b:c 1/2'
105+'[####| ] a:b:c 1/2 '
106 , view._render_line())
107+
108+ def test_render_truncated(self):
109+ # when the bar is too long for the terminal, we prefer not to truncate
110+ # the counters because they might be interesting, and because
111+ # truncating the numbers might be misleading
112+ out, view = self.make_view()
113+ task_a = ProgressTask(None, progress_view=view)
114+ task_a.update('start_' + 'a' * 200 + '_end', 2000, 5000)
115+ line = view._render_line()
116+ self.assertEqual(
117+'- start_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.. 2000/5000',
118+ line)
119+ self.assertEqual(len(line), 79)
120+
121+
122+ def test_render_with_activity(self):
123+ # if the progress view has activity, it's shown before the spinner
124+ out, view = self.make_view()
125+ task_a = ProgressTask(None, progress_view=view)
126+ view._last_transport_msg = ' 123kB 100kB/s '
127+ line = view._render_line()
128+ self.assertEqual(
129+' 123kB 100kB/s / ',
130+ line)
131+ self.assertEqual(len(line), 79)
132+
133+ task_a.update('start_' + 'a' * 200 + '_end', 2000, 5000)
134+ view._last_transport_msg = ' 123kB 100kB/s '
135+ line = view._render_line()
136+ self.assertEqual(
137+' 123kB 100kB/s \\ start_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.. 2000/5000',
138+ line)
139+ self.assertEqual(len(line), 79)
140
141=== modified file 'bzrlib/tests/test_ui.py'
142--- bzrlib/tests/test_ui.py 2010-06-21 22:29:38 +0000
143+++ bzrlib/tests/test_ui.py 2010-07-17 16:52:42 +0000
144@@ -100,51 +100,6 @@
145 finally:
146 pb.finished()
147
148- def test_progress_note(self):
149- stderr = tests.StringIOWrapper()
150- stdout = tests.StringIOWrapper()
151- ui_factory = _mod_ui_text.TextUIFactory(stdin=tests.StringIOWrapper(''),
152- stderr=stderr,
153- stdout=stdout)
154- pb = ui_factory.nested_progress_bar()
155- try:
156- result = self.applyDeprecated(deprecated_in((2, 1, 0)),
157- pb.note,
158- 't')
159- self.assertEqual(None, result)
160- self.assertEqual("t\n", stdout.getvalue())
161- # Since there was no update() call, there should be no clear() call
162- self.failIf(re.search(r'^\r {10,}\r$',
163- stderr.getvalue()) is not None,
164- 'We cleared the stderr without anything to put there')
165- finally:
166- pb.finished()
167-
168- def test_progress_note_clears(self):
169- stderr = test_progress._TTYStringIO()
170- stdout = test_progress._TTYStringIO()
171- # so that we get a TextProgressBar
172- os.environ['TERM'] = 'xterm'
173- ui_factory = _mod_ui_text.TextUIFactory(
174- stdin=tests.StringIOWrapper(''),
175- stdout=stdout, stderr=stderr)
176- self.assertIsInstance(ui_factory._progress_view,
177- _mod_ui_text.TextProgressView)
178- pb = ui_factory.nested_progress_bar()
179- try:
180- # Create a progress update that isn't throttled
181- pb.update('x', 1, 1)
182- result = self.applyDeprecated(deprecated_in((2, 1, 0)),
183- pb.note, 't')
184- self.assertEqual(None, result)
185- self.assertEqual("t\n", stdout.getvalue())
186- # the exact contents will depend on the terminal width and we don't
187- # care about that right now - but you're probably running it on at
188- # least a 10-character wide terminal :)
189- self.assertContainsRe(stderr.getvalue(), r'\r {10,}\r$')
190- finally:
191- pb.finished()
192-
193 def test_text_ui_get_boolean(self):
194 stdin = tests.StringIOWrapper("y\n" # True
195 "n\n" # False
196@@ -193,6 +148,7 @@
197 factory = _mod_ui_text.TextUIFactory(
198 stdin=tests.StringIOWrapper("yada\ny\n"),
199 stdout=out, stderr=out)
200+ factory._avail_width = lambda: 79
201 pb = factory.nested_progress_bar()
202 pb.show_bar = False
203 pb.show_spinner = False
204@@ -204,9 +160,9 @@
205 factory.get_boolean,
206 "what do you want"))
207 output = out.getvalue()
208- self.assertContainsRe(factory.stdout.getvalue(),
209- "foo *\r\r *\r*")
210- self.assertContainsRe(factory.stdout.getvalue(),
211+ self.assertContainsRe(output,
212+ "| foo *\r\r *\r*")
213+ self.assertContainsRe(output,
214 r"what do you want\? \[y/n\]: what do you want\? \[y/n\]: ")
215 # stdin should have been totally consumed
216 self.assertEqual('', factory.stdin.readline())
217
218=== modified file 'bzrlib/ui/text.py'
219--- bzrlib/ui/text.py 2010-06-21 01:30:45 +0000
220+++ bzrlib/ui/text.py 2010-07-17 16:52:42 +0000
221@@ -300,13 +300,15 @@
222 # correspond reliably to overall command progress
223 self.enable_bar = False
224
225+ def _avail_width(self):
226+ # we need one extra space for terminals that wrap on last char
227+ w = osutils.terminal_width()
228+ if w is None:
229+ return w
230+ else:
231+ return w - 1
232+
233 def _show_line(self, s):
234- # sys.stderr.write("progress %r\n" % s)
235- width = osutils.terminal_width()
236- if width is not None:
237- # we need one extra space for terminals that wrap on last char
238- width = width - 1
239- s = '%-*.*s' % (width, width, s)
240 self._term_file.write('\r' + s + '\r')
241
242 def clear(self):
243@@ -348,6 +350,10 @@
244 return ''
245
246 def _format_task(self, task):
247+ """Format task-specific parts of progress bar.
248+
249+ :returns: (text_part, counter_part) both unicode strings.
250+ """
251 if not task.show_count:
252 s = ''
253 elif task.current_cnt is not None and task.total_cnt is not None:
254@@ -363,21 +369,39 @@
255 t = t._parent_task
256 if t.msg:
257 m = t.msg + ':' + m
258- return m + s
259+ return m, s
260
261 def _render_line(self):
262 bar_string = self._render_bar()
263 if self._last_task:
264- task_msg = self._format_task(self._last_task)
265+ task_part, counter_part = self._format_task(self._last_task)
266 else:
267- task_msg = ''
268+ task_part = counter_part = ''
269 if self._last_task and not self._last_task.show_transport_activity:
270 trans = ''
271 else:
272 trans = self._last_transport_msg
273- if trans:
274- trans += ' | '
275- return (bar_string + trans + task_msg)
276+ # the bar separates the transport activity from the message, so even
277+ # if there's no bar or spinner, we must show something if both those
278+ # fields are present
279+ if (task_part or trans) and not bar_string:
280+ bar_string = '| '
281+ # preferentially truncate the task message if we don't have enough
282+ # space
283+ avail_width = self._avail_width()
284+ if avail_width is not None:
285+ # if terminal avail_width is unknown, don't truncate
286+ current_len = len(bar_string) + len(trans) + len(task_part) + len(counter_part)
287+ gap = current_len - avail_width
288+ if gap > 0:
289+ task_part = task_part[:-gap-2] + '..'
290+ s = trans + bar_string + task_part + counter_part
291+ if avail_width is not None:
292+ if len(s) < avail_width:
293+ s = s.ljust(avail_width)
294+ elif len(s) > avail_width:
295+ s = s[:avail_width]
296+ return s
297
298 def _repaint(self):
299 s = self._render_line()
300@@ -439,7 +463,7 @@
301 rate = (self._bytes_since_update
302 / (now - self._transport_update_time))
303 # using base-10 units (see HACKING.txt).
304- msg = ("%6dkB %5dkB/s" %
305+ msg = ("%6dkB %5dkB/s " %
306 (self._total_byte_count / 1000, int(rate) / 1000,))
307 self._transport_update_time = now
308 self._last_repaint = now

Subscribers

People subscribed via source and target branches