bugGNU nano - Bugs: bug #51787, <Down> gets stuck at start...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #51787: <Down> gets stuck at start of tab when tabsize is huge

Submitter:  Benno Schulenberg <bens>
Submitted:  Fri 18 Aug 2017 07:58:13 PM UTC
   
 
Severity:  3 - Normal Status:  Fixed
Assigned to:  bens Open/Closed:  Closed

Jump to the original submission

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)

David Lawrence Ramsey <dolorous>
Group Member
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)

David Lawrence Ramsey <dolorous>
Group Member
Tue 22 Aug 2017 02:13:59 PM UTC, comment #16: 

Fixed in git, 97896d30.

Benno Schulenberg <bens>
Group administrator
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.

David Lawrence Ramsey <dolorous>
Group Member
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)

David Lawrence Ramsey <dolorous>
Group Member
Mon 21 Aug 2017 08:18:19 PM UTC, comment #13: 

Oops -- I should attach the patch.

(file #41608)

Benno Schulenberg <bens>
Group administrator
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.

Benno Schulenberg <bens>
Group administrator
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)

David Lawrence Ramsey <dolorous>
Group Member
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.

David Lawrence Ramsey <dolorous>
Group Member
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.

David Lawrence Ramsey <dolorous>
Group Member
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.

Benno Schulenberg <bens>
Group administrator
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.

Benno Schulenberg <bens>
Group administrator
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)

David Lawrence Ramsey <dolorous>
Group Member
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.

David Lawrence Ramsey <dolorous>
Group Member
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.

David Lawrence Ramsey <dolorous>
Group Member
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.

Benno Schulenberg <bens>
Group administrator
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)

David Lawrence Ramsey <dolorous>
Group Member
Fri 18 Aug 2017 08:02:21 PM UTC, comment #1: 

(Forgot to add the CC.)

Benno Schulenberg <bens>
Group administrator
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.

Benno Schulenberg <bens>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by dolorous (Updated the item)
  • -email is unavailable- added by bens
  • -email is unavailable- added by bens (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 15 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-08-22 dolorous Attached File- Added 0001-moving-recalculate-leftedge-if-current_x-is-pushed-f.patch, #41619
    2017-08-22 dolorous Attached File- Added 0001-moving-recalculate-leftedge-if-current_x-is-pushed-f.patch, #41618
    2017-08-22 bens StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2017-08-21 dolorous Attached File- Added binary-test, #41609
    2017-08-21 bens Attached File- Added modulate-the-pww.patch, #41608
    2017-08-21 bens Assigned todolorous bens
    2017-08-20 dolorous Attached File- Added 0001-softwrap-fix-placewewant-s-going-out-of-range-when-m.patch, #41603
    2017-08-20 dolorous Attached File- Added 0001-softwrap-improve-vertical-downward-movement-through-.patch, #41583
    2017-08-19 bens StatusNone In Progress
        Assigned toNone dolorous
    2017-08-18 dolorous Attached File- Added 0001-softwrap-improve-vertical-downward-movement-through-.patch, #41575
    2017-08-18 bens Carbon-Copy- Added dolorous
    2017-08-18 bens Carbon-CopyRemoved dol -
    2017-08-18 bens Carbon-Copy- Added dol

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code