Merge lp:~vila/bzr/316357-SIGWINCH into lp:bzr

Proposed by Vincent Ladeuil on 2009-12-10
Status: Merged
Merged at revision: not available
Proposed branch: lp:~vila/bzr/316357-SIGWINCH
Merge into: lp:bzr
Diff against target: 75 lines (+23/-6)
3 files modified
NEWS (+3/-0)
bzrlib/osutils.py (+15/-0)
bzrlib/ui/text.py (+5/-6)
To merge this branch: bzr merge lp:~vila/bzr/316357-SIGWINCH
Reviewer Review Type Date Requested Status
Martin Pool 2009-12-10 Needs Fixing on 2009-12-15
Martin Packman (community) Needs Fixing on 2009-12-14
Review via email: mp+15939@code.launchpad.net
To post a comment you must log in.
Vincent Ladeuil (vila) wrote :

Due to popular demand (see https://bugs.launchpad.net/bugs/316357), I've put this patch up for review.

bzr will now listen to the SIGWINCH signal but it may reveal dormant bugs in parts of the code that don't yet handle EINTR.

Feedback and bug reports welcome: don't sit bored anymore watching the progress bar during those long test suite runs, shake your window size, that will shake the bzr code :)

Martin Packman (gz) wrote :

This change needs to be if-nix onlyed. SIGWINCH doesn't exist on windows, and this is not a neat hack there due to lack of size-shaking. Changes to the console window size are communicated using WINDOW_BUFFER_SIZE_RECORD from ReadConsoleInput, which isn't in the bazaar loop currently.

review: Needs Fixing
Martin Pool (mbp) wrote :

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

 vote needs-fixing

Aside from Martin.gz's comment it looks reasonable to me.

> Changes to the console window size are communicated using WINDOW_BUFFER_SIZE_RECORD from ReadConsoleInput, which isn't in the bazaar loop currently.

Maybe on Windows we should just call _win32_terminal_size every time
we want to know?

- - --
Martin <http://launchpad.net/~mbp/>
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)

iEYEARECAAYFAksnQd8ACgkQPGPKP6Cz6It9uQCfTkNUg4vS6Pi4DogMGgjy7OBZ
MUsAn3bd9NBHDMRYk4GTXTwMx/llmE48
=doaZ
-----END PGP SIGNATURE-----

review: Needs Fixing
Martin Packman (gz) wrote :

> Maybe on Windows we should just call _win32_terminal_size every time
> we want to know?

That's perfectly acceptable for this kind of application, yes. (Or even just assuming it doesn't change - it's not very easy for the user to fiddle with after all).

Also, please make the signal import lazy, will save me doing that in one of my branches after this lands and breaks it.

lp:~vila/bzr/316357-SIGWINCH updated on 2009-12-16
4763. By Vincent Ladeuil on 2009-12-16

Review feedback: import signal lazily and don't install SIGWINCH on windows.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2009-12-15 19:59:00 +0000
3+++ NEWS 2009-12-16 10:53:22 +0000
4@@ -141,6 +141,9 @@
5
6 * Interactive merge doesn't leave branch locks behind. (Aaron Bentley)
7
8+* Listen to the SIGWINCH signal to update the terminal width.
9+ (Vincent Ladeuil, #316357)
10+
11 * Lots of bugfixes for the test suite on Windows. We should once again
12 have a test suite with no failures on Windows. (John Arbash Meinel)
13
14
15=== modified file 'bzrlib/osutils.py'
16--- bzrlib/osutils.py 2009-12-14 16:16:05 +0000
17+++ bzrlib/osutils.py 2009-12-16 10:53:22 +0000
18@@ -39,6 +39,7 @@
19 from shutil import (
20 rmtree,
21 )
22+import signal
23 import subprocess
24 import tempfile
25 from tempfile import (
26@@ -1427,6 +1428,20 @@
27 _terminal_size = _ioctl_terminal_size
28
29
30+def _terminal_size_changed(signum, frame):
31+ """Set COLUMNS upon receiving a SIGnal for WINdow size CHange."""
32+ width, height = _terminal_size(None, None)
33+ if width is not None:
34+ os.environ['COLUMNS'] = str(width)
35+
36+if sys.platform == 'win32':
37+ # Martin (gz) mentioned WINDOW_BUFFER_SIZE_RECORD from ReadConsoleInput but
38+ # I've no idea how to plug that in the current design -- vila 20091216
39+ pass
40+else:
41+ signal.signal(signal.SIGWINCH, _terminal_size_changed)
42+
43+
44 def supports_executable():
45 return sys.platform != "win32"
46
47
48=== modified file 'bzrlib/ui/text.py'
49--- bzrlib/ui/text.py 2009-12-14 06:05:30 +0000
50+++ bzrlib/ui/text.py 2009-12-16 10:53:22 +0000
51@@ -248,9 +248,6 @@
52 self._term_file = term_file
53 # true when there's output on the screen we may need to clear
54 self._have_output = False
55- # XXX: We could listen for SIGWINCH and update the terminal width...
56- # https://launchpad.net/bugs/316357
57- self._width = osutils.terminal_width()
58 self._last_transport_msg = ''
59 self._spin_pos = 0
60 # time we last repainted the screen
61@@ -267,9 +264,11 @@
62
63 def _show_line(self, s):
64 # sys.stderr.write("progress %r\n" % s)
65- if self._width is not None:
66- n = self._width - 1
67- s = '%-*.*s' % (n, n, s)
68+ width = osutils.terminal_width()
69+ if width is not None:
70+ # we need one extra space for terminals that wrap on last char
71+ width = width - 1
72+ s = '%-*.*s' % (width, width, s)
73 self._term_file.write('\r' + s + '\r')
74
75 def clear(self):