bugGNU nano - Bugs: bug #51776, with current git, nano can fail to...

 
 

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

bug #51776: with current git, nano can fail to scroll when tabs are wider than the screen

Submitter:  David Lawrence Ramsey <dolorous>
Submitted:  Thu 17 Aug 2017 07:40:42 PM UTC
   
 
Severity:  4 - Important Status:  Fixed
Assigned to:  dolorous Open/Closed:  Closed

Jump to the original submission

Mon 21 Aug 2017 07:38:11 PM UTC, comment #13: 

Well, PageUp/PageDown just leave two rows of overlap.  Redrawing all rows, or scrolling and redrawing all rows minus two, that doesn't make much of a difference.  But when scrolling just one row...

So, normally I would have opted for the more economical version -- yours.  But now, with all the difficulties, I have gone for the one that makes things the most simple, the most straightforward.

Pushed to master, 5e105287.  Thanks for the original patch.

Benno Schulenberg <bens>
Group administrator
Sun 20 Aug 2017 08:56:39 PM UTC, comment #12: 

I would, but it's the logical extension of having the same done to the PageUp and PageDown code, which I recall used to use edit_scroll() too :)

David Lawrence Ramsey <dolorous>
Group Member
Sun 20 Aug 2017 06:52:46 PM UTC, comment #11: 

I thought you would protest against the great waste of CPU cycles: repainting the entire edit window when a simple scroll of one row would do.  :)

Benno Schulenberg <bens>
Group administrator
Sun 20 Aug 2017 03:25:03 PM UTC, comment #10: 

Looks good to me.

David Lawrence Ramsey <dolorous>
Group Member
Sun 20 Aug 2017 08:53:21 AM UTC, comment #9: 

I don't think the softwrap case for <Left> and <Right> needs special casing -- edit_redraw() will handle it just fine.  Similarly, the offscreen cases for <Up> and <Down> don't need an edit_scroll for the non-scroll_only case.  So, including a correction to do_right() (prev instead of next), I suggest the attached version of the patch.

