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

Add miscellaneous file open notifications #7652

Merged
merged 3 commits into from
Oct 11, 2024
Merged

Conversation

JoeRobich
Copy link
Member

@JoeRobich JoeRobich commented Oct 10, 2024

Gives the user more warning that they are editing a file outside their workspace.

image

The same notification text is now used in the language status bar when a miscellaneous file is open.

image

Features:

  • Allows the user to easily silence the notification at the workspace level.
  • A setting was added to silence the notification at the user or workspace level.
  • Only shown once a session for a particular document.

@dibarbet
Copy link
Member

dibarbet commented Oct 10, 2024

Should we show the filename in the notification? When the notification text is different multiple notifications can stack up.

Can we potentially throttle it somehow? For example if a notification is already showing, we update the notification to be 'Multiple open files..." so that further notifications don't stack up?

What should the message say?

I'm fine iwth the text shown in the picture, seems clear enough

Should we notify about the same file more than once a session?

I would say no - once per session seems reasonable

Do we need the granularity of disabling at the workspace level?

I would lean towards yes - there might be workspaces that have lots of misc files and others not. But not 100% sure.

Should we store the "Do not show ever" setting in the users settings so that it can roam with them?

I think that would be a good idea actually - yeah.

@JoeRobich JoeRobich marked this pull request as ready for review October 10, 2024 23:16
@JoeRobich JoeRobich requested a review from a team as a code owner October 10, 2024 23:16
context.workspaceState.update(SuppressMiscellaneousFilesToastsOption, undefined);

languageServer._projectContextService.onActiveFileContextChanged((e) => {
const hash = createHash(e.uri.toString(/*skipEncoding:*/ true));
Copy link
Member

Choose a reason for hiding this comment

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

maybe move this down below the other checks, since it could be more expensive?

NotifiedDocuments.add(hash);

const message = vscode.l10n.t(
'The active document is not part of the open workspace. Not all language features will be available.'
Copy link
Member

Choose a reason for hiding this comment

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

I was wondering if we wanted to add like a learn more docs link with how they might tell why the file is in misc. For example, making sure the file is part of a project, making sure the language server has a project loaded, making sure the project is part of the loaded sln, etc.

Copy link
Member Author

@JoeRobich JoeRobich Oct 10, 2024

Choose a reason for hiding this comment

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

I think that is a good idea and we can do it in a follow up.

@webreidi I see VS has a page on Misc Files. Is that a good one to link to for now? Should we have a C# ext. centric page created?

Copy link
Member Author

Choose a reason for hiding this comment

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

Created draft PR #7654 to explore this further.

Choose a reason for hiding this comment

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

I think the VS Link will only cause confusion. But we can add to the FAQ the specifics of this.

@JoeRobich JoeRobich changed the title Add miscellaneous file open notifications Add miscellaneous file open notifications Oct 11, 2024
@JoeRobich JoeRobich force-pushed the dev/jorobich/warn-misc-files branch 2 times, most recently from 1e08959 to 321ffe3 Compare October 11, 2024 05:28
@JoeRobich JoeRobich merged commit 3fc374c into main Oct 11, 2024
10 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants