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 some UX to let users know that having multiple formaters is discouraged #71173

Closed
isidorn opened this issue Mar 26, 2019 · 26 comments
Closed
Assignees
Labels
feature-request Request for new features or functionality formatting Source formatter issues verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@isidorn
Copy link
Contributor

isidorn commented Mar 26, 2019

Refs: #71093

If a user has multiple formatters we will always bother him to choose the one to use.
However nowhere do we say or help the user to know that having multiple formatters is discoureged and that he should disable some.
Since that is exactly what we want, and nowhere do we mention this.
The gear icon in the quick open is far fetched imho

I suggest the following but am open to more ideas:

  1. The very first time per workspace the format or format with actions is triggered and there are multiple show an INFO notification
    "There are multiple formatters for this file type. Please disable one of the formatters either via settings or extensions vielwet" - and have two actions (open extensions viewlet, open user settings)
  2. Stop doing this in the status bar. It is too short, and very easy to miss. And can easily get clogged with other content in the status bar

fyi @misolori

@isidorn
Copy link
Contributor Author

isidorn commented Mar 26, 2019

Also the status bar is very noise if I have format on save and I save every minute like people usually do.
Thus I am more convinced that we should not show it there all the time.

@jrieken
Copy link
Member

jrieken commented Mar 26, 2019

Stop doing this in the status bar. It is too short, and very easy to miss. And can easily get clogged with other content in the status bar

What status bar? We are not doing anything there

@jrieken jrieken added formatting Source formatter issues under-discussion Issue is under discussion for relevance, priority, approach labels Mar 26, 2019
@jrieken
Copy link
Member

jrieken commented Mar 26, 2019

The very first time per workspace the format or format with actions is triggered and there are multiple show an INFO notification

Yes - that something we have considers. It's just not so easy because the "first" happens for different languages, e.g. different extensions, with different config stories

@isidorn
Copy link
Contributor Author

isidorn commented Mar 26, 2019

If I have format on save enabled and have multiple formatters I get a notification in the status bar which formatter got selected every time I press cmd + s (as you have specified in the test plan item).

status bar

@isidorn
Copy link
Contributor Author

isidorn commented Mar 26, 2019

For "first" can't we just say first per language per workspace.

@jrieken
Copy link
Member

jrieken commented Mar 26, 2019

Just asking because prettier contributes it's own status bar message which confused people.

For "first" can't we just say first per language per workspace.

Ok, it seems that you have missed all standup- and water-cooler-discussion here and that you didn't read #41882. Short answer, 'no' we cannot do that, it doesn't make any sense to add another setting that effectively enables/disables formatter when all formatter already have a setting to enable/disable them.

@isidorn
Copy link
Contributor Author

isidorn commented Mar 26, 2019

I actually listened to you in standups. What I meant was a storage service flag that would only influence if we should show a notificaiton via the the notifcation service. So it would not affect the actual behavior, just if we show a notification or not.

@jrieken jrieken added this to the March 2019 milestone Mar 26, 2019
@jrieken
Copy link
Member

jrieken commented Mar 26, 2019

Ok, I start to understand what you are saying... I am not a fan of showing this only once because you likely dismiss it the first time without actually reading/understanding what it said. What we could do is the following

  • per language and set of formatter (extensions)
  • show a notification when multiple formatters exist
  • show the same notification as silent notification from then one

The notification would guide you towards the extensions viewlet (maybe settings editor) and gives you a nice, stable way to configure yourself out of this.

However, this doesn't really solve the issue that folks what to know what happens when multiple formatters are active and which formatter is actually being used. The status bar message is meant to be borderline annoying TBH but the its weak spot is that it doesn't guide you out of this

@octref
Copy link
Contributor

octref commented Mar 26, 2019

Can we possibly do this, together with solving #71196:

  • For any "Format with..." command, show an additional option at last "Resolve multiple formatters"
  • Drop the gear
  • Make StatusBar message, when clicked, also run the command "Resolve multiple formatters"
  • "Resolve multiple formatters" command opens settings editor while sending a notification that guides you to disable formatter extensions.

@octref
Copy link
Contributor

octref commented Mar 26, 2019

It could be added below a divider so it doesn't confuse people as an option, too:

image

So this image becomes:

image

  • Vetur
  • Prettier — Code formatter
  • ------
  • Resolve multiple formatters

@jrieken
Copy link
Member

jrieken commented Mar 26, 2019

"Resolve multiple formatters" command opens settings editor while sending a notification that guides you to disable formatter extensions.

That's not so easy... For once the settings editor cannot be told to show settings from a certain extensions and than there are some extensions that only format a single language. For them, having a setting doesn't make sense and you should just un-install them.

@octref
Copy link
Contributor

octref commented Mar 26, 2019

A very common use case is you juggle through multiple repos, some using TS formatter, and some using Prettier. I don't see myself uninstalling or disabling one of them completely, even for workspace. For example they conflict on JS but Prettier has a useful JSON formatter that I always use when editing package.json.

The message can be a simple message that prompts user's action (instead of doing the action itself), like "X formatters found for the file X.xxx, you can disable some of them in workspace configuration to avoid formatter conflict".

@jrieken
Copy link
Member