(file #41584)

Benno Schulenberg <bens>
Group administrator
Sat 19 Aug 2017 08:00:08 PM UTC, comment #8: 

Attached new version that should fix it.

do_up() and do_down() now set placewewant after calling edit_redraw().

(I know it's redundant, but it's either that or add another parameter to edit_redraw() to disable setting placewewant, and if that parameter is only used in two places, I think it might be overkill, also considering that the original version of edit_redraw() took placewewant as a parameter before you changed it in aa1ae0a.  Making edit_redraw() return the new placewewant instead of setting it might also fix the redundancy, but that may be overkill as well for the same reason.)

Similarly, do_left() and do_right() no longer set placewewant before calling edit_redraw(), since it's unneeded.

(file #41581)

David Lawrence Ramsey <dolorous>
Group Member
Sat 19 Aug 2017 07:34:46 PM UTC, comment #7: 

The revamp breaks the placewewant mechanism.  With the patch from the previous comment applied, run:

  src/nano --ignore --soft src/nano

Press <End> to move the cursor to the end of the first chunk, then press and hold <Down>.  The cursor starts moving down but also moves left now and then.  And pretty soon the cursor will be back in the first column.  Not good.

Instead of returning to the target column near the right edge of the screen after having passed over shorter chunks, it seems that placewewant gets set when pressing <Down>.  It is edit_redraw() that sets placewewant.

Benno Schulenberg <bens>
Group administrator
Sat 19 Aug 2017 12:05:13 AM UTC, comment #6: 

Check that: edit_scroll() is also needed when we go offscreen during normal movement.

For testing purposes, I've attached an experimental patch that does use edit_redraw() after edit_scroll().  It fixes all the screen update problems when navigating through tabs wider than the screen.  However, it does so at the expense of making things somewhat slower.

(file #41577)

David Lawrence Ramsey <dolorous>
Group Member
Fri 18 Aug 2017 06:59:18 PM UTC, comment #5: 

If you apply the patch in #51778, and run:

./nano --ignore --softwrap -T 241 long-tab2.txt

pressing Right to navigate through long tabs is fixed, but the screen update problems still occur, because with tabs that long, when the cursor goes offscreen, it doesn't do so from the last row of the screen, which is what nano's screen update routines check for.  Short of scrapping edit_scroll() (aside from do_scroll_up() and do_scroll_down(), of course) and using edit_redraw() for everything instead, I don't know how to fix it.  And edit_redraw() calls current_is_offscreen().

(file #41573)

David Lawrence Ramsey <dolorous>
Group Member
Fri 18 Aug 2017 06:32:29 PM UTC, comment #4: 

The patch in bug #51778 should fix the Left/Right navigation issues.  I'm still concerned about the screen update problems, though; edit_scroll() isn't enough to get the cursor back on screen with such tabs, and cursor_is_below_screen() is expensive, as you've pointed out.  (And the patch here didn't properly handle Right/End navigation when putting such tabs on one line, now that I think about it.)

David Lawrence Ramsey <dolorous>
Group Member
Fri 18 Aug 2017 05:36:52 PM UTC, comment #3: 

Hm.  I have emacs 25.1.1, and when doing 'emacs -nw NEWS' (with only "(setq-default tab-width 181)" in the .emacs file), each indented line of the NEWS file takes up five rows on the screen, just like in vim, and just like I expect it in nano.  (In emacs it is much clearer what is happening, though, because of the continuation marks at the end of each row where a tab is softwrapped.)

So... I think the current displaying of huge tabs in nano is fine.  It's just the navigation that is handicapped: pressing <Down> can get stuck at the start of a tab, holding <Right> can keep traversing the same line again and again, and holding <Left> will quickly become fully stuck (but it's not a lockup, because <PgUp> or <PageDown> will get things moving again).

Benno Schulenberg <bens>
Group administrator
Thu 17 Aug 2017 07:46:47 PM UTC, comment #2: 

As for how to fix this problem in a way that won't require rewriting all the display code, there are two main options: vim and emacs.  Since vim's vertical navigation of lines is rather different from nano's, I've followed emacs (specifically, version 24.5).  If your .emacs contains the following:

(setq-default indent-tabs-mode nil)
(setq-default tab-width 161)
(setq indent-line-function 'insert-tab)

and you open long-tab.txt with it, and scroll down, you'll see that the way it handles such tabs is to have them take up no more than one row each, regardless of their actual width.

The attached patch makes nano behave this way as well.  Note that it applies on top of the latest patch for bug #51671, since that patch simplifies the special handling of tabs to occur in only one place instead of two.

(file #41559)

David Lawrence Ramsey <dolorous>
Group Member
Thu 17 Aug 2017 07:41:18 PM UTC, comment #1: 

This is with git e09dbf1, by the way.

David Lawrence Ramsey <dolorous>
Group Member
Thu 17 Aug 2017 07:40:42 PM UTC, original submission:  

This can be reproduced as follows:

1. In an 80x24 terminal, with the attached file, run:

./nano --ignore --constantshow --smooth --softwrap -T 161 +12 long-tab.txt

2. Press Down 7 times, followed by Right 2 times.  (Down will not move the cursor in the cases where Right is used, but that's a separate problem.)  The cursor will be offscreen, as you can tell from the cursor position, but the screen will not update.  Pressing Ctrl-L to forcibly refresh the screen will work, however.

This is due to the fact that nano's display update routines all assume that a chunk takes up one row or less.  Chunk-straddling tabs stretch this, but the routines still work as long as the columns added by the tab keep the line within one row.  A tabsize greater than one row thus causes problems, since it completely breaks that assumption.  (There would be similar problems with a tabsize of 81, but the above case shows the problem more easily.)

David Lawrence Ramsey <dolorous>
Group Member

 

(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 bens (Posted a comment)
  • -email is unavailable- added by dolorous (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 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-08-28 bens Open/ClosedOpen Closed
    2017-08-21 bens Severity3 - Normal 4 - Important
        StatusNone Fixed
        Assigned toNone dolorous
        Summarywith current git, nano can misdisplay lines containing tabs wider than the screen with current git, nano can fail to scroll when tabs are wider than the screen
    2017-08-20 bens Attached File- Added even-tighter.patch, #41584
    2017-08-19 dolorous Attached File- Added 0001-display-revamp-how-screen-updates-are-done-in-the-mo.patch, #41581
    2017-08-19 dolorous Attached File- Added 0001-display-revamp-how-screen-updates-are-done-in-the-mo.patch, #41577
    2017-08-18 dolorous Attached File- Added long-tab2.txt, #41573
    2017-08-17 dolorous Attached File- Added 0001-softwrap-make-tabs-wider-than-the-screen-take-up-onl.patch, #41559
    2017-08-17 dolorous Attached File- Added long-tab.txt, #41558

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code