Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Quick Open enhancements & API cleanup #1565

Merged
merged 4 commits into from
Sep 10, 2012
Merged

Quick Open enhancements & API cleanup #1565

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ba615e9
Quick Open enhancements:
peterflynn May 30, 2012
bc4b042
Merge remote-tracking branch 'origin/master' into pflynn/quick-open-sort
peterflynn Sep 5, 2012
cbaf107
Further Quick Open improvements:
peterflynn Sep 5, 2012
3b3a37c
Stylistic fixes for code review
peterflynn Sep 9, 2012
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
35 changes: 12 additions & 23 deletions src/extensions/default/QuickOpenCSS/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,30 +81,19 @@ define(function (require, exports, module) {

/**
* @param {string} query what the user is searching for
* @returns {Array.<string>} sorted and filtered results that match the query
* @returns {Array.<SearchResult>} sorted and filtered results that match the query
*/
function search(query) {
createSelectorList();

query = query.slice(query.indexOf("@") + 1, query.length);

// Filter and rank how good each match is
var filteredList = $.map(selectorList, function (itemInfo) {

var selector = itemInfo.selector;

if (selector.toLowerCase().indexOf(query.toLowerCase()) !== -1) {
return selector;
}
}).sort(function (a, b) {
a = a.toLowerCase();
b = b.toLowerCase();
if (a > b) {
return -1;
} else if (a < b) {
return 1;
} else {
return 0;
}
return QuickOpen.stringMatch(itemInfo.selector, query);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if it's worth handling, but CSS Selectors have a mix of case sensitivity (id's and classes) and insensitivity (everything else), and this method ignores case.

});

// Sort based on ranking & basic alphabetical order
QuickOpen.basicMatchSort(filteredList);

return filteredList;
}
Expand All @@ -124,20 +113,20 @@ define(function (require, exports, module) {

/**
* Select the selected item in the current document
* @param {HTMLLIElement} selectedItem
* @param {?SearchResult} selectedItem
*/
function itemFocus(selectedItem) {
var selectorInfo = getLocationFromSelectorName($(selectedItem).text());
if (!selectedItem) {
return;
}
var selectorInfo = getLocationFromSelectorName(selectedItem.label);
if (selectorInfo) {
var from = {line: selectorInfo.selectorStartLine, ch: selectorInfo.selectorStartChar};
var to = {line: selectorInfo.selectorStartLine, ch: selectorInfo.selectorEndChar};
EditorManager.getCurrentFullEditor().setSelection(from, to);
}
}

/**
* TODO: selectedItem is currently a <LI> item from smart auto complete container. It should just be data
*/
function itemSelect(selectedItem) {
itemFocus(selectedItem);
}
Expand Down
27 changes: 14 additions & 13 deletions src/extensions/default/QuickOpenHTML/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,7 @@ define(function (require, exports, module) {
var docText = doc.getText();
var lines = docText.split("\n");


var regex = new RegExp(/\s*id\s*?=\s*?["'](.*?)["']/gi);
var regex = new RegExp(/\s+id\s*?=\s*?["'](.*?)["']/gi);
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch! Nit: there are multiple empty lines before this one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

var id, chFrom, chTo, i, line;
for (i = 0; i < lines.length; i++) {
line = lines[i];
Expand Down Expand Up @@ -113,17 +112,19 @@ define(function (require, exports, module) {

/**
* @param {string} query what the user is searching for
* @returns {Array.<string>} sorted and filtered results that match the query
* @returns {Array.<SearchResult>} sorted and filtered results that match the query
*/
function search(query) {
createIDList();

query = query.slice(query.indexOf("@") + 1, query.length);

// Filter and rank how good each match is
var filteredList = $.map(idList, function (itemInfo) {
if (itemInfo.id.toLowerCase().indexOf(query.toLowerCase()) !== -1) {
return itemInfo.id;
}
}).sort();
return QuickOpen.stringMatch(itemInfo.id, query);
});

// Sort based on ranking & basic alphabetical order
QuickOpen.basicMatchSort(filteredList);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is ignores case when sorting, and id's are case sensitive.


return filteredList;
}
Expand All @@ -144,20 +145,20 @@ define(function (require, exports, module) {

/**
* Select the selected item in the current document
* @param {HTMLLIElement} selectedItem
* @param {?SearchResult} selectedItem
*/
function itemFocus(selectedItem) {
var fileLocation = getLocationFromID($(selectedItem).text());
if (!selectedItem) {
return;
}
var fileLocation = getLocationFromID(selectedItem.label);
if (fileLocation) {
var from = {line: fileLocation.line, ch: fileLocation.chFrom};
var to = {line: fileLocation.line, ch: fileLocation.chTo};
EditorManager.getCurrentFullEditor().setSelection(from, to);
}
}

/**
* TODO: selectedItem is currently a <LI> item from smart auto complete container. It should just be data
*/
function itemSelect(selectedItem) {
itemFocus(selectedItem);
}
Expand Down
34 changes: 12 additions & 22 deletions src/extensions/default/QuickOpenJavaScript/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,29 +110,19 @@ define(function (require, exports, module) {

/**
* @param {string} query what the user is searching for
* @returns {Array.<string>} sorted and filtered results that match the query
* @returns {Array.<SearchResult>} sorted and filtered results that match the query
*/
function search(query) {
createFunctionList();

query = query.slice(query.indexOf("@") + 1, query.length);

// Filter and rank how good each match is
var filteredList = $.map(functionList, function (itemInfo) {

var functionName = itemInfo.functionName;
if (functionName.toLowerCase().indexOf(query.toLowerCase()) !== -1) {
return functionName;
}
}).sort(function (a, b) {
a = a.toLowerCase();
b = b.toLowerCase();
if (a > b) {
return -1;
} else if (a < b) {
return 1;
} else {
return 0;
}
return QuickOpen.stringMatch(itemInfo.functionName, query);
Copy link
Contributor

Choose a reason for hiding this comment

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

JavaScript function names are case sensitive. Bonus: case sensitive string matching is faster!

Copy link
Contributor

Choose a reason for hiding this comment

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

On second thought, since this is "quick" open it should probably ignore case when searching, but respect case when sorting.

For example, in the case where there's multiple matches that are all lower-case and also multiple matches that are mixed-case, I think that it would make it easier to differentiate between the choices if they were separated and not mixed together.

Copy link
Member Author

Choose a reason for hiding this comment

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

Re the various comments on case-sensitivity... The existing behavior of Quick Open is to do everything case-insensitively, and since it's been around a while I'd rather wait and not muck with it in this patch.

Although my personal feeling is that we should stay case-insensitive even in the long run (seems more consistent with other tools & other Brackets features), it seems reasonable to raise this with the broader team. We could always change it down the road if I'm in the minority. Does that sound ok to you?

Copy link
Contributor

Choose a reason for hiding this comment

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

OK by me.

});

// Sort based on ranking & basic alphabetical order
QuickOpen.basicMatchSort(filteredList);

return filteredList;
}
Expand All @@ -153,10 +143,13 @@ define(function (require, exports, module) {

/**
* Select the selected item in the current document
* @param {HTMLLIElement} selectedItem
* @param {?SearchResult} selectedItem
*/
function itemFocus(selectedItem) {
var fileLocation = getLocationFromFunctionName($(selectedItem).text());
if (!selectedItem) {
return;
}
var fileLocation = getLocationFromFunctionName(selectedItem.label);

if (fileLocation) {
var from = {line: fileLocation.line, ch: fileLocation.chFrom};
Expand All @@ -165,9 +158,6 @@ define(function (require, exports, module) {
}
}

/**
* TODO: selectedItem is currently a <LI> item from smart auto complete container. It should just be data
*/
function itemSelect(selectedItem) {
itemFocus(selectedItem);
}
Expand Down
Loading