Skip to content
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.

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

folding: provider event to signal that folding ranges have changed #99914

Closed
Colengms opened this issue Jun 11, 2020 · 9 comments
Closed

folding: provider event to signal that folding ranges have changed #99914

Colengms opened this issue Jun 11, 2020 · 9 comments
Assignees
Labels
editor-folding Editor code folding issues feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders on-testplan
Milestone

Comments

@Colengms
Copy link
Contributor

In the C/C++ Extension, we have a scenario in which folding ranges may not be accurate if partially overlapping with inactive regions, until a semantic parse has been completed. The semantic parse allows us to determine if a #if/#ifdef region is active or inactive.

A contrived example:

#define TEST 0

void foo(bool b)
{
    if (b)
    {
#if TEST
        // ...
    }
#else
        // ...
    }
#endif
}

We could wait until the semantic pass is complete, but for very complex source it can take several seconds, possibly minutes.

We could provide ranges (as we are now) without considering inactive regions. If we receive a subsequent request for folding range (such as the user editing the file), we could take the inactive regions into consideration. Though, this would leave inaccurate folding ranges for files that are viewed and not edited.

If there were a way for us to programmatically trigger a new folding ranges request as inactive regions become available (or change), that would seem to provide the best experience.

(Related: microsoft/vscode-cpptools#5429 )

Version: 1.46.0 (user setup)
Commit: a5d1cc2
Date: 2020-06-10T09:03:20.462Z
Electron: 7.3.1
Chrome: 78.0.3904.130
Node.js: 12.8.1
V8: 7.8.279.23-electron.0
OS: Windows_NT x64 10.0.19041

@aeschli aeschli added editor-folding Editor code folding issues feature-request Request for new features or functionality labels Jun 15, 2020
@aeschli aeschli changed the title Request: A way to trigger a new folding ranges request (FoldingRangeProvider) folding: provider event to signal that folding ranges have changed Jun 15, 2020
@aeschli aeschli added this to the Backlog milestone Jun 15, 2020
@aeschli
Copy link
Contributor

aeschli commented Jun 15, 2020

I think a 'hasChanged' event makes sense, but returning ranges in patches is problematic.

The folding range infrastructure currently relies on the fact that it gets all folding ranges in one go.
In a document where the user has folded some ranges, we need to decide on every update whether the region needs to stay folded, or unfolded as it is no longer reported as a folded region.
If we were to keep it folded (because more ranges might come in), how long should we wait? It might never appear again as it really got removed.

Also, from a user experience it will be confusing of some ranges do not show up for several seconds, or if hidden line suddenly reappear or some get folded.

So could C++ have a good guess on the ifdef evaluations in order to return a complete set of ranges?
Or come up with a mix of ranges that are practical to the user regardless of the actual ifdef evaluations?

@Colengms
Copy link
Contributor Author

We cannot know the active/inactive status of all if/ifdef regions until a semantic parse has been completed. The boundary of such regions may be anywhere in code, except for within a string or comment. Either we wait for a semantic parse, proceed without it and possibly fold incorrectly, and/or return a fresh set once that information is available.

@aeschli
Copy link
Contributor

aeschli commented Jun 16, 2020

Is it is possible to already do some syntax based analysis before knowing which regions are active and inactive?
Would it make sense to always create folding ranges for the if/ifdef regions themselves (one range from #if to #else and one from #else to #end), and for blocks inside and around the regions, and discard all intersecting ranges?

@Colengms
Copy link
Contributor Author

Is it is possible to already do some syntax based analysis before knowing which regions are active and inactive?

Yes. Currently, our folding ranges are based on a (fast) syntactic parse. The only reason we would need a semantic parse would be to handle ranges split by preprocessor blocks.

Would it make sense to always ... discard all intersecting ranges?

That seems reasonable. Our folding algorithm is shared with VS. I assume VS has to address a similar scenario. Trying this in VS, it initially applies saved collapsed ranges against the syntactic results, then corrects them after a semantic parse completes. Perhaps it might be worthwhile to have a chat between VS Code and VS devs to share a common approach? (I'm not personally convinced that folding the other side of a split range to within the active preprocessor block is necessarily ideal behavior).

@aeschli
Copy link
Contributor

aeschli commented Jun 22, 2020

I personally would like to stick with our current model that folding providers have to return all ranges on every request. It's simpler from the implementation perspective but also beneficial to a the user experience. My hope is that C++ can construct useful folding ranges not depending on the active/inactive information.

The change event still makes sense, e.g. if a provider has folding settings that that impact the folding ranges visible in a document. I'm happy to add it if it's still useful.

If this is not good enough for your use case then I suggest we discuss this more in the next C++ sync call.

@Colengms
Copy link
Contributor Author

The change event still makes sense

@aeschli An event to indicate that folding ranges have changed, would work. :) Thanks.

@aeschli
Copy link
Contributor

aeschli commented Oct 19, 2020

I added the following proposed API:

	export interface FoldingRangeProvider {

		/**
		 * An optional event to signal that the folding ranges from this provider have changed.
		 */
		onDidChangeFoldingRanges?: Event<void>;

		/**
		 * Returns a list of folding ranges or null and undefined if the provider
		 * does not want to participate or was cancelled.
		 * @param document The document in which the command was invoked.
		 * @param context Additional context information (for future use)
		 * @param token A cancellation token.
		 */
		provideFoldingRanges(document: TextDocument, context: FoldingContext, token: CancellationToken): ProviderResult<FoldingRange[]>;
	}

I created #108929 to finalize that API in the next milestone.

@Colengms
Copy link
Contributor Author

Colengms commented Nov 6, 2020

@aeschli Did this go into 1.51? I'm not seeing the event in @types/vscode 1.51.0

@aeschli
Copy link
Contributor

aeschli commented Nov 9, 2020

It's in vscode.proposed.d.ts and beeing finalized fin 1.52: #108929

@github-actions github-actions bot locked and limited conversation to collaborators Dec 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
editor-folding Editor code folding issues feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders on-testplan
Projects
None yet
Development

No branches or pull requests

3 participants
@aeschli @Colengms and others