This repository was archived by the owner on Sep 6, 2021. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
Validate editor preferences #7186
Merged
Merged
Changes from 20 commits
Commits
Show all changes
29 commits
Select commit
Hold shift + click to select a range
1274227
Editor Preferences Validation - interim checkin
redmunds 1b1a604
start ValidationUtils module
redmunds 7f721fc
Merge remote-tracking branch 'origin/master' into randy/issue-7020
redmunds 8d5482e
convert remaining editor prefs
redmunds bc7b0bf
cleanup
redmunds d0ef02f
fix range limit of zero
redmunds 3719aa2
more tests; cleanup comments
redmunds cfb0cce
implement preference validator
redmunds 3428312
update for preference validator
redmunds f476ac5
changes for code review
redmunds 8268997
more code review changes
redmunds fd72a4f
fix unit test; use ValidationUtils for validating\!
redmunds 0b441d9
fix validation
redmunds 5d22e35
add unit test for validator
redmunds 0824ff6
implement validator on set
redmunds a32a39b
code cleanup
redmunds b6fb768
add jsdoc info
redmunds 3a158fa
merge latest master
redmunds 35b18b8
add test for negative space units
redmunds 2418344
restore original API as deprecated
redmunds bc71a60
return both and from set()
redmunds a845bab
merge latest master
redmunds d2d4c72
fix tests
redmunds 9883d3c
merge with latest master
redmunds 835bc45
use lodash to generate validator function
redmunds 2062a78
revert to old API with a few small changes
redmunds 3d82669
jsdoc updates
redmunds aca8a81
Merge remote-tracking branch 'origin/master' into randy/issue-7020
redmunds 76a5b91
fix whitespace
redmunds File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these moving from Editor to Editor.prototype? We'd have to look at the backwards compatibility implications of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this so we have
this.document.file.fullPath.Yes, I had to rework the code in
EditorStatusBar. One implication (of the new preferences changes in general, and not this change in particular) is that the status bar now updates for the focused editor. So, if you happen to have different prefs for .html and .css files, then when you switch focus between main and inline css editor, the prefs in status bar with change for the current file being edited. Or should status bar always show prefs for main editor?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems like a positive change to me.
Still, this is a change to public API, which means we should probably deprecate the old API and not just remove it. You could use the Brackets Extension Grabber to see if there are any extensions that call
Editor.set*orEditor.get*, or you could just add deprecation warnings.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@redmunds The status bar already behaves this way (e.g. the language indicator updates). Are you saying there was a bug where the indent indicator didn't update on focus change before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tested in master and the status bar already updates the indent indicator too when focus switches between the inline editor and the full-size editor -- so the behavior isn't changing here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peterflynn Agreed. Thanks for doing the search!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the PreferencesManager uses the current file when a file is not passed as a param, but I can see that being a problem with split view or multiple window support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I will restore the original API. Any tips or sample code I can look at for displaying "deprecated" messages to console?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Original API restored and deprecated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@redmunds Good points. Pretty sure the old static APIs would still work fine with split views as well, for the same reason inlines do today (activeEditor changes with focus, and there's only one statusbar shared between the views)... but it's definitely much cleaner to be explicitly calling out an Editor instance and thus explicitly requesting a file-specific pref.