#2456 [Patch] Fix wrapping removed lines

[pawelzwronek]

Hello. This is my first ticket here, and actually my first on SF.
I would like to propose a fix for an issue 15817 reported in Notepad++ repo.

To reproduce the issue, you need to drag and drop to the top (somewhere before the selection), at least two selected lines, where the last of the lines is wrapped (have sublines). After doing so, a pink rectangle appears in the place of the removed last line.

Source of the bug is an incorrect noumber of lines passed to NeedWrapping() in CheckModificationForWrap(). As a result, the last removed line is not updated with pcs->SetHeight(lineNumber, linesWrapped) in WrapBlock(). When many lines is removed on drag&drop mh.linesAdded is negative and NeedWrapping() gets [lineDoc, lineDoc + 1], but that's not enough to rewrap removed lines when removing more than one line, and pcs thinks the last removed line is still multiline/wrapped.

[Neil Hodgson]

While this appears to be the right code to change, I don't think the analysis is correct.

When many lines is removed on drag&drop mh.linesAdded is negative ... that's not enough to rewrap removed lines

The removed lines have disappeared so don't need to be wrapped, only the line where the deletion occurred. That line (line 1 in the example) is already being scheduled for wrapping.

The following insertion on line 0 adds a line so both line 0 and the new line 1 have to be wrapped and are scheduled to do so.

However, because of the added line, the line of the deletion was moved on from line 1 to line 2 and there was never a request to wrap line 2.

This pattern of not taking account of the movement of earlier requests for wrapping may recur in more complex scenarios where multiple actions are being performed in an undo or redo group before the wrapping is actually done.

It may be possible to track the region needing to wrap more closely. Perhaps something equivalent to

if (mh.linesAdded > 0)
    wrapPending.end += mh.linesAdded;

There may be further issues making it too difficult to cover all cases. Then the wrap pending range could be extended to the rest of the document WrapPending::lineLarge whenever it may be needed.

[pawelzwronek]

Another solution, which doesn't involve expanding wrapPending, and is working is to set the height of the last removed line early on in CheckModificationForWrap with:

if (mh.linesAdded < 0) {
    pcs->SetHeight(lineDoc, 1);
}

When a text is removed wrapPending is expanded to cover removed line. Do we need to rewrap all unchanged lines in expanded wrapPendingregion or just actually affected lines. If so we could change wrapPendingto a list of regions, with auto-merging when a new region is added. But then, distribution of regions to wrap in multithreaded wrapping would be more complex, but not impossible.

[Neil Hodgson]

lineDoc is the line before any removed lines. Some of its text may have been removed but it may still need wrapping and it may even be longer than before if the line at the end of the deletion wasn't empty.

The next line lineDoc+1 may also need rewrapping although I think it can only become shorter.

A list of wrap regions is more work and adds more to go wrong.

[pawelzwronek]

When I debug CheckModificationForWrap while drag&drop the selection to the fist line

I get this:

It seems lineDoc is an empty line after removing selected lines.

[Neil Hodgson]

You are choosing whole lines but other cases may choose partial lines.

[pawelzwronek]

Oh, I see :) I'll look into this tomorrow. Time for some BG3.

[pawelzwronek]

So I think we need to move whole wrap region according to the modification like so:

@@ -2633,6 +2633,16 @@ void Editor::CheckModificationForWrap(DocModification mh) {
                view.llc.Invalidate(LineLayout::ValidLevel::checkTextAndStyle);
                const Sci::Line lineDoc = pdoc->SciLineFromPosition(mh.position);
                const Sci::Line lines = std::max(static_cast<Sci::Line>(0), mh.linesAdded);
+

+               // Check if this modification crosses any of the wrap points
+               if (wrapPending.NeedsWrap()) {
+                       if (lineDoc < wrapPending.start) { // Inserted/deleted before wrap range
+                               wrapPending.start += mh.linesAdded;
+                               wrapPending.end += mh.linesAdded;
+                       } else if (lineDoc < wrapPending.end) { // Inserted/deleted inside wrap range
+                               wrapPending.end += mh.linesAdded;
+                       }
+               }
                if (Wrapping()) {
                        NeedWrapping(lineDoc, lineDoc + lines + 1);
                }

[Neil Hodgson]

None of this is needed when wrap is turned off so the added code should be inside the if (Wrapping()).

For wrapPending.start, try this example:

• an initial single character insertion on line 11 sets wrapPending.start=11.
• a 5 line deletion is then performed starting on line 10 so mh.linesAdded=-5
• the deletion passes the lineDoc < wrapPending.start test and subtracts 5 from wrapPending.start so wrapPending.start=6 and its rewrapping 4 lines without need.

The wrap range should never be moved before the new change and NeedWrapping is about to set the start to lineDoc if that is before the range so this assignment to
wrapPending.start is unnecessary and incorrect so should be removed.

That simplifies the patch to just updating wrapPending.end. If NeedsWrap() then wrapPending.start < wrapPending.end and the code can be simplified into a single if (lineDoc < wrapPending.end) block.

Then the question is: should a negative linesAdded always reduce the range or may there be more cases (like the overlapping one above) to consider? It is obviously safe (but potentially sub-optimal) to only ever extend the range thus the if (mh.linesAdded > 0) in my earlier change proposal.

[pawelzwronek]

You are right. There is no need to move start as it may expand to no needing wrap lines. Here is corrected file:

@@ -2634,6 +2634,11 @@ void Editor::CheckModificationForWrap(DocModification mh) {
        const Sci::Line lineDoc = pdoc->SciLineFromPosition(mh.position);
        const Sci::Line lines = std::max(static_cast<Sci::Line>(0), mh.linesAdded);
        if (Wrapping()) {

+           if (wrapPending.NeedsWrap()) {
+               if (lineDoc < wrapPending.end) { // Check if this modification starts before or inside wrapPending region
+                   wrapPending.end += mh.linesAdded; // Is so move only the end position, if will be moved before lineDoc it will be fixed in NeedWrapping
+               }
+           }
            NeedWrapping(lineDoc, lineDoc + lines + 1);
        }
        RefreshStyleData();

Unfortunately I'm not as much familiar in all other side effects of this change as you, and I'm unable to help, but I have tested this change in N++ while drag&dropping bunch of lines in different scenarios and haven't noticed any issues.

Found Lexilla
simpleTests
.....................................................................................................................................................................................................................................................
----------------------------------------------------------------------
Ran 245 tests in 0.269s

OK

===============================================================================
All tests passed (3,587 assertions in 47 test cases)

[Neil Hodgson]

Committed as [2a428a].

[pawelzwronek]

Thanks :)