bugGNU nano - Bugs: bug #52183, nano does not hold desired column...

 
 

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

bug #52183: nano does not hold desired column when going <Up> over an overwide tab

Submitter:  Benno Schulenberg <bens>
Submitted:  Sat 07 Oct 2017 01:51:32 PM UTC
   
 
Severity:  3 - Normal Status:  Fixed
Assigned to:  bens Open/Closed:  Closed

Jump to the original submission

Wed 25 Oct 2017 03:51:41 PM UTC, comment #15: 

Fixed in git, 3018ab47.  Thanks for all the testing.

Benno Schulenberg <bens>
Group administrator
Fri 20 Oct 2017 07:10:03 PM UTC, comment #14: 

Right; the newer_x check was an attempt to work around the need for proper_x(), which as it turned out wasn't enough.

And no problem.  I've run the testing again with your new patch, and it passes all the tests the old patch did (nanocrap.txt, nanocrap2.txt, and nanocrap3.txt).

David Lawrence Ramsey <dolorous>
Group Member
Fri 20 Oct 2017 06:08:51 PM UTC, comment #13: 

When using proper_x() a second time, I don't think that the check for newer_x > current_x is needed.  So I propose the attached version.  Thanks for the extensive testing.

(file #42202)

Benno Schulenberg <bens>
Group administrator
Thu 19 Oct 2017 10:48:31 PM UTC, comment #12: 

And, for the record, even more detailed testing, with the file that was attached in comment #6:

nano --ignore --softwrap --nowrap -T 182 nanocrap3.txt

Vertical navigation starting from row 1, column 1 (Down 6 times, Up 6 times, Right once, repeat) works properly and preserves all columns on all rows containing text (row 1, row 3, row 5, and row 6). Home also preserves the column as the first available one on each line. In short, everything still works as expected, with the columns preserved.

David Lawrence Ramsey <dolorous>
Group Member
Thu 19 Oct 2017 09:21:58 PM UTC, comment #11: 

(The previous testing, as well as this testing, is done with an 80x24 terminal.)

Even more detailed testing:

nano --ignore --softwrap --nowrap -T 219 nanocrap2.txt

Vertical navigation starting from row 1, column 1 (Down 5 times, Up 5 times, Right once, repeat) works properly and preserves all columns on all rows containing text (row 1, row 3, row 4, and row 5).  Home also preserves the column as the first available one on each line.  In short, everything still works as expected, with the columns preserved.

(file #42199)

David Lawrence Ramsey <dolorous>
Group Member
Thu 19 Oct 2017 09:13:43 PM UTC, comment #10: 

More detailed testing with current git and my patch from comment #9 applied:

nano --ignore --softwrap --nowrap -T 132 nanocrap.txt

Starting at row 1, column 1, pressing Down 5 times moves 5 rows down, and then pressing Up 5 times moves 5 rows up, as expected, with the columns preserved.  Pressing Right, and repeating this for row 1, column 3 also works as expected.  Repeating this for row 1, and columns 5, 7, 9, 10, 12, 14, 16, 18, 20, 22, 24, 25, 27, and 28 also works as expected, with the columns preserved.

Starting at row 4, column 265, pressing Up 3 times moves 3 rows up, and then pressing Down 3 times moves 3 rows down, as expected, with the columns preserved.  Pressing Right, and repeating this for row 4, and columns 267, 269, 271, 273, 274, 276, 278, 280, 282, 284, 286, 288, 289, 290, 291, 293, 295, 297, 299, 301, 303, 305, 307, 309, 311, 313, 315, 317, and 319 also works as expected, with the columns preserved.

Starting at row 4, column 267 or later, pressing Home moves once to column 267, and pressing Up moves the cursor to column 161 of row 3, column 133 of row 2, and column 1 of row 1, as expected; this is of course different from ordinary vertical movement because Home keeps the cursor at the first available column of each line (shifting forward through overwide tabs), as it should.

(file #42198)

David Lawrence Ramsey <dolorous>
Group Member
Mon 16 Oct 2017 11:32:46 PM UTC, comment #9: 

Attached is a tweaked version of your last patch that seems to fix the issue, although I've only done preliminary testing so far.

It goes back to the first attempts to use proper_x() in what became set_proper_index_and_pww(); you said then that calling proper_x() twice wasn't necessary, and actual_x() should be enough for the second time.  However, in this case, it does seem to be necessary, since the second actual_x() is what's causing the backward shift.  (Moving up a row and keeping the same column puts us in the middle of a tab, and actual_x() moves the cursor back to the beginning of that tab, which is on the previous row, which means we moved up too high.)

(file #42175)

David Lawrence Ramsey <dolorous>
Group Member
Mon 16 Oct 2017 10:59:49 PM UTC, comment #8: 

And when tracing through it with the patch applied:

When moving up from row 5 in the testing, in set_proper_x_and_pww(), lines 107-110, the variables in question are:

was_edge = 240
*leftedge = 160
openfile->current_x = 56
newer_x = 42

The problem occurs because current_x is always set to newer_x if the leftedge shifts backward or the index shifts forward.  In this particular case, the leftedge shifts backward, but since newer_x gets recalculated to a value less than current_x, the cursor gets placed further back than it should be.

David Lawrence Ramsey <dolorous>
Group Member
Mon 16 Oct 2017 09:35:31 PM UTC, comment #7: 

To make the last post clearer, I mean that if you run the same test with current git (58ecc03) without the patch in comment #4 applied, the column position when moving upward is wrong (obviously), but pressing Up from row 5 does properly move from row 5 to row 3, and pressing Up from row 3 does properly move from row 3 to row 1.

David Lawrence Ramsey <dolorous>
Group Member
Mon 16 Oct 2017 08:50:48 PM UTC, comment #6: 

I just found a case where it regresses, relative to current git (using my nano binary, but I've cut it down to a smaller case).  With the attached file, and your patch from comment #4 applied, in an 80x24 terminal, run:

./nano --ignore --softwrap --nowrap -T 182 ../../nanocrap3.txt

Press Down twice.  From row 1, you'll move to row 3; then from row 3, you'll move to row 5.  Then press Up once.  From row 5, you'll move to row 1 (instead of row 3).

(file #42173)

David Lawrence Ramsey <dolorous>
Group Member
Sun 15 Oct 2017 04:25:54 PM UTC, comment #5: 

After some quick testing, it seems to work; thanks.  I'll keep testing it.

David Lawrence Ramsey <dolorous>
Group Member
Sun 15 Oct 2017 08:31:18 AM UTC, comment #4: 

Third attempt.

(file #42158)

Benno Schulenberg <bens>
Group administrator
Sun 15 Oct 2017 07:12:57 AM UTC, comment #3: 

It breaks the going backward over tabs where there is something on that same row after the tab.  The accepting of the newer_x has to be conditionalized, it has to accept it only when...

Benno Schulenberg <bens>
Group administrator
Sat 14 Oct 2017 07:35:45 PM UTC, comment #2: 

Thanks for your efforts.  I've also tried to fix this issue, right after finding and reporting it, but couldn't get it to do what I wanted.  My idea was that proper_x() should recompute the leftedge also when the index is not incremented, because the given leftedge does not correspond to the chunk that the cursor will actually be placed on.  But I couldn't make it work.

But now, on a second attempt, it does seem to work.  See attached patch, and tell me what it breaks.  :)

(file #42148)

Benno Schulenberg <bens>
Group administrator
Thu 12 Oct 2017 09:12:45 PM UTC, comment #1: 

I've looked into this, and I'm stumped for the moment.  Either proper_x() will have to be extended to shift backwards as well as forwards, or your other idea of making the chunk iterators skip over chunks entirely taken up by tabs will have to be implemented.

I've made attempts at the latter, and I haven't been able to do anything except break things in new and interesting ways.  However, maybe you can think of another way.  If you want to make an attempt, keep in mind that most of the chunk iterator calls assume that one chunk always takes up one row of the screen.  Breaking that assumption in all chunk iterator calls will lead to major problems; for example, with regard to display, place_the_cursor() needs to count exactly how many rows everything takes up.  The tab skipping will have to only be used when moving vertically.

For the record, the two attempts are attached; they're both against git d011983.  Maybe they can give you some ideas, if nothing else?

(file #42131, file #42132)

David Lawrence Ramsey <dolorous>
Group Member
Sat 07 Oct 2017 01:51:32 PM UTC, original submission:  

To reproduce, run:

  src/nano  --ignore --nowrap --softwrap  -T184

Then press and hold A until the cursor is nearly at the end of the first row.  Then press <Tab> once.  Then press and hold X until the cursor is somewhere in the center of the current row (which should be the third row -- the terminal should be between 80 and 90 columns wide).

Now press <Up>.  Instead of going to the center of the first row, the cursor goes to the tab character at the end of that row.  Wrong.

Now move the cursor to around position 20 in that first row.  Then press <Down>.  The cursor goes down two rows, to the corresponding column in the third row.  That's how it should be.  And an <Up> should bring it back where it came from: to that same column in the first row.

Benno Schulenberg <bens>
Group administrator

 

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

Attached Files
file #42199:  nanocrap2.txt added by dolorous (81B - text/plain)
file #42198:  nanocrap.txt added by dolorous (79B - text/plain)
file #42173:  nanocrap3.txt added by dolorous (77B - text/plain)
file #42131:  tabfix.patch added by dolorous (10KiB - text/x-patch)
file #42132:  tabfix2.patch added by dolorous (15KiB - 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 (Updated the item)
  • -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 13 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-11-19 bens Open/ClosedOpen Closed
    2017-10-25 bens StatusIn Progress Fixed
        Assigned toNone bens
    2017-10-20 bens Attached File- Added 0001-moving-don-t-slide-and-slither-over-tabs-when-they-a.patch, #42202
    2017-10-19 dolorous Attached File- Added nanocrap2.txt, #42199
    2017-10-19 dolorous Attached File- Added nanocrap.txt, #42198
    2017-10-16 dolorous Attached File- Added double-proper-x-when-edge-changed.patch, #42175
    2017-10-16 dolorous Attached File- Added nanocrap3.txt, #42173
    2017-10-15 bens Attached File- Added accept-newerx-when-edge-changed.patch, #42158
    2017-10-14 bens Attached File- Added recompute-leftedge-always.patch, #42148
        StatusNone In Progress
    2017-10-12 dolorous Attached File- Added tabfix.patch, #42131
        Attached File- Added tabfix2.patch, #42132

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code