bugGNU nano - Bugs: bug #52139, with softwrap, <Down> can...

 
 

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

bug #52139: with softwrap, <Down> can take too large a step on overwide tabs

Submitter:  Benno Schulenberg <bens>
Submitted:  Thu 28 Sep 2017 07:49:28 PM UTC
   
 
Severity:  3 - Normal Status:  Fixed
Assigned to:  bens Open/Closed:  Closed

Jump to the original submission

Thu 12 Oct 2017 07:19:10 PM UTC, comment #12: 

Fixed in git, 7a3a45e6.  Thanks for the extensive testing.

Benno Schulenberg <bens>
Group administrator
Wed 11 Oct 2017 02:12:10 AM UTC, comment #11: 

I've put it through the same testing as before: in an 80x24 terminal, vertical upward movement (both <Up> and <PageUp>, with and without --smooth) works properly using tab sizes of 1, 2, 7, 9, 81, 132, 161, 219, 329, 2001, and 2397.

David Lawrence Ramsey <dolorous>
Group Member
Sat 07 Oct 2017 12:28:59 AM UTC, comment #10: 

After some testing, it seems to work (aside from the issue you mentioned, which I can see in the 219-column tab case in my testing of the earlier version).  Thanks.

David Lawrence Ramsey <dolorous>
Group Member
Thu 05 Oct 2017 08:08:44 PM UTC, comment #9: 

The problem is that the start of a tab can be anywhere.  The end of a tab can be computed with a simple formula, but a tab can start anywhere in maximum width that it occupies.  So the actual column position of the tab needs to be computed in order to compare it to the left edge.  See attached, improved patch.

(This is still not a complete fix for the tabs problem, because the cursor will be misplaced when it is in the middle of a row and moving up and a tab occupies several rows and begins near the end of a row: the cursor will be placed on the tab when it should be placed in the centre of that row.  But that will be reported in a different bug.)

Please test the current patch against the current bug.

(file #42071)

Benno Schulenberg <bens>
Group administrator
Sun 01 Oct 2017 10:11:00 PM UTC, comment #8: 

I've looked into it more, and proper_x() seems to be what's throwing off the leftedge and target into what you wouldn't expect (as far as I can tell; if you can give me a more specific example of what you don't expect, it would be more useful).

And having the chunk iterators skip over empty chunks wouldn't fix the problem entirely, since the case in comment #3 makes the tab only take up roughly half of one chunk on one row, and the other half on the next row, so neither chunk is empty.  Hmmm.

David Lawrence Ramsey <dolorous>
Group Member
Sun 01 Oct 2017 08:33:03 PM UTC, comment #7: 

Okay.

Just to be clear, by "a chunk that is completely empty," you mean a chunk where a tab begins before it and ends after it, so that the cursor cannot be placed anywhere on it?  ("No character on that row" to me means a blank line, but those can obviously exist.)

go_back_chunks() and go_forward_chunks() probably should move leftedge past such a chunk, but remember that the special handling of tabs was only added in 2.8.6, so they haven't been adapted for that possibility yet, and still assume that one chunk is always one full row at most; the row count would need adjustment when that isn't the case.  Not to mention, get_softwrap_breakpoint() would probably need some way to indicate such a chunk.

David Lawrence Ramsey <dolorous>
Group Member
Sun 01 Oct 2017 07:41:12 PM UTC, comment #6: 

Hm.  I guess the forward part needs a second condition, just like the !forward part has two conditions.  Will look into that later.

But... maybe the problem is elsewhere, because when applying the attached diagnostic patch, one sees that going up and landing in the middle of a tab, the leftedge and target are not what one would expect.  I would have thought that go_back_chunks() and go_forward_chunks() would never put leftedge on a chunk that is completely empty (there is no character at all on that row).  If go_back() and go_forward() wouldn't do that, we wouldn't need these position corrections.

