Find in Files Improvements (Part 4: Sorting) #6585
Conversation
ghost
assigned 
|
@JeffryBooher Any updates here? I would like them to be merged for the next release so I can continue with the next one. This also fixes a bug where the results can change order while updating the files. |
| return 1; | ||
| } else if (index < folders1 && index >= folders2) { | ||
| return -1; | ||
| } else { |
There was a problem hiding this comment.
You don't need that else.
There was a problem hiding this comment.
+1, execution will never enter this branch
There was a problem hiding this comment.
you can probably simplify this with return (index >= folders1 && index < folder2) ? 1 : -1
There was a problem hiding this comment.
It gets this branch when index >= folders1 && index >= folders2. What we can do is:
if (index < folders1 && index < folders2) {
return entryName1.toLocaleLowerCase().localeCompare(entryName2.toLocaleLowerCase());
} else if (index >= folders1 && index >= folders2) {
return FileUtils.compareFilenames(entryName1, entryName2);
}
return (index >= folders1 && index < folder2) ? 1 : -1;
|
Apologies @TomMalbran this must have gotten lost in the shuffle. I'll review it and pull it in sometime this week. |
|
Great. Thanks |
| @@ -236,7 +240,7 @@ define(function (require, exports, module) { | |||
| * @param {string} fullPath | |||
| * @param {string} contents | |||
| * @param {RegExp} queryExpr | |||
| * @return {boolean} True iff matches were added to the search results | |||
There was a problem hiding this comment.
i don't think the iff needed to be changed here. This is scientific notation for if-and-only-if
There was a problem hiding this comment.
Oh, I see. We use the same notation in Spanish as sii but I never saw it English before, so I thought it was an error.
|
@TomMalbran done with initial review. Just a few minor code cleanup items to work on before I test it. |
|
@JeffryBooher Thanks for the review. Changed pushed. |
|
@TomMalbran why do the path headers no longer have the full path? It's relative to the project root now. If I compare Sprint 36 to this PR, FiF says "css.json - src/extensions/default/WebPlatformDocs/" vs. "extensions/default/WebPlatformDocs". I think it's ok but I didn't see a mandate or a note so I'm curious if it was accidental or did you intend for that to happen. I like that it saves space -- especially useful for bigger projects with a lot of subfolders but we should be careful that this was intentional and wasn't broken by accident. |
|
@JeffryBooher Afaik, the headers in the search results have always shown project-relative paths. That's how it works for me on Sprint 36 master currently. Do you have local changes on your machine? Or might you have had a different project root open in the two cases you tested? (Neither path example above looks like a full, absolute path to me). |
|
A case where full paths maybe used to be shown is when you have a file open that is outside your current project since Working Set files are also searched. I tested in master and I'm not seeing any path for that case -- seems like full path should be shown. |
|
@peterflynn the only thing i'm doing is switching between master and pr/6585. There may be some changes in master that are not in pr/6585, however. But the project is basically the brackets source tree. When I get results for master it says "src/..." but for pr/6585 the headers start with the folders below "src" so "thirdparty/jquery/..." @TomMalbran you might want to merge master into this branch as I always need to rejigger the submodules afterwards but primarily so we can see what's different between your branch and master. |
|
Sure will merge with master and check that. This PR is a bit old, but since it can still be merged I haven't merged it with master. |
|
@JeffryBooher Just merged with master. There is no code in this PR that changes how the results are displayed, besides sorting the files. |
|
@JeffryBooher What folder is the root of your project when you're testing? The code that makes it relative is on line 372 -- the call to |
|
hey @TomMalbran is there anyway that we can add a unit test for this to make sure the list is sorted? |
|
@JeffryBooher We currently don't have any FindInFiles test, so not sure how to make them. I still found a small bug. Since this places the currently selected file, the files order changes when this file the current file changes, which happens when you switch files and start editing which changes the results. Not sure if I should remove this, or save the current file at the start of the search and always use that one. |
|
got it. merging... |
Find in Files Improvements (Part 4: Sorting)
|
@JeffryBooher Cool, but what about the bug I mentioned? It kind of makes the file you are editing the first file, even after switching files first, which doesn't work nice with the pagination. The original idea was to make the currently selected file on the initial search the first one, so that you can see the results from that file for issue #995. But maybe that is not how we should fix that issue. So not sure if I should keep how sorting works but saving the selected file on the initial search or remove that bit of code. cc @larz0. Will fix this in a new PR, once I know which way to go. |
|
We could sort them alphabetically or we could sort by file type first then alphabetically. I think the second option is better because hen I start find-in-files when a JS file is opened I'd like to see JS files in the results first, ahead of the matches in say CSS. |
|
The files are sorted in the same way as they are in the files tree, if you open it completely. The only difference is that the selected file goes first. |
|
Ahh ok cool. |
|
@larz0 So, should the selected file on the initial search be first or not? |
|
Yes that makes sense. |
|
@TomMalbran sorry if I pulled the trigger too soon. I figured that we would follow up this issue in another PR anyway but it sounds like you have worked it out with @larz0 |
|
No problem. Will submit a PR soon to fix that issue. Testing it right now. |
The Find in Files Improvements are back with part 4.
With this PR the files in the Find in Files results are sorted before showing the results. It will always show first the selected file and the rest after sorted similar to how they are displayed in the project tree in Windows.
This fixes issue #4933.