Add a 'find in files' quick-menu #2315

TalAmuyal · 2018-06-13T22:21:59Z

Disclaimer:

I (shamelessly) copied the code from the sidebar's search and injected it into a copy of QuickOpen.
Personally, I find it more useful that way.

The "new" feature is called QuickFind :P

codecov · 2018-06-14T17:21:53Z

Codecov Report

Merging #2315 into master will decrease coverage by 0.04%.
The diff coverage is 47.87%.

@@            Coverage Diff             @@
##           master    #2315      +/-   ##
==========================================
- Coverage   42.67%   42.62%   -0.05%     
==========================================
  Files         325      332       +7     
  Lines       13131    13212      +81     
  Branches     1728     1736       +8     
==========================================
+ Hits         5603     5632      +29     
- Misses       7249     7302      +53     
+ Partials      279      278       -1

Impacted Files	Coverage Δ
browser/src/Services/Search/FinderProcess.ts	`40% <ø> (ø)`
...wser/src/Services/Search/Scorer/QuickOpenScorer.ts	`62.61% <ø> (ø)`
...r/src/Services/Search/Scorer/OniQuickOpenScorer.ts	`66.66% <ø> (ø)`
browser/src/Services/Search/Scorer/filters.ts	`74.76% <ø> (ø)`
browser/src/Services/Menu/PinnedIconView.tsx	`80% <ø> (ø)`
browser/src/Services/Search/Scorer/strings.ts	`51.72% <ø> (ø)`
browser/src/Services/Search/Scorer/Utilities.ts	`94.11% <ø> (ø)`
browser/src/Services/Search/Scorer/Comparers.ts	`6.12% <ø> (ø)`
...src/Services/Configuration/DefaultConfiguration.ts	`87.5% <ø> (ø)`	⬆️
browser/src/App.ts	`13.43% <0%> (+0.15%)`	⬆️
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97899e1...8d69256. Read the comment docs.

CrossR · 2018-06-16T10:32:05Z

Gave it a test and works fine for me!

One thing that struck me was if the search is in a menu bar, does it not make sense for the results to be in there as well? Though that could potentially warrant an issue to get some comments. I know there was talks of a combo search box.

Ie Ctrl-p but if you prefix the search with ? its file wise, > is commands, nothing is fuzzy finder etcetc.

akinsho · 2018-06-17T13:03:11Z

I haven't had the time to try it myself but I agree with @CrossR re. the results I think one of my main blockers for using the search feature is that the results get placed in another window I have to navigate to would be nice to have an FZF like Rg command that the results were what the menu is poplulated with and you can just select the one you want

TalAmuyal · 2018-06-18T14:48:33Z

How about this: The results will appear in the drop down box of the menu (like c-p). Enter will set the results to quick-fix. C-{Enter/v/h} will act like in c-p menu.

…

On Sun, Jun 17, 2018, 16:03 Akin ***@***.***> wrote: I haven't had the time to try it myself but I agree with @CrossR <https://github.com/CrossR> re. the results I think one of my main blockers for using the search feature is that the results get placed in another window I have to navigate to would be nice to have an FZF like Rg command that the results were what the menu is poplulated with and you can just select the one you want — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#2315 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE3IjR0A2nd75pXlhwi2fcZYpOZBC2Xjks5t9lOUgaJpZM4UnB0D> .

akinsho · 2018-06-18T21:12:59Z

@TalAmuyal that sounds good think its a good idea to have a way of saving the results anyway, re. Enter as saving to the qf menu (my own 2 cents here) I think the definitive action should be opening the result and the optional action should be opening in the qf e.g. c-enter, my thinking being if you find what you want in the menu then hitting enter and finding that it opens but also creates another window might not be the desired effect going off of other editor experiences I think its more likely that user would want to make a conscious effort to keep the results (aka hit a special keybinding) and would probably default expect that enter just opened the result

TalAmuyal · 2018-06-30T09:55:01Z

