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

Make channel log level settable from output view #205159

Merged
merged 8 commits into from
Mar 7, 2024

Conversation

gjsjohnmurray
Copy link
Contributor

@gjsjohnmurray gjsjohnmurray commented Feb 13, 2024

This PR closes #162262

image

Choosing the option takes you directly to the second quickpick presented by the existing Developer: Set Log Level... command:

image

Like all entries on this overflow menu the user can promote the new option to be a button:

image

For channels that don't support log levels the option gets disabled rather than hidden, so it matches the behaviour of existing overflow options, and so when shown as a button the layout doesn't change as you scroll through channels:

image

image

@gjsjohnmurray
Copy link
Contributor Author

/assign @sandy081

Pinging @mjbvz as author of the related issue. This PR doesn't achieve all that was requested, but I hope it helps.

@sandy081
Copy link
Member

@gjsjohnmurray Thanks for contributing the PR. I liked the new behaviour. I have one feedback

  • I would prefer to open the log level options inline - meaning - next to the configure log level action. It can be a menu or sub menu based on the location of the action.

@sandy081 sandy081 self-requested a review February 21, 2024 18:14
@sandy081 sandy081 added this to the March 2024 milestone Feb 21, 2024
@gjsjohnmurray
Copy link
Contributor Author

@sandy081 as a (sub)menu I don't think we can achieve both functions of the quickpick, in which you can set the log level for the channel until the next restart and the default level that will apply after restarts (the double checkmark on the right-hand end of a level).

Assuming the (sub)menu item picked will get a single checkmark and set the level only for the current session, maybe the initial button / overflow menu item should be captioned "Set Current Log Level..." rather than how they appear in my previous screenshots ("Configure Log Level...").

@sandy081
Copy link
Member

That's right, we would need two actions

  1. Set Log Level (Session or Window)
  2. Set Log Level (Default)

I would make first action that can be able to made a primary action and second action just a secondary action.

@gjsjohnmurray
Copy link
Contributor Author

@sandy081 this is ready for your review. It now uses a submenu, but instead of adding a second submenu for setting the channel's default level I added an option at the end of the submenu which updates the default to match the current choice.

image

This option is only enabled when the default differs from the current level.

Copy link

@the-coder-o the-coder-o left a comment

Choose a reason for hiding this comment

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

If that's the case, you would need to find all occurrences in the code where createInstance is called with the channelId and remove that argument. If the channelId was providing necessary context for the SetLogLevelAction, you'll need to find a new way to provide that context or reconsider the removal of the channelId parameter.

src/vs/workbench/contrib/logs/common/logs.contribution.ts Outdated Show resolved Hide resolved
@gjsjohnmurray
Copy link
Contributor Author

As with other action buttons, this one can be hidden, causing the option to move to the ... menu.

image

src/vs/platform/log/common/log.ts Outdated Show resolved Hide resolved
src/vs/workbench/contrib/output/browser/outputServices.ts Outdated Show resolved Hide resolved
@sandy081 sandy081 enabled auto-merge March 7, 2024 11:15
@sandy081 sandy081 merged commit 4dc296a into microsoft:main Mar 7, 2024
6 checks passed
@gjsjohnmurray gjsjohnmurray deleted the do-162262 branch March 8, 2024 10:35
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.

Show the log level closer to output channel and ability to configure it
4 participants