patchGNU nano - Patches: patch #9215, Cleanups and fixes for...

 
 

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

patch #9215: Cleanups and fixes for reset_cursor()

Submitter:  David Lawrence Ramsey <dolorous>
Submitted:  Mon 09 Jan 2017 05:26:16 PM UTC
   
 
Priority:  3 - Low Status:  Done
Privacy:  Public Assigned to:  bens
Open/Closed:  Closed Release:  None

Sun 26 Feb 2017 07:04:58 PM UTC, comment #5: 

Good to know I could still contribute something.  Thank you.

David Lawrence Ramsey <dolorous>
Group Member
Sun 26 Feb 2017 04:37:03 PM UTC, comment #4: 

A change similar to yours has been pushed, 2e53ed29, but without changing any of the functionality, just improving the clarity.

Benno Schulenberg <bens>
Group administrator
Wed 11 Jan 2017 07:04:50 PM UTC, comment #3: 

Corrections to the bug description: it's display_buffer(), not load_buffer(), and display_buffer() also calls edit_refresh(), so if the buffer is an invalid state, that shouldn't be done either.

David Lawrence Ramsey <dolorous>
Group Member
Wed 11 Jan 2017 06:26:44 PM UTC, comment #2: 

Okay.  I've been using these changes for a while, and I'm pretty sure that overshoots of current_y are only handled by edit_redraw() and edit_refresh(), but you're right to be paranoid here.

As for current_y's being less than zero, it can happen in a way that is actually invalid, and it's a rather obscure case: your statusline() is too late to catch it.  You can reproduce it as follows:

1. Start in an 80x24 terminal.

2. Use gdb to start nano.

3. Inside gdb, set a breakpoint on reset_cursor(), and then run nano with "--ignore --softwrap reset_cursor-softwrap-test.txt".  (The file is attached.)

4. The first time reset_cursor() is called will trigger in gdb.  This call is actually from a side effect: when the file is first being read in via load_buffer(), load_buffer() calls titlebar(NULL) to display the filename, and titlebar() calls reset_cursor().  However, the buffer is not in a valid state at this point: current is identical to fileage, and edittop is identical to filebot.

5. Trace through reset_cursor() via gdb.  There's another problem as well, if you do this again without using --softwrap: since non-softwrap mode uses (current->lineno - edittop->lineno) to get the line number, current_y ends up being -1, but since softwrap mode adds the screen line count from edittop to current[current_x] and goes until it reaches NULL, current_y ends up being 4, which is erroneously in range.

Or maybe I'm overthinking it, and a better solution to this problem is to somehow disable the reset_cursor() call if the buffer is in this state?

As for doing the entire calculation for the softwrap case, I suppose it's just paranoia on my part, as I've been running these changes as part of the softwrap overhaul I'm still working on.  (edit_redraw() and edit_refresh() need fixes as well for their checks if current is offscreen, but that's a separate issue.)

(file #39436)

David Lawrence Ramsey <dolorous>
Group Member
Wed 11 Jan 2017 11:42:45 AM UTC, comment #1: 

I don't think it is a good idea to /not/ set current_y when the calculated value for it is out of range.  I am pretty sure that some of the current scrolling behavior depends on current_y overshooting the bottom of the window in order to move the cursor to the bottomline (after the screen has been repainted, not for this first call of reset_cursor()).

If you can show that all works fine when /not/ always setting current_y...

Also, I don't like the check for the calculated value being smaller than zero.  Because: could that ever happen?  (I've inserted a statusline(ALERT) line for that condition, and I cannot make it trigger.)  And if it /could/ happen, why do the whole calculation for the softwrap case when the current line is before edittop anyway?

Benno Schulenberg <bens>
Group administrator
Mon 09 Jan 2017 05:26:16 PM UTC, original submission:  

The attached patch set does the following for reset_cursor():

1. If the number of lines between edittop and current is out of range, current_y won't be set to it at all, instead of the current case where it's set to such a value but the cursor is not moved there.

2. Checks for current_y's being out of range also account for its being in the negative range, as in update_line() (i.e. if edittop is above current).

3. Some of the code is simplified.

4. Some comments and formatting are improved.

David Lawrence Ramsey <dolorous>
Group Member

 

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-04-02 bens Open/ClosedOpen Closed
    2017-02-26 bens Priority5 - Normal 3 - Low
        StatusNone Done
        Assigned toNone bens
    2017-01-11 dolorous Attached File- Added reset_cursor-current_y-test.txt, #39436
    2017-01-09 dolorous Attached File- Added 0005-Tweaks-to-reset_cursor-part-5-Reduce-the-amount-of-d.patch, #39405
        Attached File- Added 0006-Tweaks-to-reset_cursor-part-6-Add-clarifying-comment.patch, #39406
    2017-01-09 dolorous Attached File- Added 0001-Tweaks-to-reset_cursor-part-1-Make-sure-that-openfil.patch, #39401
        Attached File- Added 0002-Tweaks-to-reset_cursor-part-2-Improve-formatting.patch, #39402
        Attached File- Added 0003-Tweaks-to-reset_cursor-part-3-Make-sure-that-openfil.patch, #39403
        Attached File- Added 0004-Tweaks-to-reset_cursor-part-4-Simplify-handling-of-i.patch, #39404

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code