#1896 Unfolding + Missing lines

[mLipok]

Step 1 put this in test.lua

a=1
if  1 then
    a = 1
end

Step 2: Fold the if on line 3

Step 3: add new line and type the word "If" on line 1 and you will see line 4 again but line 5 missing.

I tested it on the original version wscite (http://www.scintilla.org/SciTEDownload.html)

This issue seams to be not related to lua lexer, but to internal scintilla code.

[Greg Smith]

I can also reproduce this with my own custom lexer (which also folds on 'if'), This bug has been present for quite a while... It was OK in 3.5.7, not OK in 3.6.4. I do not have any product releases with any in-btetween versions to narrow this down any further.

If it helps, my lexer will increment the nesting level when it detects the f of 'if' which is when the a = 1 line pops up again and the fold marker on the 'if 1 then' line changes to unfolded.. If I then delete the f of 'if', the 'end' line reappears.

[Neil Hodgson]

It appears to be the fix for [#1799] which was [def6d8] for Scintilla and [21ad2b] for SciTE. In particular, the last chunk which tries to mend merged regions but is operating on unfinished changes: the second last line is being processed but the last line still has the old level which makes it a peer of its previous parent. The last line will next have its level incremented but that is too late.

[Greg Smith]

I did a little investigating with my own lexer. AFAICT, the lexer is setting the folding information correctly, and the information appears to be correct before the problem and after... I generated a simple test case that fails with the C/C++ lexer in Scite and for which the folding levels as set and seen by Accessor::SetLevel() and Accessor::LevelAt() do not change.

-{
-{
}

Fold the second pair of braces.

-{
+{

---

Add a space after the first brace. All is well

Now delete the space and the folded braces unfold. and you are back to the initial state.

[Neil Hodgson]

The state before and after are good but the changes to each line are made separately from top to bottom and the fold maintenance code is run on each line's change.

I can't (yet) see a way to avoid the unfolding that was added for [#1799] but it is possible to ensure that the last line is shown with these changes to SciTE (tested) or Scintilla (not tested):

diff -r 1de0ed852ea6 src/SciTEBase.cxx
--- a/src/SciTEBase.cxx Wed Jan 04 10:32:15 2017 +1100
+++ b/src/SciTEBase.cxx Sat Jan 07 22:11:40 2017 +1100
@@ -3690,10 +3690,16 @@
    if (!(levelNow & SC_FOLDLEVELWHITEFLAG) && (LevelNumber(levelPrev) < LevelNumber(levelNow))) {
        if (!wEditor.Call(SCI_GETALLLINESVISIBLE)) {
            const int parentLine = wEditor.Call(SCI_GETFOLDPARENT, line);
-           if (!wEditor.Call(SCI_GETFOLDEXPANDED, parentLine) && wEditor.Call(SCI_GETFOLDEXPANDED, line)) {
-               wEditor.Call(SCI_SETFOLDEXPANDED, parentLine, 1);
-               const int levelParentLine = wEditor.Call(SCI_GETFOLDLEVEL, parentLine);
-               ExpandFolds(parentLine, true, levelParentLine);
+           if (wEditor.Call(SCI_GETFOLDEXPANDED, parentLine)) {
+               if (!wEditor.Call(SCI_GETLINEVISIBLE, line)) {
+                   wEditor.Call(SCI_SHOWLINES, line, line);
+               }
+           } else {
+               if (wEditor.Call(SCI_GETFOLDEXPANDED, line)) {
+                   wEditor.Call(SCI_SETFOLDEXPANDED, parentLine, 1);
+                   const int levelParentLine = wEditor.Call(SCI_GETFOLDLEVEL, parentLine);
+                   ExpandFolds(parentLine, true, levelParentLine);
+               }
            }
        }
    }

diff -r 5e819dc26052 src/Editor.cxx
--- a/src/Editor.cxx    Wed Jan 04 13:32:23 2017 +1100
+++ b/src/Editor.cxx    Sat Jan 07 22:11:26 2017 +1100
@@ -5500,8 +5500,16 @@
    if (!(levelNow & SC_FOLDLEVELWHITEFLAG) && (LevelNumber(levelPrev) < LevelNumber(levelNow))) {
        if (cs.HiddenLines()) {
            const int parentLine = pdoc->GetFoldParent(line);
-           if (!cs.GetExpanded(parentLine) && cs.GetExpanded(line))
-               FoldLine(parentLine, SC_FOLDACTION_EXPAND);
+           if (cs.GetExpanded(parentLine)) {
+               if (!(cs.GetVisible(line))) {
+                   cs.SetVisible(line, line, true);
+                   SetScrollBars();
+                   Redraw();
+               }
+           } else {
+               if (cs.GetExpanded(line))
+                   FoldLine(parentLine, SC_FOLDACTION_EXPAND);
+           }
        }
    }
 }

[Neil Hodgson]

A simpler fix appears to be to check if the line is visible instead of whether it is expanded. This avoids showing line 4 in the bug or marking line 3 as expanded. This means the fold structure is more static when earlier text is changed which seems better to me.

The patches:

diff -r 5e819dc26052 src/Editor.cxx
--- a/src/Editor.cxx    Wed Jan 04 13:32:23 2017 +1100
+++ b/src/Editor.cxx    Mon Jan 09 20:38:29 2017 +1100
@@ -5500,7 +5500,7 @@
    if (!(levelNow & SC_FOLDLEVELWHITEFLAG) && (LevelNumber(levelPrev) < LevelNumber(levelNow))) {
        if (cs.HiddenLines()) {
            const int parentLine = pdoc->GetFoldParent(line);
-           if (!cs.GetExpanded(parentLine) && cs.GetExpanded(line))
+           if (!cs.GetExpanded(parentLine) && cs.GetVisible(line))
                FoldLine(parentLine, SC_FOLDACTION_EXPAND);
        }
    }
diff -r 1de0ed852ea6 src/SciTEBase.cxx
--- a/src/SciTEBase.cxx Wed Jan 04 10:32:15 2017 +1100
+++ b/src/SciTEBase.cxx Mon Jan 09 20:38:35 2017 +1100
@@ -3690,7 +3690,7 @@
    if (!(levelNow & SC_FOLDLEVELWHITEFLAG) && (LevelNumber(levelPrev) < LevelNumber(levelNow))) {
        if (!wEditor.Call(SCI_GETALLLINESVISIBLE)) {
            const int parentLine = wEditor.Call(SCI_GETFOLDPARENT, line);
-           if (!wEditor.Call(SCI_GETFOLDEXPANDED, parentLine) && wEditor.Call(SCI_GETFOLDEXPANDED, line)) {
+           if (!wEditor.Call(SCI_GETFOLDEXPANDED, parentLine) && wEditor.Call(SCI_GETLINEVISIBLE, line)) {
                wEditor.Call(SCI_SETFOLDEXPANDED, parentLine, 1);
                const int levelParentLine = wEditor.Call(SCI_GETFOLDLEVEL, parentLine);
                ExpandFolds(parentLine, true, levelParentLine);

It is unlikely that this fixes all the potential problems with this code section but it does fix this particular bug as far as I can see.

[Greg Smith]

The simpler fix stops the partial reveal of the text (which is plainly wrong), but it does not stop the fold unfolding when you delete text above it that makes no difference to the fold structure. This is equivalent to the C example I posted. In my lexer if I have:

if 1 then
if 1 then
junk();
endif
endif

I then fold up the middle if..endif. I can now insert spaces in the first line and nothing happens (at the end, or between the first if and 1 or the 1 and then). However, if I delete any of the spaces with a backwards delete, or by selection and delete, although this makes no difference to the fold levels, the inner fold expands. If I insert a blank line (or any non-fold header line between the first if and the second), I can mess with this line and the folds do not expand.

So the question is, what is so special about deleting a character in a fold header line that triggers this rather than adding characters, which does not?

[Greg Smith]

As a wild stab in the dark... is #1799 attempting to merge folds when the EOL is deleted, but for some reason is doing this when ANY character is deleted (possibly because detecting that EOL has gone is tricky).

This suggests that the expand code is being triggered in the wrong place... I would imagine that there is a well-defined routine that is called when lines are merged (as there is a lot of stuff to do... markers and so on), and this is where the expansion should be triggered from if the two lines are both fold headers, not on any delete in a fold header line on the off-chance that this might cause merging...

Would it be better to remove the #1799 patch until this is resolved?

[Neil Hodgson]

The double if unfold was caused by change set [acc4c2] for bug [#1796], not [#1799].

[Neil Hodgson]

Its likely that restricting [acc4c2] to cases where the deletion includes a line end would be reasonable.

[Greg Smith]

Changing the SC_MOD_BEFOREDELETE code to:

            } else if (mh.modificationType & SC_MOD_BEFOREDELETE) {
                // If the deletion includes any EOL then we extend the need show area.
                int lineLast = pdoc->LineFromPosition(mh.position+mh.length);
                if (lineOfPos != lineLast) {
                    endNeedShown = mh.position + mh.length;
                    for (int line = lineOfPos; line <= lineLast; line++) {
                        const int lineMaxSubord = pdoc->GetLastChild(line, -1, -1);
                        if (lineLast < lineMaxSubord) {
                            lineLast = lineMaxSubord;
                            endNeedShown = pdoc->LineEnd(lineLast);
                        }
                    }
                }
            }
            NeedShown(mh.position, endNeedShown - mh.position);

Prevents the expansion unless a line end is removed, which feels somewhat better.

However, it still unfolds when you have (for example) a blank line after an if, followed by a folded if and you delete the blank line. I can see that the original patch was designed for the case where you can merge adjacent folds (for example for multiple whole line comments). But this does not apply to the vast majority of folds.

I think that this area needs some more thought. Perhaps we need to assign some more fold flags so that we can flag mergeable folds to cope with single line comments (what else could be merged?).

Alternatively, if the lexer wants this type of behavior, make it the responsibility of the lexer to provide it.

[Neil Hodgson]

The "if (lineOfPos != lineLast)" looks OK. Its likely that the loop was supposed to work this way but it always does at least one iteration.

At one point in working on this issue, I was trying to expand the rules in FoldChanged so that it would handle more cases in a simple way. Here are the notes:

This method may make a line or lines visible - it will never hide lines.
It may also expand a header line which has the effect of making lines visible
and changing the expanded state to true (never false).

Potential rules:
If a line is visible and its header is not expanded then its header is expanded (recursively).
If a line is hidden and its header is expanded and visible then it is shown.

[Greg Smith]

On a delete, I would have thought that the cases that require an unfold are:
1. where a delete operation changes a fold header line to a non-fold header line, for example by deleting the f of if (in the original case).
2. where a delete removes the EOL immediately before a fold header (effectively removing the fold- header line).

If a delete removes an entire fold-header line, either its children are folded, in which case they will also be deleted, or they are not, in which case there is nothing to do (in this context).

In case 1, the lexer will have to change the line into a non-fold header, which should be sufficient signal to Scintilla to unfold children and can be done when the delete happens..

In case 2, which is the one we are dealing with (I think), you need to detect this in advance of the delete (which is what the current code is trying to do). The current code (with the patch) is making sure that all the deleted text is visible, but it goes too far. What I think it should do is make sure that all the text up to and including lineLast is visible, then only if lineLast is a fold header should it look at children of lineLast.

This can be done with the following somewhat simpler code:

            } else if (mh.modificationType & SC_MOD_BEFOREDELETE) {
                // If the deletion includes any EOL then we extend the need show area.
                int lineLast = pdoc->LineFromPosition(mh.position+mh.length);
                endNeedShown = mh.position + mh.length;
                for (int line = lineOfPos+1; line <= lineLast; line++) {
                    const int lineMaxSubord = pdoc->GetLastChild(line, -1, -1);
                    if (lineLast < lineMaxSubord) {
                        lineLast = lineMaxSubord;
                        endNeedShown = pdoc->LineEnd(lineLast);
                    }
                }
            }
            NeedShown(mh.position, endNeedShown - mh.position);

This seems to work (for me), so I suggest this as a patch to fix the problem. I have not looked at the text insertion case.

[Neil Hodgson]

Haven't thought this through but I see a couple of problems here.

Deleting a fold header line does not imply deleting all its child lines. For example, in a 4 line example "{\na\nb\n}", select the first 2 lines, contract the fold, press delete and "b" still exists. While this could be forced at the user manipulation level, I think it is too restrictive to require deletion of the whole fold structure in all cases.

Another case to consider is where a line is responsible for 2 levels of header "{{" and one is deleted as this doesn't stop it being a fold header but limits the range of child lines, so the lines that were children but no longer are should be shown.

[Greg Smith]

I think my comment was wrong (deleting a header implies deleteing the folded lines) as you could use the API to delete any range you like. However, I believe that the patch is OK.

The patched code makes the text marked for delete visible, then it makes any subsequent hidden folds within the text to be deleted visible.

I can not devise a situaltion in my lexer where it does not work OK. I do not know the innards of Scintilla that well, but I expect that the act of deleting a line with a fold header must already ensure that folder lines are made visible.

Even if this is not the perfect solution, it is much better than what was there in the last release and my users all seem happy with it.

[Neil Hodgson]

There is code in Editor::FoldChanged and SciTEBase::FoldChanged that backs up the code in Editor::NotifyModified so that more cases will be unfolded when needed. Before Editor::FoldChanged was implemented, applications often copied SciTE's unfolding logic and may not have been updated. So, Editor::NotifyModified should try to cover possible cases well to support these applications.

Unfolding synchronously before the deletion occurs may be more accurate as it may be looking at more consistent data. Code that operates as each line changes fold state may be trying to work with incomplete changes. This is another motivation for implementing more in NotifyModified.

While I haven't been able to find a bad case, it appears to me that starting the loop at lineOfPos+1 instead of lineOfPos means it won't cover all the children of the start line, so I prefer testing for a line end and then starting from lineOfPos.

[Greg Smith]

it appears to me that starting the loop at lineOfPos+1 instead of lineOfPos means it won't cover all the children of the start line, so I prefer testing for a line end and then starting from lineOfPos

By this, I presume you mean using my first suggested patch. However, this give bad behavior for me as it does unneccessary unfolding when you delete line ends that should not cause subsequent unfolding. I am pretty sure that the patch starting at +1 is OK.

If something in the delete changes the fold status of the first line, this is taken care of in the lexing that happens when the delete happens (when the lexer tell scintilla it has changed the header). If the delete does not change the fold status of the first line, there is no need to extend the "make visible" to the end of the fold of the first line.

I guess I will have to see what you decide to commit and then test it.

[Neil Hodgson]

Committed fix for missing line as [0a94f8] for Scintilla and [30f4e6] for SciTE.

Committed fix for unnecessary unfolding reported by Greg on 2017-01-10 as [fba1e8]. Differs from Greg's #2 patch by switching lines 3 and 4 as this was the original order and minimizes the patch.

[mLipok]

With relation to: https://groups.google.com/forum/#!topic/scite-interest/sRq7MNab9q4
It seems that this version works OK.
Thanks.