Tue 22 Aug 2017 10:29:23 PM UTC, comment #18:
Forget that version. The attached version properly sets placewewant when paging down as well as moving down one line. (It also sets placewewant at the end of the function even for PageUp; it's more consistent with PageDown, and doesn't change how PageUp works anyway.)
(file #41619)
|
Tue 22 Aug 2017 09:19:23 PM UTC, comment #17:
After some thought, I've tried to implement the suggestion in commit 047cf9b. The attached patch against git a850bb2 may do what you need, although without a specific test case, I'm not certain. In the process, it also adds the same workaround for non-softwrap uses of leftedge_for() that's already in chunk_for(); maybe that part is a good idea on its own?
(I'm posting it here because it's related to this issue, even if this issue is technically fixed.)
(file #41618)
|
Tue 22 Aug 2017 02:13:59 PM UTC, comment #16:
Fixed in git, 97896d30.
|
Mon 21 Aug 2017 09:24:44 PM UTC, comment #15:
I've experimentally changed the formula in proper_x() from the optimized one currently in nano:
if (ISSET(SOFTWRAP) && text[index] == '\t' && column < leftedge + leftedge % tabsize)
back to the one in the original patch:
if (ISSET(SOFTWRAP) && text[index] == '\t' && leftedge % tabsize != 0 && column < leftedge + tabsize)
and doing so fixes the bug in comment #14. Apparently the optimized formula isn't equivalent to the original formula in all cases.
|
Mon 21 Aug 2017 08:56:52 PM UTC, comment #14:
Looks good to me. Although while testing it, I found another movement bug with Down, that occurs both with these patches and without (using git 5e10528), that may be related to the original bug here. In an 80x24 terminal, run:
./nano --ignore --soft --nowrap -T181 ./binary-test
with the attached file. (The file is the first few lines of my nano binary, just to be sure you can reproduce it.) After the file loads, press End, and then press Down three times. The first Down will move down to the next line; the second will move to the end of the current line; and the third will lock the cursor in place.
(file #41609)
|
Mon 21 Aug 2017 08:18:19 PM UTC, comment #13:
Oops -- I should attach the patch.
(file #41608)
|
Mon 21 Aug 2017 08:17:41 PM UTC, comment #12:
Applying the attached patch on top of the one in bug #51809 gets rid of the erratic moevement. If you have improvements, let me know.
|
Sun 20 Aug 2017 10:56:22 PM UTC, comment #11:
Forget the stuff in bug #51800; that issue and this are intertwined, but only the one patch there actually fixes the problem in comment #8 of this bug.
Attached a cleaned-up version of that patch here. (Sorry for the mess; all these softwrap-tab problems are getting horrendously confusing.)
(file #41603)
|
Sun 20 Aug 2017 08:59:55 PM UTC, comment #10:
The aforementioned comment links point to the wrong bug, but you know which ones I mean.
|
Sun 20 Aug 2017 08:59:05 PM UTC, comment #9:
If you apply the patches in comment #6 and comment #7 of bug #51800, and then manually apply the patch at bug #51776 (so that you can tell when the screen freezes due to movement problems instead of screen update problems), that test case seems to be fixed.
|
Sun 20 Aug 2017 07:37:10 PM UTC, comment #8:
In current git (243380c4) the behavior with <Down> is better, but still not fully fixed. Do for example:
src/nano --ignore --soft -T184 NEWS
Then press <Down> many times. On some positions, the cursor sticks for one or two keystrokes. If you then press <Up> a few times, then <Down> will get stuck forever -- well, until you move the cursor in some other way.
|
Sun 20 Aug 2017 09:07:20 AM UTC, comment #7:
With the latest patch the behavior is better, but not yet correct for <Up>. To see what I mean, run:
src/nano --ignore -T55 --soft --nowrap src/nano
Then press <End> <Left> <Tab>. Then press M-\ followed by <Down> <Down>. As desired, the cursor stays as close to column zero as possible. Now press <Up>. The cursor does not go back where it came from (the first possible column in the second row) but jumps to the end of the first row. Incorrect. At the very least <Up> <Down> and <Down> <Up> should be invariant.
Also, the behavior for <Down> is not yet fully correct when a tab sprawls over three rows. Run:
src/nano --ignore -T181 --soft --nowrap src/nano
Then press and hold <Down>. Soon the cursor will be moving down near the right edge of the screen. The desired behavior is: to stay in column 0 whenever possible.
|
Sun 20 Aug 2017 01:24:27 AM UTC, comment #6:
Check that: it's thrown off, but it's still more accurate than before. After noting that Pico-style paging converts between firstcolumn and current_x (column to index, with the associated inaccuracy if firstcolumn is in the middle of a tab), I've taken a different approach that gives the same results as the proper_x() changes.
Specifically, when not in smooth scrolling mode, instead of calling get_edge_and_target() and doing the conversion, nano now just sets leftedge to firstcolumn and target_column to 0 directly, just before running the chunk iterators in order to actually move. The formula still sets placewewant to the proper result afterward, and it requires fewer variables to be set beforehand, so it's simpler and more accurate.
There are still a few oddities when scrolling up and down via the arrow keys afterward, but they're caused by the aforementioned positioning problems caused by the tab shifts.
The new patch is attached.
(file #41583)
|
Sat 19 Aug 2017 11:42:29 PM UTC, comment #5:
Also, on top of the patch, I've tried using proper_x() in both PageUp and PageDown (at the Pico-like paging starting from the top line of the screen), solely for testing purposes to see if it affects this, and it doesn't do anything useful. With the patch, but without the proper_x() change:
Before movement: lineno 1, xplustabs() 0, current_x 0, placewewant 0
After 1st PageDown: lineno 7, xplustabs() 91, current_x 1, placewewant 80
After 2nd PageDown: lineno 14, xplustabs() 0, current_x 0, placewewant 80
After 1st PageUp: lineno 7, xplustabs() 0, current_x 0, placewewant 0
With the patch, and with the proper_x() change:
Before movement: lineno 1, xplustabs() 0, current_x 0, placewewant 0
After 1st PageDown: lineno 7, xplustabs() 91, current_x 1, placewewant 80
After 2nd PageDown: lineno 14, xplustabs() 91, current_x 1, placewewant 80
After 1st PageUp: lineno 7, xplustabs() 0, current_x 0, placewewant 80
The vertical position is thrown off either way, just slightly differently.
|
Sat 19 Aug 2017 06:39:05 PM UTC, comment #4:
If I understand you correctly here, there are two main requirements for navigating through softwrapped lines:
1. The cursor should not lock in place if it can move.
2. Vertical cursor movement should be invariant.
Ideally, both requirements should be met. However, the fact that the cursor can never be in the middle of a tab, and needs to be shifted outside of it, leads to some corner cases where only one of the two requirements can be met. And your test case just hit one of them.
If there's a tab that takes up more than one row, we can either shift back to where it begins or forward to where it ends. On any given line with such a tab, shifting forward when moving upward, or shifting backward when moving downward, risks breaking requirement #1; and shifting backward when moving upward, or shifting forward when moving downward, risks breaking requirement #2.
Since it's impossible to meet both requirements in these cases, I prefer to meet only requirement #1, since locking the cursor stops navigation, while making vertical cursor movement variant merely throws off navigation while not stopping it. In other words, if the only way to keep the cursor from locking when moving down is to overshoot a bit (and thus throw off vertical cursor positioning for when we move back up), so be it.
For a simpler example, with pristine nano git, try running:
./nano --ignore --const --soft -T 241
Press Meta-P to turn on whitespace display. Then press Tab, followed by PageUp. Now, that you're at the top of the file, press Meta-+ twice to scroll down two rows.
nano will not scroll, because the new position is inside the tab, and the shift backward puts the cursor at the beginning of the tab, where we started. (You'll be able to tell because of the cursor position display; the screen won't be updated properly due to bug #51776, but you can press Ctrl-L to manually update the screen.) Thus, requirement #1 is broken, while requirement #2 is met. Now press Meta-- twice to scroll back up two rows. Since nano didn't scroll down, the cursor position never changed, and the top of the file is still visible, so nano will not scroll up either.
Now apply my previous patch here, and follow the same instructions again.
With the patch applied, nano will scroll, because the new position is inside the tab, and the shift forward puts the cursor at the end of the tab, after where we started. However, the cursor position will shift down more than one row, due to the tab's being more than one row wide. Thus, requirement #1 is met, while requirement #2 is broken. Now press Meta-- twice to scroll back up two rows. Since the cursor position was thrown off by scrolling down, it will not be preserved when scrolling back up, but at least it will scroll.
|
Sat 19 Aug 2017 03:10:15 PM UTC, comment #3:
The patch fixes the problem for <Down>: it no longer gets stuck. Thanks.
But... it does not fix the problem for <PageUp>. In other words: a combination of <PageDown> <PageUp> is not always invariant, and it should be.
To reproduce, on your 80x24 terminal, run:
src/nano --ignore -T91 --soft NEWS
Then keep typing these three keystrokes: ^V ^V ^Y. After six cycles you'll notice that the cursor is at the end of a line, after "boundaries are involved". It should not be possible for the cursor to get there when just doing <PageUp> and <PageDown> with the cursor starting in the leftmost column. Along the way you might have noticed that sometimes the ^Y did not bring back the screen to where the ^V two strokes earlier had brought it.
|
Fri 18 Aug 2017 09:37:33 PM UTC, comment #2:
I had to revive the proper_x() code to shift the index forward from an earlier attempt at bug #51621; this patch with it against git 54a9261 fixes the problem.
It avoids the problems caused by the earlier version by only being used at the places it's needed: do_down() and do_page_down() (which also avoids the issue of having a function that's only called in one place). The former now moves properly.
The latter also moves properly if you PageDown into the middle of a huge tab, since the same actual_x() index shift that freezes Down incorrectly shortens the number of rows paged down. For an example of that problem, run:
src/nano --ignore -T181 --soft NEWS
and immediately press PageDown. It should move to the next-to-next-to-last row (and start on "be activated with"), but it moves to the tab before that. With the patch applied, it does what it should.
I know, it's a separate issue, but since it's intertwined with this one due to how the movement code works, maybe the bug subject should change to something like "<Down> gets stuck, <PageDown> gets shortened with huge tabs"?
(file #41575)
|
Fri 18 Aug 2017 08:02:21 PM UTC, comment #1:
(Forgot to add the CC.)
|
Fri 18 Aug 2017 07:58:13 PM UTC, original submission:
To reproduce, run:
src/nano --ignore -T181 --soft NEWS
Then type <Down> a few times. After the first step, the cursor no longer moves. For some reason nano's logic chokes on the large tab.
|