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 output.showQuietly to stop extensions activating Output view (fix #105270) #205225

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

gjsjohnmurray
Copy link
Contributor

This PR fixes #105270

When the new output.showQuietly setting is true calls to OutputChannel.show or LogOutputChannel.show when Output view is not visible will produce a silent notification rather than showing the view.

Like all silent notifications this will add a dot to the bell icon at the right-hand end of the status bar. Clicking on this will show the notification which includes a button to show the Output view and the channel.

@gjsjohnmurray
Copy link
Contributor Author

/assign @sandy081

Please consider this. Feedback welcome.

@sandy081
Copy link
Member

I am wondering why this is output only feature. An extension can open any view in the panel for eg., terminal and should we have a setting per view? Please consider a general solution.

@sandy081
Copy link
Member

CC @sbatten

@sandy081 sandy081 assigned sbatten and unassigned sandy081 Feb 19, 2024
@gjsjohnmurray
Copy link
Contributor Author

@sandy081 AFAIK these are the API classes with methods an extension can use and which could cause a view container to change what it is presenting to the user:

  • LogOutputChannel.show
  • OutputChannel.show
  • Terminal.show
  • TreeView.reveal
  • WebviewView.show

I didn't find any built-in commands that could have this kind of effect.

My PR currently addresses the first two, which both act on the Output view.

One idea for a generalized approach would be a workbench.view.showQuietly setting

"workbench.view.showQuietly": {
  "workbench.panel.output": true, // the PR's current extent
  "terminal": true,
  "foo.bar.tree": true,
  "foo.bar.webview": true,
  ...
}

What do you think?

@gjsjohnmurray
Copy link
Contributor Author

Research: existing OSS view ids

image

@sandy081
Copy link
Member

I would ask @sbatten for his opinion here.

@jstm88
Copy link

jstm88 commented Feb 22, 2024

  • LogOutputChannel.show
  • OutputChannel.show
  • Terminal.show
  • TreeView.reveal
  • WebviewView.show

It certainly makes sense that some of these could be covered as well, as noted by the other feature request.

One thing: I'm not sure if TreeView here refers to the Explorer or the Outline (the user facing UI doesn't refer to a Tree View as far as I can tell). But I am reminded of my keybinding for workbench.files.action.showActiveFileInExplorer. It's directly triggered through VS Code (not an extension) so it shouldn't be affected, but just something to keep in mind depending on how the logic for that view gets triggered. I would assume other direct triggers like Ctrl+~ for the Terminal would also be exempt if any of these were to have a similar quiet option added.

Copy link

@jstm88 jstm88 left a comment

Choose a reason for hiding this comment

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

Code looks good to me. Not sure if my approval counts for anything here but I'd like to move this along if I can. :)

EDIT: As I suspected mine doesn't count for collaborator approvals. Really hope the VS Code team doesn't ignore this like they've ignored the years of bug reports leading up to it. 🙄

@gjsjohnmurray
Copy link
Contributor Author

In February @sandy081 wrote:

I would ask @sbatten for his opinion here.

@sbatten what do you think?

@stewartadam
Copy link
Member

Followed a thread of closed issues to land here, I'd love to see this merged. Extensions constantly stealing focus is infuriating and particularly annoying when e.g. rebuilding and starting devcontainers,

In February @sandy081 wrote:

I would ask @sbatten for his opinion here.

@sbatten what do you think?

@sbatten kindly requesting your input so this can move forward

@sbatten
Copy link
Member

sbatten commented Jun 21, 2024

Sorry, I didn't see this notification until someone pinged me about it directly. With this solution, won't existing, well-behaving extensions that have explicit user actions (say a button in a dialog) end up with a broken experience. Sure it is opt-in by the user, but that experience seems like it falls apart. Adding @benibenj as he takes ownership over layout related items now.

@stewartadam
Copy link
Member

stewartadam commented Jun 21, 2024

@gjsjohnmurray can confirm but I think this shouldn't affect UX around notification or pop-ups, this is just allowing output to silently continue without affecting layout or stealing user focus away from the editor.

Personally - I generally don't care at all about any of the extension outputs because they're not actionable unless I'm debugging a failure (which should present a toast notification or other UI interaction) so I'd love to see this setting added.

@sbatten
Copy link
Member

sbatten commented Jun 21, 2024

What I mean is, some extensions encounter an error and then show a notification with a button for "Show output logs"... This button would just "break"

@gjsjohnmurray
Copy link
Contributor Author

In @sbatten"s scenario the user who opted in will need to take 2 additional actions after clicking the button on the extension's notification (or wherever):

  1. Click on the dotted bell bottom right.
  2. Click on the button in the notification.

To cater for well-behaved extensions we could make the setting more advanced, allowing ids of good extensions to be listed as exceptions. And/or support listing of bad extension ids that the new behaviour will apply to.

@gjsjohnmurray
Copy link
Contributor Author

@sbatten I am willing to put effort into making the setting more granular if that will lead to the PR being accepted. Please advise.

@benibenj
Copy link
Contributor

benibenj commented Jul 11, 2024

Sorry for the wait @gjsjohnmurray. As this is adding a new Setting, I'd like to make sure this is actually something we should be fixing as it is actually caused by extensions. The issue currently only mentions one extension and the output view. If this is the main problem then we should make sure the problem is solved by the extension. If the problem needs to be in core, we need to understand if it is an output problem or a view revealing problem. I'll continue the conversation in the issue first.

@stewartadam
Copy link
Member

It's definitely an issue with multiple extensions - it's particularly frustrating with dev containers as each extension initializes with slightly different timing and the net user experience is stolen focus to open the Output pane several times.

I think more broadly, trying to fix a distributed problem is going to perpetuate this bad UX.

I don't want anything stealing focus out of my editor, and I'd love a setting to enforce that instead of trying to convince a half dozen extension maintainers to do the right thing 😅

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.

Add an option to prevent side panel from opening
9 participants