bugGNU nano - Bugs: bug #51676, changing vertical screen size when...

 
 

bug #51676: changing vertical screen size when smooth scrolling can place the cursor offscreen

Submitted by:  David Lawrence Ramsey <dolorous>
Submitted on:  Sun 06 Aug 2017 06:53:16 PM UTC  
 
Severity: 3 - NormalStatus: 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)

Thu 10 Aug 2017 11:00:44 AM UTC, comment #12:

Fixed in git, commit 06ffcfaf, with a subsequent improvement in ee3254b6.

(Maybe at a later stage it should not be the responsibility of edit_refresh() to check whether the cursor is offscreen. Maybe this check should be done in the places where it could actually happen: pasting, inserting, resizing, toggling. The other cases (the text jumps in move.c and the bracket matching in text.c) are handled by calls to edit_redraw().)

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Wed 09 Aug 2017 07:38:45 PM UTC, comment #11:

Thanks. And I'm seeing the warning then too, so it seems current_is_above_screen() is still necessary.

And after looking into it a bit, it would make sense that the message triggers then: resizing the viewport can't push the cursor above the top of the screen because of how it handles --morespace, but moving the viewport can, especially if the latter is done by a function that moves a non-constant number of lines (i.e., a number of lines that depends on the current text), like dynamic home or word movement. Vertical block movement via Ctrl-Up shows the warning too.

This is because the movement has to be done first, and the screen update second, so by definition the viewport can be out of range in the latter case, in either vertical direction.

David Lawrence Ramsey <dolorous>
Project Administrator
Wed 09 Aug 2017 07:13:03 PM UTC, comment #10:

Oops. Attached now.

And the message is easy to trigger after all: using Ctrl+Left a few times, or <Home> <Home> on a softwrapped line that is partially offscreen.

(file #41466)

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Wed 09 Aug 2017 07:03:20 PM UTC, comment #9:

I'll look into it, but your patch with the warning doesn't seem to actually be attached, so I'll need that first :)

David Lawrence Ramsey <dolorous>
Project Administrator
Wed 09 Aug 2017 05:38:55 PM UTC, comment #8:

Since I cannot imagine how the cursor could be pushed off to above the screen, I have applied the attached patch, that warns whenever the cursor somehow does end up above the screen.

I am unable to trigger the warning. But... if it is impossible for current_is_above_screen() to ever return TRUE, then... the function could simply be deleted, to save some code and resources. Are you able to trigger the message?

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Tue 08 Aug 2017 08:33:19 PM UTC, comment #7:

Hmmm. That does work, and if it makes other things simpler, even better. And current_y shouldn't be pushed above the top of the screen anyway, since toggling --morespace off via Meta-O when the cursor is at the top line of the edit window pushes the cursor down, not up.

David Lawrence Ramsey <dolorous>
Project Administrator
Tue 08 Aug 2017 07:38:43 PM UTC, comment #6:

Since current_y only needs to be set to the bottom when the cursor has actually been pushed offscreen, how about the attached patch instead? I cannot imagine a situation where the cursor is pushed offscreen to above the top row, so setting current_y to zero should never be needed.

With the attached patch applied, it even becomes possible to delete the settings of current_y from do_uncut_text() and do_insert_file(). So this looks to me like the better and most appropriate patch.

(file #41456)

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Tue 08 Aug 2017 06:24:31 PM UTC, comment #5:

Looks good to me.

David Lawrence Ramsey <dolorous>
Project Administrator
Tue 08 Aug 2017 05:49:23 PM UTC, comment #4:

Since total_refresh() is always called when the subwindows have changed, or /might/ have changed, how about the attached patch?

There should be no harm in always setting current_y this way, because at the end of edit_refresh() current_y will be set again to its actual value.

(file #41454)

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Tue 08 Aug 2017 03:36:02 AM UTC, comment #3:

Good catch. To fix that, the same logic added to do_toggle() should also be added to regenerate_screen() (and also just before the total_refresh() call). Updated patch attached.

(file #41451)

David Lawrence Ramsey <dolorous>
Project Administrator
Mon 07 Aug 2017 08:33:45 PM UTC, comment #2:

Yes, your patch fixes the reported issue. But it does not fix the tightly related issue of reducing the height of the screen (of the terminal emulator) when the cursor is on the bottom row of the edit window. See commits 2b385478 and 7b5cc1dc.

Strangely, I need to use 'openfile->current_y = editwinrows - 2' to get it to work for that other case.

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Sun 06 Aug 2017 06:55:06 PM UTC, comment #1:

And the attached patch fixes this.

(file #41440)

David Lawrence Ramsey <dolorous>
Project Administrator
Sun 06 Aug 2017 06:53:16 PM UTC, original submission:

In current git, 11072ed, there's a regression from 2.8.6 in terms of screen display. This can be reproduced as follows:

1. Run

./nano --ignore --smooth --constantshow --nohelp text.c

2. Press Down until the cursor is on the bottom row of the edit window.

3. Press Meta-X to toggle --nohelp off. The cursor will be displayed at the end of the line.

4. Hold down the Down key. The constant cursor position will keep updating, but the screen will not scroll, because the cursor was placed past the bottom of the screen when --nohelp was toggled.

This is another side effect of commit 6f9bb53's removal of the cap on the number of chunks to move backward: when the screen is resized in such a way that the cursor is placed offscreen, the screen is no longer refreshed properly afterwards.

David Lawrence Ramsey <dolorous>
Project Administrator

 

(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 bens (Posted a comment)
  • -unavailable- added by dolorous (Submitted the item)
  •  

    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 11 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Thu 10 Aug 2017 11:00:44 AM UTCbensStatusIn Progress=>Fixed
      Assigned todolorous=>bens
      Open/ClosedOpen=>Closed
    Wed 09 Aug 2017 07:13:03 PM UTCbensAttached File-=>Added warn-when-above.patch, #41466
    Tue 08 Aug 2017 07:38:43 PM UTCbensAttached File-=>Added only-set-target-when-actually-offscreen.patch, #41456
    Tue 08 Aug 2017 05:49:23 PM UTCbensAttached File-=>Added always-prepare-for-an-offscreen-cursor.patch, #41454
    Tue 08 Aug 2017 03:38:48 AM UTCdolorousSummaryturning help shortcuts on when smooth scrolling can place the cursor offscreen=>changing vertical screen size when smooth scrolling can place the cursor offscreen
    Tue 08 Aug 2017 03:36:02 AM UTCdolorousAttached File-=>Added 0001-display-fix-misplaced-cursor-when-screen-resizes-put.patch, #41451
    Mon 07 Aug 2017 08:33:45 PM UTCbensStatusNone=>In Progress
      Assigned toNone=>dolorous
    Sun 06 Aug 2017 06:55:06 PM UTCdolorousAttached File-=>Added 0001-display-fix-misplaced-cursor-when-screen-resizes-put.patch, #41440

    Back to the top


    Powered by Savane 3.1-cleanup1