Quick Edit result grouping

[peterflynn]

• Group Quick Edit results into collapsible sections by file. Collapsed sections are remembered per project as view state.
  • Also simplifies the rule list UI code in MultiRangeInlineEditor: we now simply re-render the whole list when items are added/removed, instead of trying to update it incrementally. Also removes unneeded async code in _updateSelectedMarker(), unused arg in sizeInlineWidgetToContents(), and a few other smaller things.
• The initially selected result is now the first non-collapsed result; if all results are collapsed, an explanation is shown in the editor area. One exception: if there's only one result it's always selected even if collapsed, since the rule list is hidden when there's only one result, leaving no way to expand the collapsed section.
• Consistently sort Quick Edit results (previously, their order was nondeterministic -- the cause of some intermittent unit test failures!). By default, MRIE sorts in the same order as Find in Files. In CSS Quick Edit, there's extra logic to bubble up all LESS/SCSS files to the top of the list.
  • This PR changes the Find in Files sort order - on master, files with deeper nesting are listed before shallower files with the same path prefix (e.g. /project/folder/subfolder/foo.js comes before /project/folder/bar.js)... and thus any files at the project root are always listed last. This PR reverses that: shallower items are listed higher, and thus any files at the project root are always listed first.
  • This also changes the sort order of the New Rule dropdown to be consistent with the Quick Edit order. Previously, files were sorted primarily on filename (files with the same name in very different folders appeared next to each other in the list). Now it uses the 'standard sort order' shared with Find in Files, sorting primarily on folder (files in the same folder appear next to each other in the list, regardless of filename). Duplicate filenames still display extra path snippets to disambiguate them in the dropdown.

[peterflynn]

Unit tests added -- now all the code is up & ready for review

[peterflynn]

@larz0 I haven't done a ton of polish on the section header appearance. Here's what it looks like currently:

Two changes we might want to consider:

• No longer list the filename next to each result, since it's redundant with the header. My only hesitation is that it may make it less obvious what the line number part of the label is, since the ": nn" format is almost exclusively used as a suffix on filenames... Maybe something like: a.nav-button (:28) would be clear enough, though?
• Make the result count in each section header normal weight instead of semibold like the filename part of the header.

[redmunds]

What about a.nav-button (line 28) ?

[redmunds]

What is the default file processing/display order? Now that styles are grouped by file, it seems like files should have a predictable order (maybe even same sort order as file tree?).

For example, I have a version of the citrus cafe site that I converted to LESS. The css folder has desktop.less, phone.less, and tablet.less files. I have an extension that auto-compiles each .less file into a .css stylesheet on save. I expected the files would be ordered by name, but they're in this order:

• desktop.less
• phone.less
• tablet.less
• desktop.css
• phone.css
• tablet.css

UPDATE: I see that you are explicitly sorting .css files last -- why is that? I'd much rather have files grouped by name (and then .css last if you think that's important):

• desktop.less
• desktop.css
• phone.less
• phone.css
• tablet.less
• tablet.css

[redmunds]

Since file list takes up more space vertically, what do you think about supporting Ctrl-click on arrows to collapse-all/expand-all to quickly close files you are not interested in?

[redmunds]

If all files are collapsed so no matches are displayed and I expand a file, I would expect the first rule in file to automatically become selected.

[redmunds]

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);

[peterflynn]

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...

[larz0]

@peterflynn looks good here, just need to tweak the padding a tiny bit:

.related-container .related ul {
…
padding: 10px 0px 10px 5px;
…
}

We should also make the changes you suggested.

[peterflynn]

@redmunds Re the sort order, this is described at the top: we now use the same sort order as Find in Files. In general items are sorted by folder, then by name within folders. Folder relationships seem more important in telling us which items are related than naming alphabetization, IMHO. I talked to @njx about that and he seemed to think it was a good change to. Do you feel strongly it should go back to the old order where we sort by name and ignore the folder structure?

We also specifically push CSS files to the bottom of the list. That's definitely by design -- in a project that contains preprocessor files, any CSS files are very likely to be generated code that you never edit directly. So we want to push those down to the bottom consistently.

[peterflynn]

We could also list the full project-relative path of each item in the New Rule dropdown -- that might help make the sort ordering more clear.