Note: I pulled out the whole QuickOpen folder as a stand-alone plugin and merged the find-in-file functionality as another use-case.

The code is refactored since it was a bit tangled and was hard for me to just inject the additional logic.
I tried not to change the previous business logic.

CrossR · 2018-06-30T11:31:37Z

extensions/oni-plugin-quickopen/src/QuickOpen.ts

+    }
+
+    public async searchFileByContent() {
+        const filterName = "none"


Should this be a config option?

My first instinct was to try and use smart search (ie case insenstive unless you use uppercase characters), which then failed.

This "none" is intentional.
This filter field is a second filtering done after rg.
Since rg can accept a regex, a search string like ab.*cd will be computed as rg ab.*cd | egrep "ab\.\*cd" which will very likely cause similar searches the yield no results.

However:

Found the reason that smart-case didn't work and fixed it.

Added a TODO to use a filter for highlighting w/ support for regex.

I tried the regexFilter for the menu, but it throws some exceptions.
It might be that a slight update/variation can fix it. (But I prefer to do less changes that will keep this PR further for merging)

(Will push soon)

P.S: fixed the latency/hanging issue.

CrossR · 2018-06-30T11:50:01Z

Couple of general thoughts:

When selecting something using the quickOpen.searchFileByContent, I would expect the action of selecting an entry to move me to that line, rather than that top of that file.

For larger Repos, the quickOpen.searchFileByContent, seems to stop working?
That could just be my VM choking on larger repos (I tested the Oni + VSCode repos), so would be useful for someone else to test too.

Similarly, swapping between big repos seems to cause issues where the previous repos files are still shown. I think the process for ripgrep may not be being closed or something, since I saw the menu fighting between repos and swapping back and forth between results from two different repos. I'm not too sure what the fix here is, but I did get a "Stopped before out-of-memory crash" in the debugger for this line:

const menuItems = result.items.map(e => e.toMenuItem(this._oni, this._seenItems))

So perhaps that is the line taking up most of the time? At the point of the crash I had just short of 4GB used, so something must be up. Also I had duplicated items somehow:

When I was testing on smaller repos, worked as expected and looks good tho! Some of the memory stuff may just be us not controlling ripgrep correctly or its results, since it should be fine.

TalAmuyal · 2018-06-30T19:22:22Z

@CrossR, thanks for the fast review!

Regarding the performance issue, it should be better now and I have an idea for another improvement, though I prefer to have it in a future PR since it requires another API change.

Regarding opening in the right place - fixed! :P

Also added tests :)

TalAmuyal · 2018-07-04T19:39:38Z

@CrossR, @bryphe,

I've added some tests and made them pass locally but on some of the servers it just fails.
Specifically it fails on MacOS & Windows, though it passes on the Linux server and on my MacOS laptop.

For now the tests I've added are commented out.
Could we pull this in that way?

CrossR · 2018-07-04T19:51:44Z

I can give the tests a run on my Windows machine if that would help?

I think I can see part of the issue, so I'll stick a review comment on.

CrossR

This was the changes I ended up making to mine:

That seemed to make it run fine on Windows at least.

CrossR · 2018-07-04T19:54:42Z

test/ci/QuickOpenTest.ts

+    await oni.automation.sleep(100)
+    anyOni.automation.sendKeysV2("<CR>")
+    const editor = oni.editors.activeEditor
+    await oni.automation.waitFor(() => editor.activeBuffer.filePath.endsWith("/SearchProvider.ts"))


Windows paths use \ rather than / so this is most likely failing on Windows.

Ie, the output for me in this file on my machine is "F:\User Files\Documents\Git\oni\browser\src\Services\Search\SearchProvider.ts", so this will never pass on Windows.

I think you either need to remove the /, or if a path separator is needed, convert the filepath.

CrossR · 2018-07-04T20:11:23Z

test/ci/QuickOpenTest.ts

