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

 
 

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

Submitted by:  Benno Schulenberg <bens>
Submitted on:  Sat 07 Oct 2017 01:51:32 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)

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

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

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
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>
Project Administrator
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>
Project AdministratorIn charge of this item.
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>
Project Administrator
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>
Project Administrator
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>
Project Administrator
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>
Project Administrator
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>
Project Administrator
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>
Project Administrator
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>
Project Administrator
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>
Project Administrator
Sun 15 Oct 2017 08:31:18 AM UTC, comment #4:

Third attempt.

(file #42158)

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project Administrator
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>
Project AdministratorIn charge of this item.

 

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

Attach File(s):
   
   
Comment:
   

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
  • -unavailable- added by dolorous (Updated the item)
  • -unavailable- added by bens (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 13 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sun 19 Nov 2017 08:16:36 AM UTCbensOpen/ClosedOpen=>Closed
    Wed 25 Oct 2017 03:51:41 PM UTCbensStatusIn Progress=>Fixed
      Assigned toNone=>bens
    Fri 20 Oct 2017 06:08:51 PM UTCbensAttached File-=>Added 0001-moving-don-t-slide-and-slither-over-tabs-when-they-a.patch, #42202
    Thu 19 Oct 2017 09:21:58 PM UTCdolorousAttached File-=>Added nanocrap2.txt, #42199
    Thu 19 Oct 2017 09:13:43 PM UTCdolorousAttached File-=>Added nanocrap.txt, #42198
    Mon 16 Oct 2017 11:32:46 PM UTCdolorousAttached File-=>Added double-proper-x-when-edge-changed.patch, #42175
    Mon 16 Oct 2017 08:50:48 PM UTCdolorousAttached File-=>Added nanocrap3.txt, #42173
    Sun 15 Oct 2017 08:31:18 AM UTCbensAttached File-=>Added accept-newerx-when-edge-changed.patch, #42158
    Sat 14 Oct 2017 07:35:45 PM UTCbensAttached File-=>Added recompute-leftedge-always.patch, #42148
      StatusNone=>In Progress
    Thu 12 Oct 2017 09:12:45 PM UTCdolorousAttached File-=>Added tabfix.patch, #42131
      Attached File-=>Added tabfix2.patch, #42132

    Back to the top


    Powered by Savane 3.1-cleanup1