jrieken commented Mar 26, 2019

For example they conflict on JS but Prettier has a useful JSON formatter

Prettier is not a simple formatter for a single language, the css-formatter1 is and for that I would not expect a setting but I would use workspace enablement or uninstalling.

The case of different projects and different formatters is interesting but I think the workspace-settings solution is flawed. Take vscode, we use the builtin TypeScript formatter. Now, you install prettier because of personal preference and/or because you use it in other projects. Now, why would it be ok that the vscode project has a setting to configure prettier? Isn't that a weird coupling? Everyone not using prettier would see a warning about an unknown setting. I really don't know a good answer to this...

@octref
Copy link
Contributor

octref commented Mar 26, 2019

Now, why would it be ok that the vscode project has a setting to configure prettier? Isn't that a weird coupling?

Yeah, that's the last piece to the puzzle I hope we can fill. What I really want is a workspace config that allows me to say "For *.ts file, use TS Formatter in this workspace".

Otherwise each project would end up adding:

{
  "js-beautify.enable": false,
  "prettier.enable": false,
  "yet-another-popular-ts-formatter.enable": false
  ...
}

@ayyash

This comment has been minimized.

@jrieken
Copy link
Member

jrieken commented Mar 27, 2019

After talking to @weinand and @aeschli and after understanding all the feedback and the use-cases (thanks for that) we think it's time for one more setting: editor.defaultFormatter: <extensionId>. That ensures you can mutually exclusive configure one formatter. The new setting can be defined per language and is "stronger" than all other formatter settings.

Samples:

⬇️ This configures prettier to be the formatter for all things. That would also mean for foo-files even when prettier doesn't support foo-files. Because of that you likely want to configure default formatters per language...

  "editor.defaultFormatter": "esbenp.prettier-vscode"

⬇️ This configures beautify to be used as the only formatting option for TypeScript. If beautify isn't available or disabled somehow else then no formatting happens.

"[typescript]": {
  "editor.defaultFormatter": "HookyQR.beautify"
}

There will be UI hints to guide you towards configuring formatters (in the presence of conflicts and absence of configuration). A notification will show when running format explicit, e.g Format Document, Format Selection, the notification will show silent (minified) when running format implicit, e.g format on save.

A sample-gif! It starts with format on save and the silent notification, then comes format document and the normal notification. Notice how the configuration updates for TypeScript and how format document isn't showing notifications anymore. There is still 'format with...' which allows to pick a formatter for that one format run. It shows the default and we consider allowing to change defaults from here (as suggested by @octref)

Mar-27-2019 17-18-34

When a configured extension isn't available (didn't contribute a formatter) then we also show a notification which guides you towards using a different formatter

Mar-27-2019 17-24-14

@isidorn
Copy link
Contributor Author

isidorn commented Mar 27, 2019

I think this is a great improvement. Nice work!

@jrieken
Copy link
Member

jrieken commented Mar 27, 2019

All my changes aren't yet on master but here: #71301

@isidorn
Copy link
Contributor Author

isidorn commented Mar 27, 2019

I can verify and test it more tomorrow morning if you would like.

@jrieken
Copy link
Member

jrieken commented Mar 27, 2019

And last but not least the revised 'format with...' flow, showing the current default and allowing to change that

Mar-27-2019 18-57-20

@octref
Copy link
Contributor

octref commented Mar 27, 2019

A few great new features here, so I added feature-request and verification-needed.

@isidorn I'll build and test it this afternoon, would be great if you can also give it a try tomorrow to catch anything I miss.

@octref octref added feature-request Request for new features or functionality verification-needed Verification of issue is requested and removed under-discussion Issue is under discussion for relevance, priority, approach labels Mar 27, 2019
@jrieken jrieken closed this as completed Mar 28, 2019
@jrieken
Copy link
Member

jrieken commented Mar 28, 2019

Please verify that

  • you get a prompt when having multiple formatters for a language and when explicitly formatting (format document, format selection, format on paste)
  • you get a silent prompt when doing format on save and having formatter conflicts
  • the prompt guides you out of the conflict
  • there is format with... which allows to chose a different formatter and to revisit the configuration
  • the editor.defaultFormatter can be configured in the settings editor - check doc and values
  • when a configured formatter isn't available you get a status bar message

@isidorn
Copy link
Contributor Author

isidorn commented Mar 28, 2019

Great job, works nice. I have created some follow up items and have linked them here so @octref can also express his opinion.

@octref
Copy link
Contributor

octref commented Mar 28, 2019

Works great. One feedback I have is the silent notification for onsave formatting is not very discoverable. I understand the design is to not annoy people with a notification on each save, but now people can (and should) configure their way out of the multiple formatters scenario, maybe just show a notification anyway?

@dubeg
Copy link

dubeg commented Apr 4, 2019

Couldn't the command palette be boosted with a label above the input (that wouldn't take space if empty) so that vscode can explain what's going on while immediately jumping to action, choosing a default formatter in this case?

@jrieken
Copy link
Member

jrieken commented Apr 5, 2019

@dubeg Not sure that I understand...

@vscodebot vscodebot bot locked and limited conversation to collaborators May 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality formatting Source formatter issues verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

5 participants