bugGNU nano - Bugs: bug #50687, with softwrap, the screen can fail...

 
 

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

bug #50687: with softwrap, the screen can fail to scroll

Submitter:  Benno Schulenberg <bens>
Submitted:  Fri 31 Mar 2017 09:54:05 AM UTC
   
 
Severity:  4 - Important Status:  Fixed
Assigned to:  bens Open/Closed:  Closed

Jump to the original submission

Thu 06 Apr 2017 05:55:33 PM UTC, comment #13: 

Fixed in git, f3e2ad94.

(Yes, the "Shifted" was just for testing purposes.  But it's good to add such things now and then, because it shows for example that somehow reset_cursor() is called unnecessarily when typing ^W, for example.)

Benno Schulenberg <bens>
Group administrator
Thu 06 Apr 2017 12:16:12 PM UTC, comment #12: 

Okay; I wasn't certain about the mbwidth() stuff.  Looks good to me; thanks.  (Does "Shifted" need to be displayed every time the code is run in the final version, or is that just for testing purposes?)

David Lawrence Ramsey <dolorous>
Group Member
Thu 06 Apr 2017 08:34:04 AM UTC, comment #11: 

Calling mbwidth() doesn't need checking for UTF-8 support first; that is handled in mbwidth() itself.  (The other calls of mbwidth() don't need to be guarded either; I've meaning to remove those, but didn't yet get around to it.)

Also, the "col == editwincols -1" test doesn't work for tabs.  So the right and sufficient test is quite simple: when placewewant and col don't agree, shift to next row.  See attached patch.

