#2340 `Editor::FoldAll()` does not fold inner levels

[Zufu Liu]

it seems it's intended to only fold top level? however expand all works as expected.

if (LevelIsHeader(level) &&
        (FoldLevel::Base == LevelNumberPart(level))) {

[Neil Hodgson]

Acting as intended.

[Zufu Liu]

Simple change to make Editor::FoldAll() working as expected, very faster for both contracting and expanding.

if (expanding) {
    pcs->SetVisible(0, maxLine-1, true);
    for (Sci::Line line = 0; line < maxLine; line++) {
        const FoldLevel level = pdoc->GetFoldLevel(line);
        if (LevelIsHeader(level)) {
            pcs->SetExpanded(line, true);
        }
    }
} else {
    for (Sci::Line line = 0; line < maxLine; line++) {
        const FoldLevel level = pdoc->GetFoldLevel(line);
        if (LevelIsHeader(level)) {
            pcs->SetExpanded(line, false);
            if (FoldLevel::Base == LevelNumberPart(level)) {
                const Sci::Line lineMaxSubord = pdoc->GetLastChild(line);
                if (lineMaxSubord > line) {
                    pcs->SetVisible(line + 1, lineMaxSubord, false);
                }
            }
        }
    }
}

SetScrollBars();
RedrawSelMargin();
Redraw();

merged as following:

if (expanding) {
    pcs->SetVisible(0, maxLine - 1, true);
}
for (Sci::Line line = 0; line < maxLine; line++) {
    const FoldLevel level = pdoc->GetFoldLevel(line);
    if (LevelIsHeader(level)) {
        pcs->SetExpanded(line, expanding);
        if (!expanding && FoldLevel::Base == LevelNumberPart(level)) {
            const Sci::Line lineMaxSubord = pdoc->GetLastChild(line);
            if (lineMaxSubord > line) {
                pcs->SetVisible(line + 1, lineMaxSubord, false);
            }
        }
    }
}

[Neil Hodgson]

That is reverting the change made in [4f6cd7] . The document's fold levels could potentially not match the contraction state if the application makes a mistake in responding to fold level changes. Using contraction state guarantees that there will be no contracted lines and acts as a reset to safety. If performance was important then contraction state could offer a set-all-expanded (or ...range...) since it has access to the underlying RunStyles and could just reset it.

[Zufu Liu]

OK, added ExpandAll().

template <typename LINE>
void ContractionState<LINE>::ExpandAll() {
    if (OneToOne()) {
        return;
    } else {
        const LINE lines = expanded->Length();
        expanded->FillRange(0, 1, lines);
        Check();
    }
}

ContractionState<LINE>::SetVisible() is also changed to use FillRange(), which makes code folding even faster.

if (changed) {
    visible->FillRange(static_cast<LINE>(lineDocStart), isVisible ? 1 : 0,
        static_cast<LINE>(lineDocEnd - lineDocStart) + 1);
}

[Neil Hodgson]

Why is the changed variable needed in ContractionState<LINE>::SetVisible? Isn't that equivalent to delta != 0?

[Zufu Liu]

I think delta could be deleted, and change the code to returns changed, means visibility of some line are changed, technically it's different from delta (changed screen line count).

[Neil Hodgson]

This may have returned the delta in an early version. Its fine to just track if there have been any changes.

[Zufu Liu]

The change to SetVisible() can be used to optimize Editor::ExpandLine():
Maybe it's better to open a new ticket to discuss improvements for code folding.

Sci::Line Editor::ExpandLine(Sci::Line line, FoldLevel level) {
    const Sci::Line lineMaxSubord = pdoc->GetLastChild(line, level);
    line++;
    Sci::Line lineStart = line;
    while (line <= lineMaxSubord) {
        level = pdoc->GetFoldLevel(line);
        if (LevelIsHeader(level)) {
            pcs->SetVisible(lineStart, line, true);
            if (pcs->GetExpanded(line)) {
                line = ExpandLine(line, level);
            } else {
                line = pdoc->GetLastChild(line, level);
            }
            lineStart = line + 1;
        }
        line++;
    }
    if (lineStart <= lineMaxSubord) {
        pcs->SetVisible(lineStart, lineMaxSubord, true);
    }
    return lineMaxSubord;
}

[Zufu Liu]

final version of Editor::ExpandLine():

Sci::Line Editor::ExpandLine(Sci::Line line, FoldLevel level, Sci::Line *parentLine) {
    const Sci::Line lineMaxSubord = pdoc->GetLastChild(line, level);
    line++;
    Sci::Line lineStart = parentLine ? *parentLine : line;
    while (line <= lineMaxSubord) {
        level = pdoc->GetFoldLevel(line);
        if (LevelIsHeader(level)) {
            if (pcs->GetExpanded(line)) {
                line = ExpandLine(line, level, &lineStart);
            } else {
                pcs->SetVisible(lineStart, line, true);
                line = pdoc->GetLastChild(line, level);
                lineStart = line + 1;
            }
        }
        line++;
    }
    if (parentLine) {
        *parentLine = lineStart;
    } else if (lineStart <= lineMaxSubord) {
        pcs->SetVisible(lineStart, lineMaxSubord, true);
    }
    return lineMaxSubord;
}

[Neil Hodgson]

The added complexity doesn't seem worth it.

[Zufu Liu]

What about previous version (the one without parentLine) ?

[Neil Hodgson]

That is better than the one after. Recursive functions start as more complex and adding an optional pointer argument makes it worse. The question for the previous version is whether there is sufficient benefit to batch up SetVisible calls. The data structure manipulation will often take little time compared to the GUI-level costs of performing redraws and similar so I want to see that the added code will produce a user-noticeable improvement. Its always worthwhile measuring before / after performance - sometimes the results don't match expectations.

There seems to be difference in attitudes between us here. You want even small performance improvements, perhaps because they have potential benefits. I'm just a grumbly grey-beard who has seen too many projects exceed their complexity budget and become unmaintainable.

Low level optimizations can make it more difficult to discover and apply higher-level optimizations. The fold structure is an implicit tree that gets partially rediscovered then discarded on each action. Perhaps the tree should be made concrete and persistent albeit incomplete.

[Neil Hodgson]

Since SetExpanded returns a bool, ExpandAll should as well.

There should be a test for ExpandAll:

    SECTION("ExpandAll") {
        pcs->InsertLines(0,4);
        for (int l=0;l<4;l++) {
            REQUIRE(true == pcs->GetExpanded(l));
        }

        pcs->SetExpanded(2, false);
        REQUIRE(true == pcs->GetExpanded(1));
        REQUIRE(false == pcs->GetExpanded(2));
        REQUIRE(true == pcs->GetExpanded(3));

        pcs->SetExpanded(1, false);
        REQUIRE(false == pcs->GetExpanded(1));
        REQUIRE(false == pcs->GetExpanded(2));
        REQUIRE(true == pcs->GetExpanded(3));

        pcs->ExpandAll();
        REQUIRE(true == pcs->GetExpanded(1));
        REQUIRE(true == pcs->GetExpanded(2));
        REQUIRE(true == pcs->GetExpanded(3));
    }

While the current behaviour of FoldAll(FoldAction::Contract) is good, since applications want to fold every header, there could be a FoldAction::ContractRecursive to do that.

[Zufu Liu]

for FoldAll, what about adding second parameter to tell whether to contract all headers (default is false to keep current behavior), so it works also for FoldAction::Toggle.

fun void FoldAll=2662(FoldAction action, bool allLevel)

[Neil Hodgson]

Extending this to Toggle makes it more complex to evolve. Will there be 4 toggle command variants if an ExpandTopLevel was added?

[Zufu Liu]

First part of the changes (ExpandAll, ExpandLine and SetVisible).

Not intended to add ExpandTopLevel, implement this is complex, rough equivalent to call FoldLine() on all top level header.
I just want contract recursive works for FoldAction::Contract and FoldAction::Toggle, how about change the second parameter to bool contractRecursive or bool contractAll? it's easy to handle than treat FoldAction::ContractRecursive as a flag.

[Zufu Liu]

Minor change for SetVisible, changed static_cast<char>(isVisible) in above patch to (isVisible ? 1 : 0).

The condition GetVisible(line) != isVisible can be simplified to call visible->ValueAt(static_cast<LINE>(line)) directly (not in the patch).

[Neil Hodgson]

Committed as [fe4bf2].

I think expanding the scope of Toggle may be counterproductive causing more complexity in the future. The criteria it uses to determine the current state (is the first fold header expanded?) isn't great and applications may want to use something else. Majority expanded, for example. It may be better to have apps determine which state they think the folds are in (possibly with helpers in Scintilla) then call the action they want.

it's easy to handle than treat FoldAction::ContractRecursive as a flag.

Bundling up the options, either inside an enum or as a struct, makes it easier to pass them around. Treating enums as flags isn't that bad. bool parameters have their own problems.

[Zufu Liu]

Implement FoldAction::ContractRecursive as a flag.
SciTEBase::FoldAll() can be changed to

wEditor.SetRedraw(false);
wEditor.FoldAll(SA::FoldAction::Toggle);
wEditor.SetRedraw(true);

[Zufu Liu]

FoldAction::ContractAll seems better than FoldAction::ContractRecursive as there is no recursive.

for contacting, FoldLevel::Base == LevelNumberPart(level) will fails for top level fold without parent in Python like indentation based folding, e.g.

# initial indentation before def
    def test():
        pass

for toggling, Discover current state may fails for fold without child.

[Neil Hodgson]

ContractAll sounds redundant to a FoldAll function. Perhaps ContractEach or ContractEveryHeader.

Indenting the top level is unlikely for Python or similar languages.

>pyw -u "Indented.py"
  File "C:\u\examples\Indented.py", line 2
    def test():
IndentationError: unexpected indent

may fails for fold without child

Headers should have children.

[Neil Hodgson]

The actual difference is that Contract... contracts every level.

[Zufu Liu]

Though it's invalid Python, however I thing toggle folds should still works.
A simple fix:

FoldLevel topLevel = FoldLevel::NumberMask;
for (; line < maxLine; line++) {
    if (LevelIsHeader(level)) {
        const FoldLevel levelLine = LevelNumberPart(level);
        if (topLevel >= levelLine) {
            topLevel = levelLine; // top level fold without parent
            const Sci::Line lineMaxSubord = pdoc->GetLastChild(line, level);