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

Fix dirty setting update logic #58926

Merged
merged 8 commits into from Sep 21, 2018

Conversation

Projects
None yet
2 participants
@guywald1
Contributor

guywald1 commented Sep 19, 2018

This PR closes #58537.

Setting that are dirty will now update:

  • On changes when focused - with a debounce of 1000ms
  • On blur - immediately

guywald1 added some commits Sep 19, 2018

Fix dirty setting update logic
* Dirty setting will update when focused with a higher debounce
* Dirty setting will immediately update when blurred
@roblourens

This comment has been minimized.

Show comment
Hide comment
@roblourens

roblourens Sep 20, 2018

Member

I'm not sure, I liked the comment that only text setting updates should have a slow debounce, and other types should be committed as fast as possible. What do you think? Should we send the type with onDidChangeSetting, and debounce differently based on the type?

Member

roblourens commented Sep 20, 2018

I'm not sure, I liked the comment that only text setting updates should have a slow debounce, and other types should be committed as fast as possible. What do you think? Should we send the type with onDidChangeSetting, and debounce differently based on the type?

@roblourens roblourens added this to the September 2018 milestone Sep 20, 2018

guywald1 added some commits Sep 20, 2018

Change behavior for non-text inputs
* Added a new enum for setting types
* Added separate `Delayer` with a long debounce for non-text inputs
@guywald1

This comment has been minimized.

Show comment
Hide comment
@guywald1

guywald1 Sep 20, 2018

Contributor

@roblourens Updated :)

Contributor

guywald1 commented Sep 20, 2018

@roblourens Updated :)

@roblourens

Thanks!!

@roblourens roblourens merged commit ab13118 into Microsoft:master Sep 21, 2018

1 of 2 checks passed

VS Code #20180921.78 failed
Details
license/cla All CLA requirements met.
Details
@guywald1

This comment has been minimized.

Show comment
Hide comment
@guywald1

guywald1 Sep 21, 2018

Contributor

@roblourens Just making sure - CI build for latest commit says unit tests are failing on Linux, but it seems that the problem is not with my changes. Is it OK?

Contributor

guywald1 commented Sep 21, 2018

@roblourens Just making sure - CI build for latest commit says unit tests are failing on Linux, but it seems that the problem is not with my changes. Is it OK?

@roblourens

This comment has been minimized.

Show comment
Hide comment
@roblourens

roblourens Sep 21, 2018

Member

Yeah I checked it out and it looked like a flaky test 👍

Member

roblourens commented Sep 21, 2018

Yeah I checked it out and it looked like a flaky test 👍

@guywald1 guywald1 deleted the guywald1:58537-settings-autosave-debounce branch Oct 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment