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

RussPitcher · 2018-09-26T10:25:53Z

PR Summary

Add a settings option to enable the last line of a syntax-aware folded section to be displayed when the section is folded. Relates to issue #1514

I have added a speculative note in the changelog.md file as I wasn't sure if I should update it or not.

PR Checklist

Note: Tick the boxes below that apply to this pull request by putting an x between the square brackets.
Please mark anything not applicable to this PR NA.

PR has a meaningful title
Summarized changes
This PR is ready to merge and is not work in progress
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready

Update from head fork

msftclas · 2018-09-26T10:26:04Z

All CLA requirements met.

rjmholt · 2018-09-27T17:18:31Z

Tagging @glennsarti for review (won't let me add you in the reviewers dropdown...)

rjmholt · 2018-09-27T17:20:36Z

@RussPitcher No need to add to the changelog -- even though I would like it to be done in each PR, it would create merge conflicts for every PR. So we generate it at release time. Just make sure you include a pithy description in your commit messages (which we squash, but will do our best to preserve information from).

rjmholt

I've cloned and tested this branch (it might be easier in future to choose a non-master branch, since the mechanism for checking out a GitHub PR branch only works if that branch's name doesn't conflict with an existing one).

Looks like it works really nicely -- a really great addition. Thanks so much for contributing this @RussPitcher.

I'll let you revert the changelog addition. The other thing that might be worth adding is a couple of tests, since @glennsarti went to some effort to write tests for the folding code.

I'll let @tylerl0706, @rkeithhill and @glennsarti review as well before we do anything.

rjmholt · 2018-09-27T17:40:01Z

CHANGELOG.md

@@ -1,5 +1,8 @@
 # vscode-powershell Release History

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


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.

rjmholt · 2018-09-27T17:40:57Z

package.json

@@ -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,


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

rjmholt · 2018-09-27T17:43:05Z

src/features/Folding.ts

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


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)?

+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?

rjmholt · 2018-09-27T17:43:52Z

src/features/Folding.ts

@@ -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--;


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

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

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.

glennsarti

Requires some changes to setting detection
+1 to making this the default. However this will change the setting detection
This really requires test changes if we make it the default, all of the tests will break.

glennsarti · 2018-09-28T00:14:31Z

src/features/Folding.ts

@@ -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--;


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

glennsarti · 2018-09-28T00:14:47Z

src/features/Folding.ts

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


+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?

glennsarti · 2018-09-28T01:36:49Z

@rjmholt @RussPitcher I've raised an alternate implementation of this PR at #1551. I've kept the commit messages horrible but if we choose to use that they can be cleaned up.

TylerLeonhardt · 2018-10-23T21:48:34Z

Closing this in favor of #1557 🙂

ens-rpitcher and others added 4 commits September 19, 2018 16:54

Initial attempt

a3e21d9

Merge pull request #1 from PowerShell/master

fe90445

Update from head fork

Add missing semicolon

52e38f8

Merge branch 'feature-code-folding-show-last-line'

39988ed

rjmholt requested review from TylerLeonhardt and rkeithhill September 27, 2018 17:17

rjmholt approved these changes Sep 27, 2018

View reviewed changes

glennsarti suggested changes Sep 28, 2018

View reviewed changes

glennsarti mentioned this pull request Sep 28, 2018

{WIP} (GH-1514) Show last line folding #1551

Closed

TylerLeonhardt closed this Oct 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

RussPitcher commented Sep 26, 2018 •

edited by rkeithhill

Loading

msftclas commented Sep 26, 2018 •

edited

Loading

rjmholt commented Sep 27, 2018

rjmholt commented Sep 27, 2018 •

edited

Loading

rjmholt left a comment

rjmholt Sep 27, 2018

rjmholt Sep 27, 2018

rjmholt Sep 27, 2018

glennsarti Sep 28, 2018

rjmholt Sep 27, 2018

glennsarti Sep 28, 2018

glennsarti Sep 28, 2018

glennsarti left a comment

glennsarti Sep 28, 2018

glennsarti Sep 28, 2018

glennsarti commented Sep 28, 2018

TylerLeonhardt commented Oct 23, 2018

		@@ -1,5 +1,8 @@
		# vscode-powershell Release History

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

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

	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());

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

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

Conversation

RussPitcher commented Sep 26, 2018 • edited by rkeithhill Loading

PR Summary

PR Checklist

msftclas commented Sep 26, 2018 • edited Loading

rjmholt commented Sep 27, 2018

rjmholt commented Sep 27, 2018 • edited Loading

rjmholt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennsarti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennsarti commented Sep 28, 2018

TylerLeonhardt commented Oct 23, 2018

RussPitcher commented Sep 26, 2018 •

edited by rkeithhill

Loading

msftclas commented Sep 26, 2018 •

edited

Loading

rjmholt commented Sep 27, 2018 •

edited

Loading