Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[CLOSED] Find in Files Improvements (Part 4: Sorting) #5987

Open
core-ai-bot opened this issue Aug 30, 2021 · 23 comments
Open

[CLOSED] Find in Files Improvements (Part 4: Sorting) #5987

core-ai-bot opened this issue Aug 30, 2021 · 23 comments

Comments

@core-ai-bot
Copy link
Member

Issue by TomMalbran
Sunday Jan 19, 2014 at 02:27 GMT
Originally opened as adobe/brackets#6585

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.

TomMalbran included the following code: https://github.com/adobe/brackets/pull/6585/commits
@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Sunday Feb 23, 2014 at 21:12 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by JeffryBooher
Monday Feb 24, 2014 at 02:49 GMT

Apologies@TomMalbran this must have gotten lost in the shuffle. I'll review it and pull it in sometime this week.

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Monday Feb 24, 2014 at 02:58 GMT

Great. Thanks

@core-ai-bot
Copy link
Member Author

Comment by JeffryBooher
Wednesday Feb 26, 2014 at 22:28 GMT

@TomMalbran done with initial review. Just a few minor code cleanup items to work on before I test it.

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Thursday Feb 27, 2014 at 00:12 GMT

@JeffryBooher Thanks for the review. Changed pushed.

@core-ai-bot
Copy link
Member Author

Comment by JeffryBooher
Friday Feb 28, 2014 at 18:56 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Friday Feb 28, 2014 at 19:37 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by redmunds
Friday Feb 28, 2014 at 20:09 GMT

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.

@core-ai-bot
Copy link
Member Author

Comment by JeffryBooher
Friday Feb 28, 2014 at 23:04 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Friday Feb 28, 2014 at 23:07 GMT

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.

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Friday Feb 28, 2014 at 23:26 GMT

@JeffryBooher Just merged with master. There is no code in this PR that changes how the results are displayed, besides sorting the files.

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Saturday Mar 01, 2014 at 01:36 GMT

@JeffryBooher What folder is the root of your project when you're testing? src, or the parent folders of src, or something else? It's odd because the paths aren't absolute in either case, so it sounds like they're both being turned into relative paths, just relative to different things. And if not the project root, I dunno what the reference point would be...

The code that makes it relative is on line 372 -- the call to ProjectManager.makeProjectRelativeIfPossible() -- which is totally unchanged in this patch...

@core-ai-bot
Copy link
Member Author

Comment by JeffryBooher
Monday Mar 03, 2014 at 21:30 GMT

hey@TomMalbran is there anyway that we can add a unit test for this to make sure the list is sorted?

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Monday Mar 03, 2014 at 21:53 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by JeffryBooher
Tuesday Mar 04, 2014 at 00:08 GMT

got it. merging...

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Tuesday Mar 04, 2014 at 00:24 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by larz0
Tuesday Mar 04, 2014 at 00:36 GMT

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.

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Tuesday Mar 04, 2014 at 00:38 GMT

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.

@core-ai-bot
Copy link
Member Author

Comment by larz0
Tuesday Mar 04, 2014 at 00:42 GMT

Ahh ok cool.

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Tuesday Mar 04, 2014 at 01:04 GMT

@larz0 So, should the selected file on the initial search be first or not?

@core-ai-bot
Copy link
Member Author

Comment by larz0
Tuesday Mar 04, 2014 at 01:09 GMT

Yes that makes sense.

@core-ai-bot
Copy link
Member Author

Comment by JeffryBooher
Tuesday Mar 04, 2014 at 04:30 GMT

@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

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Tuesday Mar 04, 2014 at 04:32 GMT

No problem. Will submit a PR soon to fix that issue. Testing it right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@core-ai-bot