bugGNU nano - Bugs: bug #51621, softwrapping extends or...

 
 

bug #51621: softwrapping extends or foreshortens tabs

Submitted by:  Benno Schulenberg <bens>
Submitted on:  Sun 30 Jul 2017 07:23:37 AM UTC  
 
Severity: 3 - NormalStatus: Fixed
Assigned to: David Lawrence Ramsey <dolorous>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)

Sun 13 Aug 2017 07:09:31 PM UTC, comment #22:

Thanks. Pushed to master, 65d16551.

Benno Schulenberg <bens>
Project Administrator
Sun 13 Aug 2017 04:19:45 PM UTC, comment #21:

Looks good to me. I'll update the patch at bug #51671 to account for it.

David Lawrence Ramsey <dolorous>
Project AdministratorIn charge of this item.
Sun 13 Aug 2017 02:51:24 PM UTC, comment #20:

I don't think the prev_index variable is needed. So I suggest the attached version of the patch.

(file #41504)

Benno Schulenberg <bens>
Project Administrator
Sun 13 Aug 2017 09:39:30 AM UTC, comment #19:

Okay. I checked on the wrong line, since you didn't mention the number of lines to scroll exactly (which you'vew fixed in the new bug report). Still, it did point me toward problems in the existing patches, and the fixes to them are needed.

David Lawrence Ramsey <dolorous>
Project AdministratorIn charge of this item.
Sun 13 Aug 2017 08:43:28 AM UTC, comment #18:

To comment #14: no, that was not with both patches applied, but with just the first one. As I always say: one issue at a time.

But... in fact, the problem is not caused by your patch at all. It is present also in master. :| Sorry for not checking properly. This new bug has been rereported as bug #51743.

Benno Schulenberg <bens>
Project Administrator
Sun 13 Aug 2017 06:28:18 AM UTC, comment #17:

...and that incorrect movement example obviously assumes you have the last patch with proper_x() applied already; it doesn't occur with nano with the new patch applied.

However, after noting that, with this patch applied, nano handles tabs the same way vim does, I've also noted that vim splits display of control characters over two lines, where nano treats them the same way as two-column Unicode and doesn't. I prefer the latter behavior, since I wrote it that way and it looks better to me, but I wonder whether vim's behavior is the more common case; should it change for nano?

David Lawrence Ramsey <dolorous>
Project AdministratorIn charge of this item.
Sun 13 Aug 2017 06:13:49 AM UTC, comment #16:

Argh. It turns out that the index shifting is more complex than I thought, and can lead to cases where the cursor doesn't move when it should, which I think is worse than the alternative (without index shifting, vertical positioning is sometimes not quite preserved due to the inability to place an index inside a tab).

In the interest of having tabs handled properly, even if there are some oddities with movement, here's the last patch with proper_x() removed.

For an example of incorrect movement caused by proper_x(), load the attached file with:

./nano --ignore --softwrap --atblanks tabtest4.txt

in a 60x24 terminal, press PageDown to go to the last line, then press Left to go to the end of the last line, and then press Up a few times. The cursor will eventually lock in place when it shouldn't, and Up will no longer move the cursor.

