Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/editor/CSSInlineEditor.js
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ define(function (require, exports, module) {
* @param {!{line:Number, ch:Number}} pos
* @return {?$.Promise} synchronously resolved with an InlineWidget, or
* {string} if pos is in tag but not in tag name, class attr, or id attr, or
* null if we're not going to provide anything.
* null if we're not even close to a context where we could provide anything.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind comments like this in the source but, for documentation, I don't think the API should feel like it has a pulse. Comments like "We can't determine the context" are fine in logic branches but in JSDoc it feels a little too folksy...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are already well over 1000 occurrences of "we" in our code today, so I think the horse maybe has already left the barn on that one :-) And this sentence was already structured that way...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that was my veiled attempt to get you to re-word since you added a second "we" to the sentence making it feel a little sarcastic and folksy but it's also somewhat ambiguous with the 2 references to "we" : "we're not even close to to a context where we could..."

Maybe "The insertion point is not close enough to a context where we could provide..." is a little less ambiguous. If you wanted to keep it folksy you could say "You're not even close enough to a context where we could..." but that might be too folksy :)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JeffryBooher Change pushed that rewords this a little bit. Lmk if you need anything more to merge.

*/
function htmlToCSSProvider(hostEditor, pos) {

Expand Down
16 changes: 8 additions & 8 deletions src/editor/EditorManager.js
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ define(function (require, exports, module) {
* @param {!Editor} editor The host editor
* @param {Array.<{priority:number, provider:function(...)}>} providers
* prioritized list of providers
* @param {string=} defaultErrorMsg Default error message to display if no initial provider found
* @param {string=} defaultErrorMsg Default message to display if no providers return non-null
* @return {$.Promise} a promise that will be resolved when an InlineWidget
* is created or rejected if no inline providers have offered one.
*/
Expand Down Expand Up @@ -279,10 +279,10 @@ define(function (require, exports, module) {
* An optional priority parameter is used to give providers with higher priority an opportunity
* to provide an inline editor before providers with lower priority.
*
* @param {function(!Editor, !{line:number, ch:number}):?$.Promise} provider
* @param {function(!Editor, !{line:number, ch:number}):?($.Promise|string)} provider
* @param {number=} priority
* The provider returns a promise that will be resolved with an InlineWidget, or returns null
* to indicate the provider doesn't want to respond to this case.
* The provider returns a promise that will be resolved with an InlineWidget, or returns a string
* indicating why the provider cannot respond to this case (or returns null to indicate no reason).
*/
function registerInlineEditProvider(provider, priority) {
if (priority === undefined) {
Expand All @@ -297,10 +297,10 @@ define(function (require, exports, module) {
* An optional priority parameter is used to give providers with higher priority an opportunity
* to provide an inline editor before providers with lower priority.
*
* @param {function(!Editor, !{line:number, ch:number}):?$.Promise} provider
* @param {function(!Editor, !{line:number, ch:number}):?($.Promise|string)} provider
* @param {number=} priority
* The provider returns a promise that will be resolved with an InlineWidget, or returns null
* to indicate the provider doesn't want to respond to this case.
* The provider returns a promise that will be resolved with an InlineWidget, or returns a string
* indicating why the provider cannot respond to this case (or returns null to indicate no reason).
*/
function registerInlineDocsProvider(provider, priority) {
if (priority === undefined) {
Expand Down Expand Up @@ -946,7 +946,7 @@ define(function (require, exports, module) {
*
* @param {Array.<{priority:number, provider:function(...)}>} providers
* prioritized list of providers
* @param {string=} errorMsg Error message to display if no initial provider found
* @param {string=} errorMsg Default message to display if no providers return non-null
* @return {!Promise} A promise resolved with true if an inline widget is opened or false
* when closed. Rejected if there is neither an existing widget to close nor a provider
* willing to create a widget (or if no editor is open).
Expand Down
4 changes: 2 additions & 2 deletions src/extensions/default/JavaScriptQuickEdit/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ define(function (require, exports, module) {
*
* @param hostEditor {!Editor} editor
* @param {!{line:Number, ch:Number}} pos
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you are fixing comment stuff, Number should be number.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, will fix

* @return {functionName: {string}, reason: {string}}
* @return {functionName: string, reason: string}
*/
function _getFunctionName(hostEditor, pos) {
var token = hostEditor._codeMirror.getTokenAt(pos, true);
Expand Down Expand Up @@ -109,7 +109,7 @@ define(function (require, exports, module) {
* @param {!string} functionName
* @return {?$.Promise} synchronously resolved with an InlineWidget, or
* {string} if js other than function is detected at pos, or
* null if we're not going to provide anything.
* null if we're not ready to provide anything.
*/
function _createInlineEditor(hostEditor, functionName) {
// Use Tern jump-to-definition helper, if it's available, to find InlineEditor target.
Expand Down
10 changes: 5 additions & 5 deletions src/nls/root/strings.js
Original file line number Diff line number Diff line change
Expand Up @@ -180,15 +180,15 @@ define({
"FILE_FILTER_CLIPPED_SUFFIX" : "and {0} more",

// Quick Edit
"ERROR_QUICK_EDIT_PROVIDER_NOT_FOUND" : "No Quick Edit provider found for current cursor position",
"ERROR_CSSQUICKEDIT_CLASSNOTFOUND" : "CSS Quick Edit: place cursor in class name",
"ERROR_CSSQUICKEDIT_IDNOTFOUND" : "CSS Quick Edit: place cursor in id name",
"ERROR_CSSQUICKEDIT_UNSUPPORTEDATTR" : "CSS Quick Edit: place cursor in tag name, class name, or id name",
"ERROR_QUICK_EDIT_PROVIDER_NOT_FOUND" : "No Quick Edit available for current cursor position",
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed "provider" since it's an implementation term that probably won't mean much to end users

"ERROR_CSSQUICKEDIT_CLASSNOTFOUND" : "CSS Quick Edit: incomplete class attribute",
"ERROR_CSSQUICKEDIT_IDNOTFOUND" : "CSS Quick Edit: incomplete id attribute",
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the code, these two arise when the cursor is in the attribute, but the attribute value is blank or perhaps otherwise malformed... so the original string was incorrect.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another case for the class attribute that triggers this message is: class="c1 | c2" Attribute is not blank or malformed, but cursor is not in a class name so the code fails. So, that's why "place cursor in class name" seemed appropriate.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should have a separate error for that case (though it also seems pretty unlikely to hit, since you'd have to have multiple spaces between the class names in order to hit it).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a separate error case since it looks easy. I also found #7389 in the process, so I guess it's good I wound up poking at this code :-)

"ERROR_CSSQUICKEDIT_UNSUPPORTEDATTR" : "CSS Quick Edit: place cursor in tag name, class attribute, or id attribute",
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed up usage of name vs. attribute

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we just shorten this to CSS Quick Edit: place cursor in tag name, class, or id?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, will do

"ERROR_TIMINGQUICKEDIT_INVALIDSYNTAX" : "CSS Timing Function Quick Edit: invalid syntax",
"ERROR_JSQUICKEDIT_FUNCTIONNOTFOUND" : "JS Quick Edit: place cursor in function name",

// Quick Docs
"ERROR_QUICK_DOCS_PROVIDER_NOT_FOUND" : "No Quick Docs provider found for current cursor position",
"ERROR_QUICK_DOCS_PROVIDER_NOT_FOUND" : "No Quick Docs available for current cursor position",

/**
* ProjectManager
Expand Down
9 changes: 6 additions & 3 deletions src/project/FileSyncManager.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,12 @@
* FileSyncManager is a set of utilities to help track external modifications to the files and folders
* in the currently open project.
*
* Currently, we look for external changes purely by checking file timestamps against the last-sync
* timestamp recorded on Document. Later, we will use actual native directory-watching callbacks
* instead.
* Currently, we detect external changes purely by checking file timestamps against the last-sync
* timestamp recorded on Document. Brackets triggers this check whenever an external change was detected
* by our native file watchers, and on window focus. We recheck all open Documents, but with file caching
* the timestamp check is a fast no-op for everything other than files where a watcher change was just
* notified. If watchers/caching are disabled, we'll essentially check only on window focus, and we'll hit
* the disk to check every open Document's timestamp every time.
*
* FUTURE: Whenever we have a 'project file tree model,' we should manipulate that instead of notifying
* DocumentManager directly. DocumentManager, the tree UI, etc. then all listen to that model for changes.
Expand Down