Fix devdraw's title and "open in top" on OSX and acme's infinite loop #1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
On OSX 10.10, when you open an application that depends on devdraw, the
title bar only shows the first letter of the application's name. The
patch sets a default title as soon as the window is created, which
fixes this issue.
On OSX 10.10, when you open an application that depends on devdraw, this
application is opened in top of other windows, however the menu bar is
not updated. The patch calls topwin() at the end of makewin() in
src/cmd/devdraw/cocoa-screen.m .
An infinite loop has been detected in libdraw that affects acme. It
can be reproduced following the following steps in acme:
1. Open a new "win"
2. Execute any command, e.g. "ls"
3. Press "ESC"
4. Press left
5. Press "ESC"
6. Press "ESC"
7. Acme freezes up
This patch makes sure that len is greater that 0 in _string() at
/src/libdraw/string.c:81
rsc
force-pushed
the
master
branch
2 times, most recently
from
f809cef
to
01463ba
Compare
|
Please see codereview(1) or the new CONTRIBUTING.md for the contribution process. Pull requests are not the most convenient form. I merged the devdraw patch in with your commit message, but I left the string.c change out. I followed up with a different fix for the string problem. acme was making an incorrect higher-level call. Thanks for the easy reproduction instructions. |
mkhl
added a commit
to mkhl/plan9port
that referenced
this pull request
Feb 1, 2018
Fixes 9fans#122. As reported in 9fans#122, file:1:1 moves to the end of the file, and file:1:2 fails with “address out of range”. I’ll use file:2:3 as an example so we can tell the line and column number apart. What’s happening is this: plumb/basic matches 2:3 using twocolonaddr (from plumb/fileaddr), then sets addr to `2-9fans#1+9fans#3` (the 1 is constant and was introduced because column numbers are 1-based). Acme interprets this in three steps: 1) find the range (q0, q1) that contains line 2 2) create the range (q2, q2) where q2 = q0 - 1 3) create the range (q3, q3) where q3 = q2 + 3 The second step has a branch where if q0 == 0 and 1 > 0 (remember that 1 is constant and comes form plumb/basic), q0 is set to _the end of the file_. This makes addressing things at the end of the file easier. The problem then is that if we select line 1, which starts at the beginning of the file, q0 is always 0 and the branch in step 2) will _always_ be used. 1:1 is interpreted as 1-9fans#1+9fans#1 which starts at 0, wraps around to the end of the file, then moves 1 character backwards and then forwards again, ending at the end of the file. 1:2 is interpretes as 1-9fans#1+9fans#2 which starts at 0, wraps around to the end od the file, then moves 1 character backwards and tries moving 2 characters forwards beyond the end of the file, resulting in the out of range error. I can think of several ways to solve or work around this problem: 1) Let column numbers start from 0. Plumb/basic would use `-#0` when setting addr, which does not move the left side of the range and would not wrap around. This would require no changes to the code but could break compatibility with people’s setups. 2) Create a new address notation for this use case, like `line:column`. The problem is essentially that the plumbing rule transforms something that declares _intent_ into a sequence of operations that _should_ match that intent but which can run into unexpected conditions when interpreted. New notation would preserve the intention and could be interpreted accordingly. This would require changes to every program that interprets addr, so at least acme and sam, and to the plumbing rules. 3) Let forward character motion wrap around to the beginning of the file. The rule in plumb/basic assumes that moving backwards and then forwards by 1 character ends at the start of the previous selection. Letting forward motion wrap around restores this property when interpreting addresses. This would require changes to addr.c, but I’m not sure whether there are other pieces of software or people’s setups that assume that forward motion does _not_ wrap around. It also seems like it would blur the conceptual lines between the beginning and the end of a file, which seems undesirable. 4) Require empty ranges to enable wrapping, delay out of range errors. The wrapping currently occurs whenever the current range starts at position 0, although (what I imagine to be) the most common use case is negative motion starting from the initial empty range (0,0). If we restrict wrapping so it happens only when the range is (0,0), the motion sequence mentioned above would have an intermediate invalid state, a range (-1,-1), which would result in an out of range error. If we skip generating that error we will arrive at the correct position. Just skipping the error handling here would mean that textsetselect would handle those errors, and it currently adjusts ranges instead of generating error messages. If that is undesirable, we could add error handling at the end of address. Instead of wrapping from (0,0), wrapping could also require a flag to number that address initially sets to TRUE but sets to FALSE once it has left (0,0). This seems more explicit about when it would allow wrapping but it would prevent addresses like file:1-0-9fans#1 from wrapping to the end. This change implements 4).
mkhl
added a commit
to mkhl/plan9port
that referenced
this pull request
Feb 1, 2018
Fixes 9fans#122. As reported in 9fans#122, `file:1:1` moves to the end of the file, and `file:1:2` fails with “address out of range”. I’ll use file:2:3 as an example so we can tell the line and column number apart. What’s happening is this: plumb/basic matches `2:3` using twocolonaddr (from plumb/fileaddr), then sets addr to `2-9fans#1+9fans#3` (the 1 is constant and was introduced because column numbers are 1-based). Acme interprets this in three steps: find the range (q0, q1) that contains line 2 create the range (q2, q2) where q2 = q0 - 1 create the range (q3, q3) where q3 = q2 + 3 The second step has a branch where if q0 == 0 and 1 > 0 (remember that 1 is constant and comes form plumb/basic), q0 is set to the end of the file. This makes addressing things at the end of the file easier. The problem then is that if we select line 1, which starts at the beginning of the file, q0 is always 0 and the branch in step 2) will always be used. `1:1` is interpreted as `1-9fans#1+9fans#1` which starts at 0, wraps around to the end of the file, then moves 1 character backwards and then forwards again, ending at the end of the file. `1:2` is interpretes as `1-9fans#1+9fans#2` which starts at 0, wraps around to the end od the file, then moves 1 character backwards and tries moving 2 characters forwards beyond the end of the file, resulting in the out of range error. @rsc proposed transforming `:X:Y` into `:X-#0+#Y-9fans#1` instead since that avoids wrapping around by not moving backwards at first. This change modifies `plumb/basic` to do that.
mkhl
added a commit
to mkhl/plan9port
that referenced
this pull request
Feb 2, 2018
Fixes 9fans#122. As reported in 9fans#122, `file:1:1` moves to the end of the file, and `file:1:2` fails with “address out of range”. I’ll use file:2:3 as an example so we can tell the line and column number apart. What’s happening is this: plumb/basic matches `2:3` using twocolonaddr (from plumb/fileaddr), then sets addr to `2-9fans#1+9fans#3` (the 1 is constant and was introduced because column numbers are 1-based). Acme interprets this in three steps: find the range (q0, q1) that contains line 2 create the range (q2, q2) where q2 = q0 - 1 create the range (q3, q3) where q3 = q2 + 3 The second step has a branch where if q0 == 0 and 1 > 0 (remember that 1 is constant and comes form plumb/basic), q0 is set to the end of the file. This makes addressing things at the end of the file easier. The problem then is that if we select line 1, which starts at the beginning of the file, q0 is always 0 and the branch in step 2) will always be used. `1:1` is interpreted as `1-9fans#1+9fans#1` which starts at 0, wraps around to the end of the file, then moves 1 character backwards and then forwards again, ending at the end of the file. `1:2` is interpretes as `1-9fans#1+9fans#2` which starts at 0, wraps around to the end od the file, then moves 1 character backwards and tries moving 2 characters forwards beyond the end of the file, resulting in the out of range error. @rsc proposed transforming `:X:Y` into `:X-#0+#Y-9fans#1` instead since that avoids wrapping around by not moving backwards at first. This change modifies `plumb/basic` to do that.
mkhl
added a commit
to mkhl/plan9port
that referenced
this pull request
Feb 7, 2018
Fixes 9fans#122. As reported in 9fans#122, `file:1:1` moves to the end of the file, and `file:1:2` fails with “address out of range”. I’ll use file:2:3 as an example so we can tell the line and column number apart. What’s happening is this: plumb/basic matches `2:3` using twocolonaddr (from plumb/fileaddr), then sets addr to `2-9fans#1+9fans#3` (the 1 is constant and was introduced because column numbers are 1-based). Acme interprets this in three steps: find the range (q0, q1) that contains line 2 create the range (q2, q2) where q2 = q0 - 1 create the range (q3, q3) where q3 = q2 + 3 The second step has a branch where if q0 == 0 and 1 > 0 (remember that 1 is constant and comes form plumb/basic), q0 is set to the end of the file. This makes addressing things at the end of the file easier. The problem then is that if we select line 1, which starts at the beginning of the file, q0 is always 0 and the branch in step 2) will always be used. `1:1` is interpreted as `1-9fans#1+9fans#1` which starts at 0, wraps around to the end of the file, then moves 1 character backwards and then forwards again, ending at the end of the file. `1:2` is interpretes as `1-9fans#1+9fans#2` which starts at 0, wraps around to the end od the file, then moves 1 character backwards and tries moving 2 characters forwards beyond the end of the file, resulting in the out of range error. @rsc proposed transforming `:X:Y` into `:X-#0+#Y-9fans#1` instead since that avoids wrapping around by not moving backwards at first. This change modifies `plumb/basic` to do that.
mkhl
added a commit
to mkhl/plan9port
that referenced
this pull request
Mar 27, 2018
Fixes 9fans#122, 9fans#140. As reported in 9fans#122, `file:1:1` moves to the end of the file, and `file:1:2` fails with “address out of range”. I’ll use file:2:3 as an example so we can tell the line and column number apart. What’s happening is this: plumb/basic matches `2:3` using twocolonaddr (from plumb/fileaddr), then sets addr to `2-9fans#1+9fans#3` (the 1 is constant and was introduced because column numbers are 1-based). Acme interprets this in three steps: 1. find the range (q0, q1) that contains line 2 2. create the range (q2, q2) where q2 = q0 - 1 3. create the range (q3, q3) where q3 = q2 + 3 The second step has a branch where if q0 == 0 and 1 > 0 (remember that 1 is constant and comes form plumb/basic), q0 is set to the end of the file. This makes addressing things at the end of the file easier. The problem then is that if we select line 1, which starts at the beginning of the file, q0 is always 0 and the branch in step 2) will always be used. `1:1` is interpreted as `1-9fans#1+9fans#1` which starts at 0, wraps around to the end of the file, then moves 1 character backwards and then forwards again, ending at the end of the file. `1:2` is interpretes as `1-9fans#1+9fans#2` which starts at 0, wraps around to the end od the file, then moves 1 character backwards and tries moving 2 characters forwards beyond the end of the file, resulting in the out of range error. In 9fans#140 @rsc proposed transforming `:X:Y` into `:X-#0+#Y-9fans#1` instead since that avoids wrapping around by not moving backwards at first. This change modifies `plumb/basic` to do that.
rsc
pushed a commit
that referenced
this pull request
Nov 14, 2018
Fixes #122, #140. As reported in #122, `file:1:1` moves to the end of the file, and `file:1:2` fails with “address out of range”. I’ll use file:2:3 as an example so we can tell the line and column number apart. What’s happening is this: plumb/basic matches `2:3` using twocolonaddr (from plumb/fileaddr), then sets addr to `2-#1+#3` (the 1 is constant and was introduced because column numbers are 1-based). Acme interprets this in three steps: 1. find the range (q0, q1) that contains line 2 2. create the range (q2, q2) where q2 = q0 - 1 3. create the range (q3, q3) where q3 = q2 + 3 The second step has a branch where if q0 == 0 and 1 > 0 (remember that 1 is constant and comes form plumb/basic), q0 is set to the end of the file. This makes addressing things at the end of the file easier. The problem then is that if we select line 1, which starts at the beginning of the file, q0 is always 0 and the branch in step 2) will always be used. `1:1` is interpreted as `1-#1+#1` which starts at 0, wraps around to the end of the file, then moves 1 character backwards and then forwards again, ending at the end of the file. `1:2` is interpretes as `1-#1+#2` which starts at 0, wraps around to the end od the file, then moves 1 character backwards and tries moving 2 characters forwards beyond the end of the file, resulting in the out of range error. In #140 @rsc proposed transforming `:X:Y` into `:X-#0+#Y-#1` instead since that avoids wrapping around by not moving backwards at first. This change modifies `plumb/basic` to do that.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
On OSX 10.10, when you open an application that depends on devdraw, the
title bar only shows the first letter of the application's name. The
patch sets a default title as soon as the window is created, which
fixes this issue.
On OSX 10.10, when you open an application that depends on devdraw, this
application is opened in top of other windows, however the menu bar is
not updated. The patch calls topwin() at the end of makewin() in
src/cmd/devdraw/cocoa-screen.m .
An infinite loop has been detected in libdraw that affects acme. It
can be reproduced following the following steps in acme:
1. Open a new "win"
2. Execute any command, e.g. "ls"
3. Press "ESC"
4. Press left
5. Press "ESC"
6. Press "ESC"
7. Acme freezes up
This patch makes sure that len is greater that 0 in _string() at
/src/libdraw/string.c:81