(file #40281)

Benno Schulenberg <bens>
Group administrator
Wed 05 Apr 2017 09:12:54 PM UTC, comment #10: 

Addendum to last comment: after some testing, it doesn't seem to cause a problem with End, actually, but since End uses similar logic to Home, I might just be missing something (or being overly paranoid).  Either way, it can go in the separate bug report.

David Lawrence Ramsey <dolorous>
Group Member
Wed 05 Apr 2017 08:57:15 PM UTC, comment #9: 

The patch seems good as a short-term fix.  I did tweak it a bit, though: calling mbwidth() requires checking for UTF-8 support first, as is done elsewhere; and it's not enough to just check if the character is less than 0x20 to see if it's a control character, since it might be a high-bit control character.  The tweaked version is attached.

The thing in comment #3 actually applies to both Home and End; I'll report it.

(file #40273)

David Lawrence Ramsey <dolorous>
Group Member
Wed 05 Apr 2017 07:38:25 PM UTC, comment #8: 

Attached patch avoids the scrolling problem.  It makes cursor behave a bit strange with double-width characters, but... I like it better in fact than the cursor jumping from the left to the right of the screen and back again when we're just holding <Up> or <Down>.  What do you think?

(The thing in comment #3 is a separate bug and needs rereporting.)

(file #40272)

Benno Schulenberg <bens>
Group administrator
Wed 05 Apr 2017 07:15:46 PM UTC, comment #7: 

Since actual_x() is what causes the issue here, the problem extends to up, down, Home, and End placing the cursor on the wrong line when converting from columns to indexes.  Using current_is_above_screen() or current_is_below_screen() won't fix it for up and down, since the line still won't be what the code is expecting with the current softwrap formulas.

Either there has to be some way to adjust the cursor in these particular cases (either a modified actual_x(), or some manual adjustment after the original actual_x()), or chunks have to be changed to variable-length to avoid putting two-column characters  (either control or Unicode) on the screen edge in the first place.

I haven't figured out how to do the former without breaking something else, and since 2.8.1 seems to be a relatively quick bugfix release, I figure the latter is out for that release.

David Lawrence Ramsey <dolorous>
Group Member
Mon 03 Apr 2017 11:58:56 PM UTC, comment #6: 

There's an easier way to reproduce it, from both the upward and downward directions, without the uncertainty of binary files.  This is because two-column Unicode characters, like control characters, can't be split on screen edges; whatever fixes this for one should fix it for the other.

1. Load the attached file chinese-page.html (based on one of the Chinese files in another bug, but with enough text duplicated to make more than one page) with the following options:

stty cols 80 && stty rows 24 && src/nano --ignore --softwrap --constant chinese-page.html

2. Press Down 19 times; nano won't scroll down (visually).

3. Press Ctrl-L to update the screen properly.

4. Press Up 9 times; nano won't scroll up (visually).

(file #40254)

David Lawrence Ramsey <dolorous>
Group Member
Mon 03 Apr 2017 08:14:06 PM UTC, comment #5: 

Okay.  I can reproduce it when scrolling up (not with your exact example, probably due to differences in binaries), but I'm not surprised that it occurs then too.  It relies on the same assumption as going down, but in reverse: that we're on the upper edge of the screen, but shifting backwards due to the control character on the edge puts it on the wrong line.

David Lawrence Ramsey <dolorous>
Group Member
Mon 03 Apr 2017 07:32:34 PM UTC, comment #4: 

Indeed, it can occur also when scrolling up (or rather: trying to scroll up).  Run:

  stty cols 80 && stty rows 24 && src/nano +,1521 --ignore --softwrap --constant src/nano

Then type nine times <Up>.  On the ninth time, the cursor fails to move; and on the tenth and subsequent times, the screen fails to scroll.

Benno Schulenberg <bens>
Group administrator
Sat 01 Apr 2017 07:22:31 PM UTC, comment #3: 

Also, dynamic home doesn't work quite right if a control character straddles a line; if the second half of the control character is at the beginning of a line, actual_x() shifts it back to the first half at the end of the previous line, which leads to its misdetecting the location of the beginning of the line and moving the cursor back.

David Lawrence Ramsey <dolorous>
Group Member
Fri 31 Mar 2017 05:47:20 PM UTC, comment #2: 

Check that; it's not edit_scroll() but near there.  It's that the check for being on the last line of the edit window relies on current_y's being (editwinrows - 1), but with the control character's shifting it up in this case, current_y is on the line before that.  (do_up() probably has the same problem for the checks for the first line of the edit window, so it should probably get an equivalent fix after this.)

The variable-length chunk changes would fix it by making sure we never straddle a control character on the edge of the screen like that.  Alternatively, using current_is_below_screen() to detect whether current has gone offscreen instead of the cheaper way would fix it.  Either way, that adds more processing time.  Unless there's another (cheaper) way to check if current has gone offscreen when it may not technically be on the last row?

David Lawrence Ramsey <dolorous>
Group Member
Fri 31 Mar 2017 05:38:14 PM UTC, comment #1: 

After some testing, it's not getting stuck (as in, not moving from a particular position); it's actually moving offscreen but not doing screen updates when it should.

Try it  with the options "--ignore --soft --constantshow" instead of just "--ignore --soft", and you'll see: when it appears to get stuck it really just isn't updating the screen when scrolling down, since the cursor position does change, and pressing Ctrl-L does update it.

Maybe bug #50691 is just another manifestation of the same problem, or maybe not, but it looks as though the problem is in edit_scroll() either way.

David Lawrence Ramsey <dolorous>
Group Member
Fri 31 Mar 2017 09:54:05 AM UTC, original submission:  

To reproduce, run:

  stty cols 80 && stty rows 24 && src/nano --ignore --soft src/nano

(Yes, that means opening nano's binary with nano itself.)

Now press and hold <Down>.  Just before reaching the last row, the cursor gets stuck on the "^" of a "^@" that straddles the row boundary.  (Or if your binary is much different, it might get stuck on a replacement character, �, indicating an invalid starter byte.)

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 (Posted a comment)
  • -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 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-04-14 bens Open/ClosedOpen Closed
    2017-04-06 bens StatusIn Progress Fixed
        Assigned toNone bens
    2017-04-06 bens Attached File- Added 0001-display-show-the-cursor-where-it-ought-to-be-not-on-.patch, #40281
    2017-04-05 dolorous Attached File- Added 0001-display-show-the-cursor-where-it-ought-to-be-not-on-.patch, #40273
    2017-04-05 bens Attached File- Added 0001-display-show-the-cursor-where-it-ought-to-be-not-on-.patch, #40272
        StatusNone In Progress
    2017-04-03 dolorous Attached File- Added chinese-page.html, #40254
    2017-04-03 bens Summarywith softwrap, the cursor can get stuck on straddling control codes with softwrap, the screen can fail to scroll
    2017-03-31 bens Severity3 - Normal 4 - Important
        Summarywith softwrap, the cursor gets stuck on straddling control codes with softwrap, the cursor can get stuck on straddling control codes

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code