(file #42045)

Benno Schulenberg <bens>
Group administrator
Sun 01 Oct 2017 04:52:44 PM UTC, comment #5: 

More info regarding the report in comment #3:

The problem occurs when pressing <Down> on anywhere from column 133 through column 159: that entire row, not counting the columns taken up by the tab.

More info regarding the report in comment #4:

The problem occurs when pressing <Down> on anywhere from column 220 through column 239: that entire row, not counting the columns taken up by the tab.

David Lawrence Ramsey <dolorous>
Group Member
Sun 01 Oct 2017 04:35:00 PM UTC, comment #4: 

And with the attached file nanocrap2.txt, run:

./nano --ignore --softwrap -T 219 +1,222 ./nanocrap2.txt

The same problem occurs: <Down> moves two rows, <Up> afterward moves one row.

(Both this test and the test in comment #3 should be run in an 80x24 terminal, by the way.)

(file #42042)

David Lawrence Ramsey <dolorous>
Group Member
Sun 01 Oct 2017 04:28:55 PM UTC, comment #3: 

Running the same tests listed in bug #52125 with my nano binary, I've found one (admittedly obscure) case where this problem still happens, even with your last patch applied.  With current git and your last patch applied, run:

./nano --ignore --softwrap -T 132 +1,135 nanocrap.txt

Press <Down> once, and the cursor will move down two rows.  Press <Up once>, and the cursor will move up only one row; you'll have to press <Up> twice to get back to the row you started on.

The nanocrap.txt file is attached; it contains a small enough chunk of my nano binary to show the problem.

(file #42039)

David Lawrence Ramsey <dolorous>
Group Member
Sun 01 Oct 2017 03:00:29 AM UTC, comment #2: 

Okay, that makes sense; thanks for the explanation.  And this patch does seem to work after some quick testing, although I still need to do the tests I did with the last patch to make sure.

The tests you added/changed that use (*leftedge - 1) and (editwincols - 1) do make me a bit nervous, given how nano is supposed to still function in a one-column terminal, but they don't seem to cause any problems so far under such circumstances.  (Although checking that did lead me to discover bug #52149.)

David Lawrence Ramsey <dolorous>
Group Member
Sat 30 Sep 2017 07:40:25 PM UTC, comment #1: 

Okay, I think I've got it solved now.  See attached patch.

Trying to explain the formula in words: when we've landed on a tab and we haved moved down (that is: forward) and the end of the row that preceeds the current row is effectively on the same tab as the current landing point, then not moving the index forward would mean we would recede a row from the current one, which is a no-no, so the index must be incremented.

Similarly, when moving up (that is: backward) [and we've landed on a tab] and the end of the preceding row is on the same tab as the landing point AND the end of the current row is NOT on the same tab as the landing point (which means that there is at least a tab character on the current row, maybe even text, but there IS something that we can put the cursor on), then the index must incremented because otherwise the cursor would skip a row that has something tangible on it.

I've dropped the other two conditions from the formula, as I don't remember what they're for (probably just approximations of the right formula).

(file #42019)

Benno Schulenberg <bens>
Group administrator
Thu 28 Sep 2017 07:49:28 PM UTC, original submission:  

To reproduce, run on an 80-column terminal:

  src/nano --ignore --softwrap -T84  +1,10  NEWS

The cursor will be on the "7" of "27".  Now press <Down> twice.  The cursor will land on the "i" of "wider".  But it now takes three presses of <Up> to get the cursor back to the starting position, on the "7".  This is wrong.  Any amount of <Up>s or <Down>s should require the exact same amount of the opposite key to get back to the starting point (as long as EOF and BOF are not reached).

It is the second <Down> that takes too large a stride -- it should stop on the tab character on the third row, before going to the "i" on the next row on the third <Down>.

Benno Schulenberg <bens>
Group administrator

 

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

Attached Files
file #42042:  nanocrap2.txt added by dolorous (81B - text/plain)
file #42039:  nanocrap.txt added by dolorous (79B - text/plain)
file #42019:  determine-when-to-nudge.patch added by bens (864B - text/x-patch)

 

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
  • -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 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-11-19 bens Open/ClosedOpen Closed
    2017-10-12 bens StatusIn Progress Fixed
    2017-10-05 bens Attached File- Added 0001-moving-prevent-the-cursor-sticking-on-or-skipping-ov.patch, #42071
    2017-10-01 bens Attached File- Added add-post-shift-indicator.patch, #42045
    2017-10-01 dolorous Attached File- Added nanocrap2.txt, #42042
    2017-10-01 dolorous Attached File- Added nanocrap.txt, #42039
    2017-09-30 bens Attached File- Added determine-when-to-nudge.patch, #42019
        StatusNone In Progress
        Assigned toNone bens
        Carbon-Copy- Added dolorous

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code