+    oni.commands.executeCommand("quickOpen.searchFileByContent")
+    await oni.automation.waitFor(() => oni.menu.isMenuOpen())
+    await oni.automation.sleep(100)
+    anyOni.automation.sendKeysV2("^exp.*?class.*?Qu.*?Op.*?Item")


I'm not fully sure what this search string is doing, but it kills my dev instance, so I assume that it either does the same to the tests, or its taking that long the correct file isn't opened (a few times on my machine I saw ACCOUNTING.md opened instead).

Perhaps its worth changing this, or swapping the working directory to a different folder in the Oni repo, so the search is easier?

CrossR · 2018-07-04T20:21:34Z

test/ci/QuickOpenTest.ts

+    anyOni.automation.sendKeysV2("^exp.*?class.*?Qu.*?Op.*?Item")
+    await oni.automation.sleep(100)
+    anyOni.automation.sendKeysV2("<CR>")
+    await oni.automation.sleep(500)


On my machine, I needed slightly longer sleeps, so I swapped them all to 2000.

CrossR · 2018-07-04T20:32:49Z

test/ci/QuickOpenTest.ts

+    anyOni.automation.sendKeysV2("<CR>")
+    await oni.automation.sleep(500)
+    const filePath = oni.editors.activeEditor.activeBuffer.filePath
+    assert.assert(filePath.endsWith("QuickOpenItem.ts"), `filePath`)


I was hitting issues with this assert where the test was finding itself! So it would search for some string and the first occurrence would be the test file itself. I think if you swap the search string to something unique to the test (MyUniqueTestString) and then assert the file is the test file, you should get better results.

TalAmuyal · 2018-07-05T15:32:10Z

@CrossR, thanks a bunch for that last batch or comments - I am now confident that I can make it work on the CI servers!

bryphe · 2018-07-05T15:34:20Z

Sweet! Thanks for all the help @CrossR ! 💯

TalAmuyal · 2018-07-17T21:00:18Z

@CrossR, when you have the time, please provide feedback or approve this PR.

CrossR

Issues I had last time seem to be fixed now!

One bug I have noticed is that is looks like hidden folders are ignored, so searching for something in there seems to fail. Ie, try searching for a number from the config.yml in the .github folder in the Oni repo.

That said....its a small bug, so I think its worth getting this merged so it can be used. The bug can be fixed later, and it lets us get more feedback. May be worth waiting for @bryphe or @akin909 input on that too though?

akinsho · 2018-07-19T22:50:01Z

It sounds like a fairly small bug that can be fixed in a follow up PR I'd love to see this functionality personally 👍

bryphe · 2018-07-20T02:04:33Z

That said....its a small bug, so I think its worth getting this merged so it can be used. The bug can be fixed later, and it lets us get more feedback.

Sounds good to me!

TalAmuyal · 2018-07-20T02:59:04Z

I'm so happy it is finally merged that I'm almost tearful...
Thanks guys, will move on the the next PR.
(BTW, will fix that hidden folder issue)

bryphe · 2018-07-20T03:29:18Z

Thanks for all the hard work on it, @TalAmuyal ! Excited it made it in too! 🎉

TalAmuyal self-assigned this Jun 13, 2018

TalAmuyal force-pushed the feature_add_a_find_in_files_menu branch from c9950a5 to 8618e94 Compare June 14, 2018 17:21

TalAmuyal mentioned this pull request Jun 15, 2018

[RFC] Modularization #2326

Open

TalAmuyal mentioned this pull request Jun 23, 2018

[RFC] Implementation of Spacemacs/Spacevim Concepts #2322

Open

akinsho mentioned this pull request Jun 28, 2018

Oni GUI for FZF.vim #2367

Closed

TalAmuyal force-pushed the feature_add_a_find_in_files_menu branch 2 times, most recently from a650afc to f21b05d Compare June 30, 2018 09:44

TalAmuyal requested a review from CrossR June 30, 2018 11:05

CrossR reviewed Jun 30, 2018

View reviewed changes

TalAmuyal mentioned this pull request Jun 30, 2018

Oni freezes after performing global search #1973

