bugGNU nano - Bugs: bug #51800, when a tab is split over rows,...

 
 

bug #51800: when a tab is split over rows, <Down> and <Up> go to the wrong place

Submitted by:  Benno Schulenberg <bens>
Submitted on:  Sun 20 Aug 2017 11:38:21 AM UTC  
 
Severity: 5 - BlockerStatus: Fixed
Assigned to: Benno Schulenberg <bens>Open/Closed: Closed

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Sun 20 Aug 2017 11:36:41 PM UTC, comment #13:

...and now that PageUp problem is properly reported in its own bug.

David Lawrence Ramsey <dolorous>
Project Administrator
Sun 20 Aug 2017 11:19:17 PM UTC, comment #12:

For the record, on small enough terminal sizes, the same issue that makes do_up() lock in place (even if it's worked around in current git) can make do_page_up() do so as well. In an 80x6 terminal, with current git (unpatched), run:

./nano --ignore --soft --nohelp --morespace -T241 NEWS

Press PageDown, and then PageUp. PageUp will mot move the cursor at all.

David Lawrence Ramsey <dolorous>
Project Administrator
Sun 20 Aug 2017 10:59:03 PM UTC, comment #11:

Argh. I officially have no idea what to do here. The issue with do_up()'s and do_page_up()'s use of proper_x() still stands, but I see no alternative that works as of yet. Although applying the fix in bug #51776 first would make this easier to debug.

David Lawrence Ramsey <dolorous>
Project Administrator
Sun 20 Aug 2017 10:27:24 PM UTC, comment #10:

Hmmm. Down can still behave oddly sometimes with the patches applied. In an 80x24 terminal, try:

./nano --ignore --soft -T241 +9,480 ../NEWS

and press Down 5 times; the screen won't update after the 5th press, ven with the display patch added; somehow edit_redraw() doesn't catch things. Also, the cursor scrolling to the right edge of one line after the 4th press may violate cursor positioning, but given how placewewant is technically far past that, it's actually expected; see below. Unless you want to limit placewewant to actual screen columns in softwrap mode, but I recall you already turned that down once.

At the end of each get_edge_and_target():

After 1st Down: *leftedge 240, placewewant 479, current_x 1, xplustabs() 241
After 2nd Down: *leftedge 480, placewewant 639, current_x 2, xplustabs() 482
After 3rd Down: *leftedge 0, placewewant 159, current_x 0, xplustabs() 0
After 4th Down: *leftedge 0, placewewant 159, current_x 70, zplustabs() 70
After 5th Down: *leftedge 0, placewewant 159, current_x 0, zplustabs() 0

David Lawrence Ramsey <dolorous>
Project Administrator
Sun 20 Aug 2017 09:46:15 PM UTC, comment #9:

Check that: Up can still lock sometimes with those patches applied. But making do_up() and do_page_up() use proper_x() to shift the cursor forward (and hence undo the upward movement, at least in part) will cause that. proper_x() makes Down and PageDown overshoot, while it makes Up and PageUp undershoot (and effectively not move sometimes), which is why the older version of the patch here didn't apply to to Up or PageUp.

David Lawrence Ramsey <dolorous>
Project Administrator
Sun 20 Aug 2017 09:09:24 PM UTC, comment #8:

And the formula to calculate overlong tabs in go_back_chunks() and go_forward_chunks() in the second patch is based on the last posted proper_x() patch, not the optimized version you committed; maybe the formula here can be optimized further, along the same lines, but I don't have any more time right now to do that.

David Lawrence Ramsey <dolorous>
Project Administrator
Sun 20 Aug 2017 08:39:33 PM UTC, comment #7:

And these two attached patches against git 243380c hopefully implement your go_back_chunks() and go_forward_chunks() overlarge tab skipping.

The first of them moves go_back_chunks() to before go_forward_chunks() to avoid function prototype issues. (It doesn't look obvious, but if you cut go_back_chunks() and paste it after go_forward_chunks(), making no other changes, that's what the diff looks like.)

The second of them renames go_back_chunks() and go_forward_chunks() to go_back_raw_chunks() and go_forward_raw_chunks(), and adds versions of go_back_chunks() and go_forward_chunks() that are wrappers over the raw versions and do the special tab handling. (Only the non-raw versions get prototypes, since only they should be called directly.) It also removes your temporary fix for do_up(), since it's no longer needed.

(file #41599, file #41600)

David Lawrence Ramsey <dolorous>
Project Administrator
Sun 20 Aug 2017 08:14:09 PM UTC, comment #6:

Okay, here's the simple fix.

The special handling of tabs also means that when moving through tabs wider than the screen, get_edge_and_target() can throw placewewant out of range, albeit indirectly. To demonstrate how, in git 243380c, in an 80x24 terminal, start gdb, put a breakpoint on get_edge_and_target(), and run:

./nano --ignore --soft --nowrap -T120 +9 ../NEWS

Press Down. In get_edge_and_target(), placewewant is 0, while *leftedge is calculated to be 0.

Press Down a second time. In get_edge_and_target(), placewewant is 80, while *leftedge is calculated to be 80.

Press Down a third time. In get_edge_and_target(), placewewant is 160, while leftedge is calculated to be 240. Since the function blindly subtracts placewewant from leftedge, it causes an underflow in parget_column, which throws off placewewant later, and hence throws off vertical positioning.

The attached patch fixes this; target_column will now be zeroed in such cases.

(file #41598)

David Lawrence Ramsey <dolorous>
Project Administrator
Sun 20 Aug 2017 08:05:49 PM UTC, comment #5:

I'm working on a proper fix for the do_up() business, but there's one simple fix that needs to go in. Give me a bit to resync it with current git.

David Lawrence Ramsey <dolorous>
Project Administrator
Sun 20 Aug 2017 07:27:06 PM UTC, comment #4:

Yes, I already had the do_home() part too, and a simplification in proper_x(). Pushed to master, 24a64d37. Thanks for the original patch.

Closing, because unreleased.

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Sun 20 Aug 2017 04:30:59 PM UTC, comment #3:

Hmmm. The tweaked proper_x() patch does work, aside from the issues you mention in comment #2. However, proper_x() also needs to be used for do_home(); otherwise, if a chunk ends in a straddling tab, Home will put the cursor at the beginning of the tab. For example, with the patch applied, in an 80x24 terminal, run:

./nano --ignore --soft --nowrap -T12

Press Tab 7 times (to add enough tabs to move to the next row), and then press "x". Now press Home. The cursor will be at the beginning of the tab instead of on the "x".

Attached a tweaked version of your tweaked version that fixes this by making do_home() use proper_x().

(file #41589)

David Lawrence Ramsey <dolorous>
Project Administrator
Sun 20 Aug 2017 01:56:19 PM UTC, comment #2:

Still, with the patch applied the behavior is better than that of 2.8.6, which will completely lock up as soon as you try to pass over a tab that spreads over more than two rows.

But I think that the right solution for huge tab sizes would be that go_back_chunks() and go_forward_chunks() pass over any empty chunk as if it doesn't exist when that chunk happens to be the final chunk of the movement.

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Sun 20 Aug 2017 01:26:28 PM UTC, comment #1:

Based on your proper_x() patch, attached patch fixes the problem for normal tab sizes. It passes your tabtest4 test. However, as soon as tabsize is larger than editwincols, <Up> can get stuck. Not good.

(file #41587)

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Sun 20 Aug 2017 11:38:21 AM UTC, original submission:

Forgetting for a moment about absurdly large tab sizes, even with a normal tab size, up and down navigation is erratic when there are tabs that cross a row boundary (during softwrapping). For example, run:

src/nano --ignore --soft --nowrap -T12 NEWS

Then type <End> <Ctrl+Left>, followed by enough tabs to push the word "ability" fully to the next row.

Then type M-\ and <Down>. The cursor goes to the end of the first row, instead of to the "a" of ability.

Then type M-G 2,0 <Enter> <Up> <Up>. Again, the cursor goes to the end of the first row and stays there, instead of going to the "a" of "ability" and then to the "2" of "2017".

These bugs need to be fixed first and foremost. All the bugs with tab sizes that exceed the screen's width are of a lesser order.

(For completeness: it is commit 65d16551 that triggered/exposed this problem.)

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.

 

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

Attach File(s):
   
   
Comment:
   

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 10 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sun 20 Aug 2017 08:39:33 PM UTCdolorousAttached File-=>Added 0001-tweaks-move-go_forward_chunks-before-go_back_chunks.patch, #41599
      Attached File-=>Added 0002-softwrap-iterate-past-chunks-made-of-tabs-wider-than.patch, #41600
    Sun 20 Aug 2017 08:14:09 PM UTCdolorousAttached File-=>Added 0001-softwrap-fix-placewewant-s-going-out-of-range-when-m.patch, #41598
    Sun 20 Aug 2017 07:27:06 PM UTCbensStatusIn Progress=>Fixed
      Assigned toNone=>bens
      Open/ClosedOpen=>Closed
    Sun 20 Aug 2017 04:30:59 PM UTCdolorousAttached File-=>Added 0001-softwrap-properly-move-up-down-home-over-tabs-that-a.patch, #41589
    Sun 20 Aug 2017 01:56:19 PM UTCbensStatusNone=>In Progress
    Sun 20 Aug 2017 01:26:28 PM UTCbensAttached File-=>Added 0001-softwrap-properly-move-up-and-down-over-tabs-that-ar.patch, #41587
    Sun 20 Aug 2017 11:38:21 AM UTCbensCarbon-Copy-=>Added dolorous

    Back to the top


    Powered by Savane 3.1-cleanup1