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

use lightweight mode by default when vscode is running from web browsers #1780

Merged

Conversation

Eskibear
Copy link
Contributor

The purpose is to reduce pop-up notifications.
Here we remove the notification, and by default only launches LS in lightweight mode if vscode is running from a web browser. It's based on the hypothesis that when users open vscode on a web browser they mainly want to browse code or do simple editing.

@fbricon
Copy link
Collaborator

fbricon commented Jan 27, 2021

@rgrunber make sure this change doesn't break the Che/Theia integration

@rgrunber rgrunber assigned rgrunber and unassigned rgrunber Jan 27, 2021
Copy link
Member

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

This definitely affects Theia. env.uiKind is '2' (UIKind.Web) so the various prompts are skipped. Perhaps env.appName may need to be used to further narrow this down ?

@Eskibear
Copy link
Contributor Author

I just tested some scenarios on VS Code, below are possible values I found:

  • env.uiKind
    • 1 for desktop
    • 2 for web
  • env.appName
    • Visual Studio Code
    • Visual Studio Code - Insiders
  • env.remoteName
    • wsl when using Remote-WSL
    • ssh-remote when using Remote-SSH
    • codespaces when using GitHub Codespaces

So yes, we can check env.appName if you want to keep this notification in Che/Theia. Furthermore, with env.remoteName we actually have the potential to optimize for different scenarios.

@akaroml
Copy link
Contributor

akaroml commented Jan 28, 2021

I may suggest env.uiKind === "web" && env.appName.contains("Visual Studio Code")

@Eskibear
Copy link
Contributor Author

OK, I've updated the PR to check both env.uiKind and env.appName

Copy link
Member

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

Looks fine to me. I can confirm Theia is excluded.

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.

None yet

5 participants