(file #41500, file #41501)

David Lawrence Ramsey <dolorous>
Project AdministratorIn charge of this item.
Sun 13 Aug 2017 03:34:58 AM UTC, comment #15:

Done. Apply the patch in comment #14, then apply the newest patch at bug #51671, and the problem should be gone.

David Lawrence Ramsey <dolorous>
Project AdministratorIn charge of this item.
Sun 13 Aug 2017 03:29:50 AM UTC, comment #14:

I was confused by this, until I checked: the first patch that splits tabs across chunks doesn't cause the problem; the second one on top of it that handles Unicode multi-column spacing does, because it tries to be too clever when melding logic for both cases.

Attached an updated version of the first patch anyway, since its changelog entry wasn't properly updated when I split the Unicode handling into its own patch. An updated version of the second patch is forthcoming, at its associated bug.

(file #41498)

David Lawrence Ramsey <dolorous>
Project AdministratorIn charge of this item.
Sat 12 Aug 2017 09:21:51 AM UTC, comment #13:

Patch does strange things. To reproduce, run:

src/nano --ignore --nowrap --softwrap --atblanks doc/faq.html

and type M-+ until the second chunk of a softwrapped line is on the top row. Then type <Tab>. See how some amount of whitespace is inserted and how then the cursor jumps to somewhere in the next row. Then type M-P to switch on visible whitespace. The statusbar exclaims: "Badness: tried to display...".

Benno Schulenberg <bens>
Project Administrator
Sun 06 Aug 2017 03:34:06 PM UTC, comment #12:

I figured the patch with less code was better to put first, but okay; attached.

Although the check for the character's being a tab is still in this one, since it's still needed as part of the ideographic one later.

(file #41437)

David Lawrence Ramsey <dolorous>
Project AdministratorIn charge of this item.
Sun 06 Aug 2017 08:18:02 AM UTC, comment #11:

Please provide the patches in reversed order. An ideographic space is a highly unusual case, while tabs are everywhere. So let's first fix the tabs issue. And better attach the ideographic patch to the relevant bug report.

Benno Schulenberg <bens>
Project Administrator
Sat 05 Aug 2017 11:00:17 PM UTC, comment #10:

Argh. In the latest version, there's an off-by-one error in the check for two-column Unicode, and since it's two separate problems, it should be two separate patches (although they conflict if applied separately).

So, to deal with this, I've attached: first, the fix for two-column Unicode blanks; and, second, on top of that, the fix for tabs being spread across chunks.

Sorry for the mess.

(file #41432, file #41433)

David Lawrence Ramsey <dolorous>
Project AdministratorIn charge of this item.
Sat 05 Aug 2017 10:08:14 PM UTC, comment #9:

...and that version was missing the logic to prevent an infinite loop when the terminal is only one column wide.

The attached version really fixes this.

(file #41431)

David Lawrence Ramsey <dolorous>
Project AdministratorIn charge of this item.
Sat 05 Aug 2017 09:58:46 PM UTC, comment #8:

Sorry, I misread your last post; you wanted the tab check gone, not the lastblank check gone. However, your comment on how tabs were the only multibyte blank character got me thinking.

There is actually at least one more multicolumn space character that the multibyte version of isblank() covers: Unicode 003000, Ideographic Space (a two-column space character), which atblanks mode currently doesn't break before properly if it goes over the screen edge.

The attached new version of the patch includes a fix for this as well.

(file #41430)

David Lawrence Ramsey <dolorous>
Project AdministratorIn charge of this item.
Sat 05 Aug 2017 08:55:18 PM UTC, comment #7:

Okay. Attached a new version that does all of these except the "lastblank_column > goal_column" removal, since that part is actually needed to fix the erroneous behavior noted in comment #4. Specifically:

If the last whitespace on a row is a full tab, and it's followed by text on the same row that doesn't completely fit on the row, atblanks mode is supposed to break the text after the tab and put the text on the next row. Since lastblank_column is the column after the whitespace, and it ends up on text on the same row, it's goal_column or less in this case, so it doesn't need adjustment.

If the last whitespace on a row is a full tab that doesn't completely fit on the row, atblanks mode is supposed to break the text inside the tab and put the remaining part of the tab on the next row, so that the tab width doesn't change. Since lastblank_column is the column after the whitespace, and it ends up after the tab that goes to the next row, it's greater than goal_column in this case, so it does need adjustment to keep it in the same row.

Without the check for lastblank_column's being greater than goal_column, it's adjusted in both cases, and the first case ends up wrong because the line is always broken at the edge of the screen, when it should be broken either on the last whitespace before the edge of the screen, or on the text at the edge of the screen (barring multicolumn characters).

For a real example of why this is needed, try the following with the other attached file (the simplest thing I can come up with without having to open the nano binary, since it varies between our machines):

In an 80x24 terminal, with the patch applied, run:

./nano --ignore --softwrap --atblanks atblankstest.txt

Note that the line is broken after the tab, and "there's^Lgoing" is on the next row: this is correct. Now edit the one part of get_softwrap_breakpoint() to comment out that check, and run the above command again: the line is not broken after the tab, and is instead broken in the middle of the control character at the edge of the screen; this is incorrect.

(file #41424, file #41425)

David Lawrence Ramsey <dolorous>
Project AdministratorIn charge of this item.
Sat 05 Aug 2017 12:57:13 PM UTC, comment #6:

Thanks for the patch. But... all these calls of move_index_past_partial_tab() happen after a call of actual_x(). Better replace those latter five calls with calls to, say, proper_x(), which first does an actual_x() and then, for the non-tiny version and for the softwrap case, checks whether to increment the x.

Also, the check for "leftedge > 0" seems superfluous, as for the leftedge == 0 case the modulo will be zero.

Further, I think that the check for "lastblank_column > goal_column" is enough, since a tab is the only way that the goal column can be overshot, no?

Benno Schulenberg <bens>
Project Administrator
Fri 04 Aug 2017 08:55:53 PM UTC, comment #5:

One last version of the patch is attached.

move_chunk_index_past_tab() is now renamed move_index_past_partial_tab(), which I hope is clearer, and the patch is now an official git patch, made against git 94b484e.

(file #41418)

David Lawrence Ramsey <dolorous>
Project AdministratorIn charge of this item.
Fri 04 Aug 2017 04:32:02 AM UTC, comment #4:

Here's an improved version of the last patch, again.

In the last version, when using atblanks, if a tab was the whitespace at the end of a line (where we should break the line in atblanks mode), the check that was supposed to catch tabs extending past the end of the line was erroneously catching all tabs, so the line wasn't broken properly then. The attached new version fixes this.

(file #41407)

David Lawrence Ramsey <dolorous>
Project AdministratorIn charge of this item.
Thu 03 Aug 2017 04:45:13 PM UTC, comment #3:

Addendum: the display glitch when displaying tabs in whitespace display mode also exists in 2.8.0 (before the variable-width softwrap overhaul) and in 2.7.5 (before the initial softwrap overhaul), so it's more long-standing than I thought. Maybe there should be another flag in display_string() that softwrap mode can use, that indicates "if this line starts with an incomplete tab, don't display the whitespace character for tabs at the start of it"? But that's a separate issue, then.

David Lawrence Ramsey <dolorous>
Project AdministratorIn charge of this item.
Wed 02 Aug 2017 03:37:14 PM UTC, comment #2:

Here's an improved version of the last patch.

First, do_home() only needs leftedge_x adjusted, not leftedge, so the latter now remains unchanged. Second, the index adjustments are now in their own function, since all the formula for it needs is a leftedge and a reference to the index to be adjusted (although I'm still not sure about the name move_chunk_index_past_tab()).

(file #41391)

David Lawrence Ramsey <dolorous>
Project AdministratorIn charge of this item.
Wed 02 Aug 2017 08:21:37 AM UTC, comment #1:

Attached a crude attempt at fixing this, against git ac2c81f. It works as follows:

If a tab spans two chunks, get_softwrap_breakpoint() now breaks the line in the middle of it, at the last column of the screen row. This keeps up the assumption in the rest of the softwrap code that the end of one chunk is the beginning of the next one.

A few functions need to manually adjust their character indexes: do_up(), do_down(), do_pageup(), do_pagedown(), and do_home(). This is because, when the cursor is in the middle of a tab, actual_x() will shift the index back to the beginning of the tab, and if the tab begins on the previous chunk, we want it to be shifted forward to the first character after the tab, so that it stays on the same chunk. There is some minor code duplication when doing this, since I'm not sure how to move it into a function without adding cluttering #ifdefs by your standards.

Also, breaking a tab across chunks like this causes a minor glitch when whitespace display mode is on. The character indicating a tab is displayed twice for such tabs: once at the beginning of the tab on the first chunk, and again at the beginning of the second chunk the tab extends to. I think this can't be fixed without making changes to edit_draw(), but that function is rather complex, and I don't understand it that well.

(file #41390)

David Lawrence Ramsey <dolorous>
Project AdministratorIn charge of this item.
Sun 30 Jul 2017 07:23:37 AM UTC, original submission:

To reproduce, make your terminal sixty columns wide. Then run:

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

and type: aaaabb <Home> M-P. Then type seven times <Tab>. Note how the "bb" now sit at the start of the next row, as they should. Then type another <Tab>. See how the "bb" have visually advanced by twelve columns. This isn't right. A tab isn't a multicolumn character that needs to be pushed to the next row. A tab is a special character that "simulates" the required number of spaces to get to the next multiple of eight (or whatever the tabsize is).

If you have lines that are indented with 64 spaces, and other lines that are indented with 8 tabs, all those lines should visually have the same indentation, also when softwrapped in their leading whitespace.

Now run the same command but add the --atblanks option. Then type the same: aaaabb <Home> M-P, followed by seven tabs. The "aaaabb" now sits at the start of the second row, as it should because it doesn't fully fit on the first row. Then type another <Tab>. The text does not advance -- instead another tab is added to the first row, but it occupies only four spaces, the other four apparently run offscreen. This isn't right. If instead of a tab I would type eight spaces, then the "aaaabb" would advance when typing the last four spaces, because only the first four still fit on the first row.

Benno Schulenberg <bens>
Project Administrator

 

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

Attach File(s):
   
   
Comment:
   

Attached Files
file #41501:  tabtest4.txt added by dolorous (50B - text/plain)
file #41425:  atblankstest.txt added by dolorous (152B - text/plain)
file #41407:  tabfix3.patch added by dolorous (5KiB - text/x-patch)
file #41391:  tabfix2.patch added by dolorous (5KiB - text/x-patch)
file #41390:  tabfix.patch added by dolorous (5KiB - 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)
  • -unavailable- added by bens
  •  

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

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 28 Aug 2017 07:08:58 PM UTCbensOpen/ClosedOpen=>Closed
    Sun 13 Aug 2017 07:09:31 PM UTCbensStatusIn Progress=>Fixed
    Sun 13 Aug 2017 02:51:24 PM UTCbensAttached File-=>Added 0001-softwrap-don-t-push-a-straddling-tab-to-the-next-chu.patch, #41504
    Sun 13 Aug 2017 06:13:49 AM UTCdolorousAttached File-=>Added 0001-softwrap-don-t-break-softwrapped-chunks-before-parti.patch, #41500
      Attached File-=>Added tabtest4.txt, #41501
    Sun 13 Aug 2017 03:29:50 AM UTCdolorousAttached File-=>Added 0001-softwrap-don-t-break-softwrapped-chunks-before-parti.patch, #41498
    Sun 06 Aug 2017 03:34:06 PM UTCdolorousAttached File-=>Added 0001-softwrap-don-t-break-softwrapped-chunks-before-parti.patch, #41437
    Sat 05 Aug 2017 11:00:17 PM UTCdolorousAttached File-=>Added 0001-softwrap-break-before-multi-column-Unicode-blanks-if.patch, #41432
      Attached File-=>Added 0002-softwrap-don-t-break-softwrapped-chunks-before-parti.patch, #41433
    Sat 05 Aug 2017 10:08:14 PM UTCdolorousAttached File-=>Added 0001-softwrap-don-t-break-softwrapped-chunks-before-parti.patch, #41431
    Sat 05 Aug 2017 09:58:46 PM UTCdolorousAttached File-=>Added 0001-softwrap-don-t-break-softwrapped-chunks-before-parti.patch, #41430
    Sat 05 Aug 2017 08:55:18 PM UTCdolorousAttached File-=>Added 0001-softwrap-don-t-break-softwrapped-chunks-before-parti.patch, #41424
      Attached File-=>Added atblankstest.txt, #41425
    Sat 05 Aug 2017 12:57:13 PM UTCbensStatusNone=>In Progress
      Assigned toNone=>dolorous
    Fri 04 Aug 2017 08:55:53 PM UTCdolorousAttached File-=>Added 0001-softwrap-don-t-break-softwrapped-chunks-before-parti.patch, #41418
    Fri 04 Aug 2017 04:32:02 AM UTCdolorousAttached File-=>Added tabfix3.patch, #41407
    Wed 02 Aug 2017 03:37:14 PM UTCdolorousAttached File-=>Added tabfix2.patch, #41391
    Wed 02 Aug 2017 08:21:37 AM UTCdolorousAttached File-=>Added tabfix.patch, #41390
    Sun 30 Jul 2017 07:23:37 AM UTCbensCarbon-Copy-=>Added dolorous

    Back to the top


    Powered by Savane 3.1-cleanup1