Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Validate settings #974

Merged
merged 5 commits into from
Jan 5, 2018
Merged

Validate settings #974

merged 5 commits into from
Jan 5, 2018

Conversation

winstliu
Copy link
Contributor

@winstliu winstliu commented Aug 6, 2017

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

Previously, Settings View would set the setting value after the editor stopped changing. It would also update the editor's value whenever the actual setting changed on disk. This worked fine for all cases outside of when an invalid value was entered in the editor. In this case (assuming min of 0, max of 100), the editor changes to 0. This is an invalid value, which Atom core changes to 1. The observe callback fires, and the editor view is changed back to 1. However, when entering another invalid value below the minimum, the on-disk config does not change and the observe callback is not fired. This leaves the Settings View with a visual 0 value but an actual value of 1. Similarly, when entering a value that is unable to be coerced to the correct type (eg abcde for a setting of type integer), no observe callback is fired and the Settings View is again inconsistent with the on-disk value.

I've fixed both of these scenarios by validating the setting value inside of Settings View before passing it to Atom. If the new value is out of the valid range, the closest value is selected and the editor field is updated. If the new value is unable to be coerced, the editor field is reset to the on-disk value.

Alternate Designs

Maybe there's a simpler solution to this.

Benefits

Less inconsistencies between Settings View and config.cson.

Possible Drawbacks

Not 100% sold that value < minimum will always work as expected. If the only type that can have minimums and maximums is integer then it should be fine.

In order to test this, I had to manually emit the did-stop-changing event so that onDidStopChanging would be called. While it works, I would definitely be open to a way that isn't private and doesn't chain absurdly deep :).

Applicable Issues

Fixes #484

@winstliu winstliu merged commit 9315802 into master Jan 5, 2018
@winstliu winstliu deleted the wl-settings-validation branch January 5, 2018 02:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting restricted to a numeric range doesn't restrict when changed and changed again
1 participant