[redmunds]

@peterflynn I agree with grouping files by folder and sorting .css files last. Current sorting within a folder is file type first then file name.

I'd prefer sorting within a folder to be by file name first then file type. This is to group preprocessor files together with their generated .css files (when they're in the same folder). Then I can quickly see which .css files are "native" (better term here?) versus which are generated from preprocessor files, and also which preprocessor files are missing their generated .css files.

[RaymondLim]

Hiding selection indicator does not reset or remove the current selection. So when you navigate with Alt-Up/Down shortcuts you may be switching selection within the collapsed file. Not sure what is the better behavior, but switching selection without showing the selection in the list is not a good experience though.

[peterflynn]

I'll change it so that:

• Alt-Up/Down when selection is inside a collapsed group causes the group to expand again.
• Alt-Up/Down arrow otherwise skips over collapsed groups.

Sound good?

[RaymondLim]

Yep, make sense.

[peterflynn]

@redmunds Currently, it sorts CSS files completely to the bottom regardless of folder -- so the order is first by type (preprocessor vs. CSS), then by folder, then by filename. I feel pretty strongly that we don't want to mix CSS files into the list of LESS/SCSS files. If you have both in your project, you almost certainly want to ignore all of the CSS files. Grouping them all at the bottom makes it easier to ignore them than if they're interleaves all throughout the other results that you do care about.

[peterflynn]

Re the Ctrl-click: I like that idea, but I'm a little worried about getting this landed by tomorrow AM as is, given all the other changes... mind if we leave that as a separate PR? (Which potentially might not land until 1.1)

[redmunds]

@peterflynn

Currently, it sorts CSS files completely to the bottom regardless of folder

I don't remember that being discussed. This assumes that any project that uses a CSS Preprocessor uses it for 100% of the stylesheets. I don't think that's a safe assumption for larger sites with many contributors, or sites that use CSS 3rd party libraries or widgets.

But it's getting down to the wire, so unless I hear anyone else pipe in, then I guess we'll go with this for now.

[redmunds]

@peterflynn I can wait for the Ctrl-click functionality -- just thought I'd throw it out there.

[peterflynn]

@redmunds I pushed fixes for everything noncontroversial above (see commit for full list).

Except that I haven't removed the filename from the individual result items yet -- still need to experiment with what looks best. It will require updating a lot of unit tests, so I don't want to commit that change until we've settled on what looks best.

[peterflynn]

Here are some options for the rule list line numbers with filename removed:

I think I like the first three the best. My only hesitation with the first one is that it pushes the rule names (the thing the user probably cares about the most) a little more to the right, resulting in more of the names being clipped.

CC @larz0 @placegraphichere

[larz0]

@peterflynn @placegraphichere I like:

because it takes up less room and can only mean one thing. Real-estate in this sidebar is quiet precious.

[peterflynn]

@larz0 I find that one a little hard to parse visually, especially since CSS selectors sometimes have a ":" near the end. Is there anything more we could do to separate it visually from the text to the left? Fixed-pt font? Semi-bold? More padding...?

[peterflynn]

I added some padding and that seems to improve readability:

@larz0 / @placegraphichere Any objections to that?

[redmunds]

Is line number even necessary? When you click on the selector you see the line number in the inline editor.

[peterflynn]

Didn't hear anything, so I went ahead and pushed up the implementation in my last screenshot above. I've made the unit tests more robust though, so if we want to tweak this further it will be a much smaller change.

[peterflynn]

@redmunds I thought about ditching them entirely, but it seemed like there's some value: (a) to see which rules might be close together vs. in widely separated parts of a file, or (b) in LESS/SCSS files to see if one result might be nested inside another (though you can't be 100% certain since we don't show the end line number).

Anyway, the line number is something we've always had in the UI so far, and unlike the filename we haven't done anything in this PR to make it redundant... so I'm inclined to not rock the boat and just keep it.

[larz0]

@peterflynn yeah that's looks better. Good call!

[redmunds]

Merging.

[peterflynn]

Thanks @redmunds! Re the sorting thing: I mentioned it briefly before MAX but there wasn't a long discussion. I'm open to tweaking the sort order if we get feedback on it after 1.0 ships.