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

Quick Edit result grouping #9614

Merged
merged 8 commits into from
Oct 23, 2014
Merged

Quick Edit result grouping #9614

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3acf985
Initial implementation of Quick Edit grouping the results list by fil…
peterflynn Sep 24, 2014
54c98fb
Quick Edit grouping fixes:
peterflynn Sep 24, 2014
85fa143
Merge remote-tracking branch 'origin/master' into pflynn/quick-edit-g…
peterflynn Oct 20, 2014
e428ea9
- Consistently sort files: CSS below preprocessors, then sorted by pa…
peterflynn Oct 21, 2014
8687f61
- Simplify code that renders the rule list (esp _createListItem(), which
peterflynn Oct 21, 2014
2e4402e
Merge remote-tracking branch 'origin/master' into pflynn/quick-edit-g…
peterflynn Oct 22, 2014
071cd33
- Quick Edit: if user expands collapsed section and nothing selected …
peterflynn Oct 22, 2014
f60710e
Quick Edit: don't show filename on each result item anymore, since it's
peterflynn Oct 23, 2014
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
86 changes: 37 additions & 49 deletions src/editor/CSSInlineEditor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@ define(function (require, exports, module) {
DocumentManager = require("document/DocumentManager"),
EditorManager = require("editor/EditorManager"),
Editor = require("editor/Editor").Editor,
LanguageManager = require("language/LanguageManager"),
ProjectManager = require("project/ProjectManager"),
FileUtils = require("file/FileUtils"),
HTMLUtils = require("language/HTMLUtils"),
MultiRangeInlineEditor = require("editor/MultiRangeInlineEditor"),
Strings = require("strings"),
Expand Down Expand Up @@ -146,7 +148,7 @@ define(function (require, exports, module) {
/** Item renderer for stylesheet-picker dropdown */
function _stylesheetListRenderer(item) {
var html = "<span class='stylesheet-name'>" + _.escape(item.name);
if (item.subDirStr.length) {
if (item.subDirStr) {
html += "<span class='stylesheet-dir'> — " + _.escape(item.subDirStr) + "</span>";
}
html += "</span>";
Expand Down Expand Up @@ -240,65 +242,50 @@ define(function (require, exports, module) {

/**
* @private
* Sort fileInfo objects by name then sub-directory
* Sort files with LESS/SCSS above CSS, and then within each grouping sort by path & filename
* (the same order we use for Find in Files)
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

No @return annotation

function _sortFileInfos(a, b) {
var nameComparison = a.name.localeCompare(b.name);
if (nameComparison !== 0) {
return nameComparison;
function _fileComparator(a, b) {
var aIsCSS = LanguageManager.getLanguageForPath(a.fullPath).getId() === "css",
bIsCSS = LanguageManager.getLanguageForPath(b.fullPath).getId() === "css";
if (aIsCSS && !bIsCSS) {
return 1;
} else if (!aIsCSS && bIsCSS) {
return -1;
} else {
return FileUtils.comparePaths(a.fullPath, b.fullPath);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, the next version of JSLint flags an error when there is no default return statement (which I generally agree with), so I think this should be changed to:

            if (aIsCSS && !bIsCSS) {
                return 1;
            } else if (!aIsCSS && bIsCSS) {
                return -1;
            }
            return FileUtils.comparePaths(a.fullPath, b.fullPath);

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that's one of the reasons we chose not to update to that version :-) Personally, I prefer the structure the way it is so all return cases have matching indent...

return a.subDirStr.localeCompare(b.subDirStr);
}

/**
* @private
* Prepare file list for display
*/
function _prepFileList(fileInfos) {
var i, j, firstDupeIndex,
displayPaths = [],
dupeList = [];
function _prepFileList(files) {
// First, sort list (the same ordering we use for the results list)
files.sort(_fileComparator);

// Add subdir field to each entry
fileInfos.forEach(function (fileInfo) {
fileInfo.subDirStr = "";
});

// Add directory path to files with the same name so they can be
// distinguished in list. Start with list sorted by name.
fileInfos.sort(_sortFileInfos);

// For identical names, add a subdir
for (i = 1; i < fileInfos.length; i++) {
if (_sortFileInfos(fileInfos[i - 1], fileInfos[i]) === 0) {
// Duplicates found
firstDupeIndex = i - 1;
dupeList.push(fileInfos[i - 1]);
dupeList.push(fileInfos[i]);

// Lookahead for more dupes
while (++i < fileInfos.length &&
_sortFileInfos(dupeList[0], fileInfos[i]) === 0) {
dupeList.push(fileInfos[i]);
}

// Get minimum subdir to make each unique
displayPaths = ViewUtils.getDirNamesForDuplicateFiles(dupeList);

// Add a subdir to each dupe entry
for (j = 0; j < displayPaths.length; j++) {
fileInfos[firstDupeIndex + j].subDirStr = displayPaths[j];
}

// Release memory
dupeList = [];
// Find any files that share the same name (with different path)
var fileNames = {};
files.forEach(function (file) {
if (!fileNames[file.name]) {
fileNames[file.name] = [];
}
}
fileNames[file.name].push(file);
});

// Sort by name again, so paths are sorted
fileInfos.sort(_sortFileInfos);
// For any duplicate filenames, set subDirStr to a path snippet the helps
// the user distinguish each file in the list.
_.forEach(fileNames, function (files) {
if (files.length > 1) {
var displayPaths = ViewUtils.getDirNamesForDuplicateFiles(files);
files.forEach(function (file, i) {
file.subDirStr = displayPaths[i];
});
}
});

return fileInfos;
return files;
}

function _onHostEditorScroll() {
Expand All @@ -309,7 +296,8 @@ define(function (require, exports, module) {
.done(function (rules) {
var inlineEditorDeferred = new $.Deferred();
cssInlineEditor = new MultiRangeInlineEditor.MultiRangeInlineEditor(CSSUtils.consolidateRules(rules),
_getNoRulesMsg, CSSUtils.getRangeSelectors);
_getNoRulesMsg, CSSUtils.getRangeSelectors,
_fileComparator);
cssInlineEditor.load(hostEditor);
cssInlineEditor.$htmlContent
.on("focusin", _updateCommands)
Expand Down
2 changes: 1 addition & 1 deletion src/editor/Editor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1793,7 +1793,7 @@ define(function (require, exports, module) {
* Sets the height of an inline widget in this editor.
* @param {!InlineWidget} inlineWidget The widget whose height should be set.
* @param {!number} height The height of the widget.
* @param {boolean} ensureVisible Whether to scroll the entire widget into view.
* @param {?boolean} ensureVisible Whether to scroll the entire widget into view. Default false.
Copy link
Contributor

Choose a reason for hiding this comment

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

Type annotation with default value should be {boolean=}

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a subtle difference -- = means an argument can be omitted and any arguments to the right slide over one space. Until there are arguments to the right and we write code to support that behavior, I'd rather leave it as ? to indicate it's "merely" nullable.

Copy link
Contributor

Choose a reason for hiding this comment

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

{?type} can by null (which implies it will be handled in some way). {type=} mean it can be omitted and give a default value (which basically means "handled in some way"). To me, the difference is that the default value is specified with {type=} form.

= means an argument can be omitted and any arguments to the right slide over one space.

That is incorrect. According to this page:

"Indicates that an argument in a function type is optional. An optional argument can be omitted from the function call. An optional argument cannot precede a non-optional argument in the argument list."

(highlighting by me)

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting. There's some ambiguity there because it doesn't say whether it's talking about formal vs. actual parameters, but I think your interpretation is most likely correct. That disagrees with how some of our existing code uses (e.g. all of FileSystem), but since that usage now seems incorrect I'll change it to = here.

*/
Editor.prototype.setInlineWidgetHeight = function (inlineWidget, height, ensureVisible) {
var self = this,
Expand Down
2 changes: 1 addition & 1 deletion src/editor/InlineTextEditor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ define(function (require, exports, module) {
/**
* Update the inline editor's height when the number of lines change. The
* base implementation of this method does nothing.
* @param {boolean} force the editor to resize
* @param {boolean} force the editor to resize (ignored)
Copy link
Contributor

Choose a reason for hiding this comment

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

Method does nothing at all -- why did you feel it was necessary to indicate that the parameter is ignored?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I should just finish what I started and entirely remove this completely unused arg...

*/
InlineTextEditor.prototype.sizeInlineWidgetToContents = function (force) {
// brackets_codemirror_overrides.css adds height:auto to CodeMirror
Expand Down
Loading