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

Auto save in new settings results in multiple validation errors when typing text #58537

Closed
ramya-rao-a opened this issue Sep 12, 2018 · 10 comments · Fixed by #58926
Closed

Auto save in new settings results in multiple validation errors when typing text #58537

ramya-rao-a opened this issue Sep 12, 2018 · 10 comments · Fixed by #58926
Assignees
Labels
feature-request Request for new features or functionality settings-editor VS Code settings editor issues verification-needed Verification of issue is requested verified Verification succeeded

Comments

@ramya-rao-a
Copy link
Contributor

Steps to Reproduce:

  1. Open new settings editor, search for "emmet extension"
  2. Emmet: Extensions Path is a setting that takes path to a directory. Start typing a path

Since we auto-save in 100ms, the config change event is fired for almost every other character typed, which results in validation checks from the emmet extension resulting in a series of error notifications.

There might be other extensions in the marketplace that do such validations after listening the config change event.

@vscodebot vscodebot bot added the emmet Emmet related issues label Sep 12, 2018
@ramya-rao-a
Copy link
Contributor Author

ramya-rao-a commented Sep 12, 2018

Suggestions:

  • Increase the delay for auto-save to 1000ms. This is the default delay for file auto save. Or re-use the value from the auto save delay setting. This will not solve the problem, but will make it less annoying as the validations will be spaced more apart
  • Or Allow settings to opt-in to a much more delayed save than 100ms
  • Or Dont auto-save settings by default. Have a save button and an option to turn on auto save.

cc @Microsoft/vscode

@ramya-rao-a ramya-rao-a added settings-editor VS Code settings editor issues and removed emmet Emmet related issues labels Sep 12, 2018
@roblourens roblourens added the feature-request Request for new features or functionality label Sep 12, 2018
@jrieken
Copy link
Member

jrieken commented Sep 13, 2018

I will add another suggestion:

  • save only on focus out, e.g. when the settings editor (or any of its input fields) lose focus then save settings

@dbaeumer
Copy link
Member

or we make it dependent on the setting changed. A 1 out of n selection can save right after the value changed. A field were the user types should save after a longer delay or when the focus gets lost.

@joaomoreno
Copy link
Member

Atomic setting changes should react as fast as possible (1 out of n selection, checkboxes, etc). Text input changes (and similar) should probably work on focus out, like @jrieken suggests.

@roblourens
Copy link
Member

I think that increasing the debounce timeout for text boxes would be good, but I'm worried that only committing the change on blur could confuse users. At least for some settings, like if you change window.title, or a font family/size, or anything else that affects the appearance of the editor... letter spacing, line height, tab size, etc... and you don't see it take effect. Then you start hunting around for a "save" button. Not everyone will try clicking out of the text box at this point.

@sbatten
Copy link
Member

sbatten commented Sep 13, 2018

I don't think it has to be one or the other. We can save on focus out as well as with a bit longer delay (without losing focus) so if the user is quickly moving through settings or is just a fast typist and moves away from the settings view, it just saves immediately. Otherwise, it saves after some delay.

@roblourens roblourens added this to the September 2018 milestone Sep 21, 2018
@roblourens roblourens added the verification-needed Verification of issue is requested label Sep 21, 2018
@attdona
Copy link

attdona commented Sep 25, 2018

My extension has N properties, and an action triggered by onDidChangeConfiguration make sense only when the user has finished changing more than one property.

In my opinion the optimal way is to give the user full control through an explicit Save action, otherwise an implicit save on focus out (no properties in focus) should work as expected.

@isidorn isidorn added the verified Verification succeeded label Sep 25, 2018
@roblourens
Copy link
Member

My extension has N properties, and an action triggered by onDidChangeConfiguration make sense only when the user has finished changing more than one property.

You can't guarantee that even in the JSON editor, some users will probably have autosave turned on, or just hit cmd+s at different times like I do by habit. I think it's better to handle that on the extension side, rather than have a setting editing "session".

@ramya-rao-a
Copy link
Contributor Author

Being the devil's advocate here.. (actually extension authors' advocate)...

You can't guarantee that even in the JSON editor, some users will probably have autosave turned on, or just hit cmd+s at different times like I do by habit.

While that is true for such users, we are now moving away from a bulk update -> one event fired most of the times to one setting update -> one event fired all the time in the new settings editor. Which means much more traffic... It is slightly concerning

I think it's better to handle that on the extension side

How do you suggest extensions handle this? Batch all the config changes, and act on the batch on regular intervals? Why have multiple extensions do that when the product can do that once?

@roblourens
Copy link
Member

How do you suggest extensions handle this? Batch all the config changes, and act on the batch on regular intervals?

Yes. I've forked this into #59391 and we can keep discussing there.

@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 5, 2018
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 settings-editor VS Code settings editor issues verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants