Skip to content

Add option to show last line with syntax-aware code folding #1546

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

Add option to show last line with syntax-aware code folding #1546

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a3e21d9
Initial attempt
Sep 19, 2018
fe90445
Merge pull request #1 from PowerShell/master
RussPitcher Sep 26, 2018
52e38f8
Add missing semicolon
Sep 26, 2018
39988ed
Merge branch 'feature-code-folding-show-last-line'
Sep 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
# vscode-powershell Release History

- [vscode-PowerShell #????](https://github.com/PowerShell/vscode-PowerShell/pull/????) -
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will cause merge conflicts later on if we leave it in. If you put your summary from here into a commit message, it will make its way into the changelog.

Just stick the summary into a commit taking out the changelog entry is probably the easiest way to go. Otherwise there is also the git commit --amend -> git push origin +branch option.

New code folding option - 'powershell.codeFolding.showLastLine' - Set true to show the last line of a folded section similar to the default VSCode folding style

## v1.8.4
### Friday, August 31, 2018
#### [vscode-powershell](https://github.com/powershell/vscode-powershell)
Expand Down
5 changes: 5 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,6 +478,11 @@
"default": true,
"description": "Enables syntax based code folding. When disabled, the default indentation based code folding is used."
},
"powershell.codeFolding.showLastLine": {
"type": "boolean",
"default": false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tylerl0706 @rkeithhill @glennsarti @RussPitcher Thoughts on this being the default? (Note as in the description below that this is VSCode's default)

"description": "Shows the last line of a folded section similar to the default VSCode folding style. When disabled, the entire folded region is hidden."
},
"powershell.codeFormatting.preset": {
"type": "string",
"enum": [
Expand Down
6 changes: 6 additions & 0 deletions src/features/Folding.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,10 +101,13 @@ class LineNumberRange {
*/
public rangeKind: vscode.FoldingRangeKind;

private settings: Settings.ISettings;

constructor(
rangeKind: vscode.FoldingRangeKind,
) {
this.rangeKind = rangeKind;
this.settings = Settings.load();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to ensure that settings are only loaded once? Like passing them into the constructor? Or does Settings.load() guarantee that (I assume it does actually, but want to check)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. Settings should go into the constructor signature.

Settings.load() has no side effects (that I know of) but in theory there could be some race conditions?

}

/**
Expand All @@ -121,6 +124,9 @@ class LineNumberRange {
): LineNumberRange {
this.startline = document.positionAt(start.startIndex).line;
this.endline = document.positionAt(end.startIndex).line;
if (this.settings.codeFolding && this.settings.codeFolding.showLastLine) {
this.endline--;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume endline-1 is never going to be a bad value for VSCode. We might want to test some edge cases here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a guard here to make sure that endline is never less than startline

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acutally, in retrospect. This is wrong place to do this. Because, changing the endline too early means the isValidRange function will give out false positives (

vscode-powershell/src/features/Folding.ts

Line 153 in 7afec44

return (this.endline - this.startline >= 1);
)

Instead it should replace

vscode-powershell/src/features/Folding.ts

Lines 160 to 162 in 7afec44

public toFoldingRange(): vscode.FoldingRange {
return new vscode.FoldingRange(this.startline, this.endline, this.rangeKind);
} 

    public toFoldingRange(settings:ISettings): vscode.FoldingRange {
        if (settings.codeFolding && settings.codeFolding.showLastLine) {
            return new vscode.FoldingRange(this.startline, this.endline - 1, this.rangeKind);
        } else {
            return new vscode.FoldingRange(this.startline, this.endline, this.rangeKind);
        }
    }

And then change the provideFoldingRanges method to;

vscode-powershell/src/features/Folding.ts

Lines 255 to 261 in 7afec44

return foldableRegions
// It's possible to have duplicate or overlapping ranges, that is, regions which have the same starting
// line number as the previous region. Therefore only emit ranges which have a different starting line
// than the previous range.
.filter((item, index, src) => index === 0 || item.startline !== src[index - 1].startline)
// Convert the internal representation into the VSCode expected type
.map((item) => item.toFoldingRange()); 

        const settings = this.currentSettings();
        return foldableRegions
            // It's possible to have duplicate or overlapping ranges, that is, regions which have the same starting
            // line number as the previous region. Therefore only emit ranges which have a different starting line
            // than the previous range.
            .filter((item, index, src) => index === 0 || item.startline !== src[index - 1].startline)
            // Convert the internal representation into the VSCode expected type
            .map((item) => item.toFoldingRange(settings));
    }

    private currentSettings(): ISettings { Settings.load(); }

The private method currentSettings() is needed so we can mock it in the unit tests to flick the behaviour.

}
return this;
}

Expand Down
2 changes: 2 additions & 0 deletions src/settings.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export interface IBugReportingSettings {

export interface ICodeFoldingSettings {
enable?: boolean;
showLastLine?: boolean;
}

export interface ICodeFormattingSettings {
Expand Down Expand Up @@ -116,6 +117,7 @@ export function load(): ISettings {

const defaultCodeFoldingSettings: ICodeFoldingSettings = {
enable: true,
showLastLine: false,
};

const defaultCodeFormattingSettings: ICodeFormattingSettings = {
Expand Down