Open

TalAmuyal force-pushed the feature_add_a_find_in_files_menu branch 4 times, most recently from 98ffb89 to 9ad6ec1 Compare July 4, 2018 19:33

CrossR reviewed Jul 4, 2018

View reviewed changes

TalAmuyal force-pushed the feature_add_a_find_in_files_menu branch from 9ad6ec1 to 9fd162a Compare July 5, 2018 15:33

TalAmuyal force-pushed the feature_add_a_find_in_files_menu branch 2 times, most recently from 13b85e3 to 864fe09 Compare July 6, 2018 14:06

TalAmuyal force-pushed the feature_add_a_find_in_files_menu branch 7 times, most recently from d45a522 to 54bf22e Compare July 13, 2018 15:57

TalAmuyal force-pushed the feature_add_a_find_in_files_menu branch 4 times, most recently from a98ac62 to 51a526e Compare July 15, 2018 22:18

TalAmuyal added 8 commits July 18, 2018 21:06

Pull out QuickOpen to a plugin and add a 'find in files' option

9444a63

Fix smart-case search for 'find-in-files'

1147357

Fix opening file on wrong line/columnt

8d19659

Make the linter happy

2e877d2

Add tests

e702250

Refactor tests

0719412

Annotate tests

b76cb92

Break test down

6ab5b69

TalAmuyal force-pushed the feature_add_a_find_in_files_menu branch from b818854 to 91f18ae Compare July 18, 2018 18:11

Test

8d69256

TalAmuyal force-pushed the feature_add_a_find_in_files_menu branch from 91f18ae to 8d69256 Compare July 18, 2018 19:20

CrossR approved these changes Jul 19, 2018

View reviewed changes

TalAmuyal merged commit f475061 into onivim:master Jul 20, 2018

bryphe mentioned this pull request Aug 2, 2018

Search: Search doesn't show results on master #2487

Closed

Add a 'find in files' quick-menu #2315

Add a 'find in files' quick-menu #2315

Uh oh!

Conversation

TalAmuyal commented Jun 13, 2018

Uh oh!

codecov bot commented Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

CrossR commented Jun 16, 2018

Uh oh!

akinsho commented Jun 17, 2018

Uh oh!

TalAmuyal commented Jun 18, 2018 via email

Uh oh!

akinsho commented Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TalAmuyal commented Jun 30, 2018

Uh oh!

CrossR Jun 30, 2018

Choose a reason for hiding this comment

Uh oh!

TalAmuyal Jun 30, 2018

Choose a reason for hiding this comment

Uh oh!

TalAmuyal Jun 30, 2018

Choose a reason for hiding this comment

Uh oh!

CrossR commented Jun 30, 2018

Uh oh!

TalAmuyal commented Jun 30, 2018

Uh oh!

TalAmuyal commented Jul 4, 2018

Uh oh!

CrossR commented Jul 4, 2018

Uh oh!

CrossR left a comment

Choose a reason for hiding this comment

Uh oh!

CrossR Jul 4, 2018

Choose a reason for hiding this comment

Uh oh!

CrossR Jul 4, 2018

Choose a reason for hiding this comment

Uh oh!

CrossR Jul 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CrossR Jul 4, 2018

Choose a reason for hiding this comment

Uh oh!

TalAmuyal commented Jul 5, 2018

Uh oh!

bryphe commented Jul 5, 2018

Uh oh!

TalAmuyal commented Jul 17, 2018

Uh oh!

CrossR left a comment

Choose a reason for hiding this comment

Uh oh!

akinsho commented Jul 19, 2018

Uh oh!

bryphe commented Jul 20, 2018

Uh oh!

TalAmuyal commented Jul 20, 2018

Uh oh!

bryphe commented Jul 20, 2018

Uh oh!

Uh oh!

codecov bot commented Jun 14, 2018 •

edited

Loading

akinsho commented Jun 18, 2018 •

edited

Loading

CrossR Jul 4, 2018 •

edited

Loading