search

brackets-archive / bracketsIssues

Archive of issues in brackets.
0 stars 0 forks source link

[CLOSED] Instant Search. #9915

Open core-ai-bot opened 3 years ago

core-ai-bot commented 3 years ago

Issue by abose Saturday Jul 11, 2015 at 17:02 GMT Originally opened as https://github.com/adobe/brackets/pull/11375

Instant Find In Files Search This change deals with moving the search logic to Node with instant search results. The overall changes covered are as follows:-

  1. The search cache and the search logic itself now live in Node process.
  2. Increased search performance and instant search - see the results as you type.
  3. Brackets UI is unblocked/responsive while a search is in progress.
  4. Preference entries findInFiles.instantSearch and findInFiles.nodeSearch for enabling.
  5. If Node search fails, the Brackets search will be used as fallback until a project switch occurs and Node responds.
  6. Ordering of search results made better: Now not only the currently opened file gets precedence (very first) but also the files in working set. Order of precedence : Open file -> files in working set -> remaining files.
  7. File watchers and document change events captured and propagated to Node.

Pending

Contributors:@rroshan1@prksingh@prafulVaishnav@nethip .

abose included the following code: https://github.com/adobe/brackets/pull/11375/commits

core-ai-bot commented 3 years ago

Comment by rroshan1 Sunday Jul 12, 2015 at 07:24 GMT

I believe this awesome "Instant Find In Files Search" is going to be a hit with users and help bring more developers to Brackets!! We have come a long way from the slow search perception to instant search which no other editor provides this well. Guys, please use this feature and do let us know your feedback (experience + issues). Also, you can review the code and share your comments.

core-ai-bot commented 3 years ago

Comment by rroshan1 Sunday Jul 12, 2015 at 08:38 GMT

Here is a gif of how this feature looks!! instant_search

core-ai-bot commented 3 years ago

Comment by mackenza Sunday Jul 12, 2015 at 13:08 GMT

@abose I checked out upstream/abose/instant_search and gave it a try. It worked great on my Brackets repo and I wanted to see it work on others, so I switched projects and tried again and find-in-files doesn't trigger until I press enter. I went back to brackets and tried again and it's not working there either.

I added findInFiles.instantSearch: true and findInFiles.nodeSearch to my brackets.json but it didn't change.

Am I doing something wrong?

core-ai-bot commented 3 years ago

Comment by rroshan1 Sunday Jul 12, 2015 at 13:13 GMT

@mackenza It is intended!! Actually, Node caches the project files at project load, so during that 1 minute or so, if someone launches a find in files search, Brackets falls back to its older search (without instant search and acts on hitting enter). But once caching is complete, it starts with instant search!!

core-ai-bot commented 3 years ago

Comment by mackenza Sunday Jul 12, 2015 at 13:27 GMT

@abose is there some way we could give a visual indication of when the indexing is done?

core-ai-bot commented 3 years ago

Comment by mackenza Sunday Jul 12, 2015 at 13:57 GMT

@abose I have an older laptop I run Linux on (Dell Latitude E6500) and I can't seem to get this working. When I switch projects, I wait for a good long time (recognizing that my slow laptop means slower chaching time) but I never seem to get instant search. I look at top on the machine and I would expect to see brackets-node with some cpu trying to cache the project, but I don't see that either.

core-ai-bot commented 3 years ago

Comment by abose Sunday Jul 12, 2015 at 15:17 GMT

@mackenza could you set the preferences entries to true, restart brackets, and then try the search. What is the project size you are on? [number of files, size in MB] The first caching of the files will take a bit of time, but search will still function as normal on pressing enter.
Also can you tell us the file caching time: - to get this, open search window, type something and wait till you see the results- instant search will kick in eventually after cache complete. Also can you give the OS/age of laptop/ Any other performance stats that we can work upon. Did you get a chance to try this out in any other configuration?

core-ai-bot commented 3 years ago

Comment by abose Sunday Jul 12, 2015 at 15:30 GMT

@larz0 Could you check this out.@mackenza was asking for some visual indication till the indexing is complete.

core-ai-bot commented 3 years ago

Comment by mackenza Sunday Jul 12, 2015 at 17:52 GMT

@abose I couldn't get anything working, even Getting Started project. So I did a git checkout master and then back to git checkout abose/instant_search and now it seems to work like a charm. I am getting results back seconds after I switch projects, even large ones (like I cloned v8). Not sure why my install went dead there but it's back now. Great feature.

core-ai-bot commented 3 years ago

Comment by mackenza Sunday Jul 12, 2015 at 18:22 GMT

@abose I was thinking about the UI and have some ideas but wasn't sure of the difference between findInFiles.instantSearch: true|false and findInFiles.nodeSearch: true|false? I can infer some things, but the combination is interesting... i.e. could I have nodeSearch false if I have instantSearch true?

Depending on the answer to the above and the relationship of the 2 settings I would think a UI like the following would do the trick:

instant_search

So if you ignore the horrible gimp editing, I am proposing a 2-state toggle button like we have for case and pattern already to indicate instant-search either on or off (default should be on imo) and some sort of green-light vs redlight/greylight indicator whether the current project has finished caching or not.

I am sure@larz0 can make it look good ;)

core-ai-bot commented 3 years ago

Comment by rroshan1 Sunday Jul 12, 2015 at 20:04 GMT

@mackenza : No we cannot have instantSearch set to true with nodeSearch set to false. Node Search will automatically be disabled in case Node does not work properly and Brackets will fall back to older search. But if Node works fine, user has the option of going ahead with Instant Search (using Node of course) or disable it to use traditional search. +1 to the idea of some switch indicator for Instant Search.

core-ai-bot commented 3 years ago

Comment by mackenza Sunday Jul 12, 2015 at 21:33 GMT

@rroshan1 that is what I thought. It seems to me that findInFiles.nodeSearch need not be a public setting since it's setting has no meaningful effect to the user. It should be something you can toggle at a dev level for perhaps debugging purposes but not exposed to the user in the default brackets.json or in a UI.

core-ai-bot commented 3 years ago

Comment by larz0 Monday Jul 13, 2015 at 02:03 GMT

@mackenza thanks for the tag :)

Instant Search should be on by default and in the odd time when Brackets is indexing we should indicate it by using our spinner with a label mentioning it's for Instant Search.

brackets_cachingforinstantsearch

Love this feature :+1:

core-ai-bot commented 3 years ago

Comment by rroshan1 Monday Jul 13, 2015 at 06:47 GMT

@larz0 Indicator for caching (not exactly indexing) is good. :+1: This means Instant Search will be active after caching is complete. But, we should have an indicator for instant search as well since we are providing the user the option to disable it. Also, during the caching, the instant search functionality is disabled and it is enabled only after caching is complete. What sort of indicators would you suggest for it? Summary State of Instant Search :-

core-ai-bot commented 3 years ago

Comment by abose Monday Jul 13, 2015 at 10:42 GMT

I think for the time being, we can just keep the spinner on indexing as suggested by@larz0 . Exposing too many options about internal details to the user could be confusing.

But adding a toggle for instant search could be useful in some very large projects.

core-ai-bot commented 3 years ago

Comment by mackenza Monday Jul 13, 2015 at 12:40 GMT

Disabling Instant Search seems low volume enough that it can stay as preference set in brackets.json and not exposed in the UI. I think that is what@larz0 is indicating.

core-ai-bot commented 3 years ago

Comment by redmunds Monday Jul 13, 2015 at 16:51 GMT

I took this branch for a quick spin. The performance is impressive.

I originally thought that I would not like "instant search", and would turn it off, but it's fast enough that it works! On my machine anyway -- it's good to have this a preference so users with slower machines can turn it off.

I notice that the sort order is incorrect. This was a huge bottleneck in my "incremental update" branch, so I'm curious how much this will effect performance after it's fixed. It should only delay the initial indexing.

Also, the sort order for working set items has changed. In master, the currently viewed file is displayed at the top and all other are sorted as they are in the file tree. In this branch, all of the working set items are listed at the top. I think I like this better, but thought I'd bring it up for discussion.

I also notice that search bar is not dismissed after instant search. This makes sense because the search is updated as you type. I think it also makes sense to always show the bottom panel (even when there are no hits for search). I came to this conclusion in my branch so user gets faster feedback, but it also makes sense with instant search because panel can go away and come back as you type.

I also ran into the problem when searching too early and nothing happens -- need some kind of feedback or execute search when ready.

Ctrl-click on disclosure arrows in panel is broken. It should toggle all entries open/closed. It either does nothing, or it toggles all and then after a short delay toggles everything back to initial state

This branch is very promising. Overall a great job!

core-ai-bot commented 3 years ago

Comment by nethip Monday Jul 13, 2015 at 20:39 GMT

@redmunds Thanks for your kind words! And really appreciate you checking out this branch and sharing your feedback. The team indeed had put in lot of effort to get it to this state. So kudos to the entire team for doing a fabulous job and for putting in countless hours. In fact we had go back and forth between web threads vs a new node process vs using existing node process. Finally we settled with using existing node process.(which gave us more challenges). We are really excited about this feature and can't wait to see this getting shipped for 1.4.

Agree with you on the visual feedback when instant search is not yet available/ the search takes more time to get the results.

core-ai-bot commented 3 years ago

Comment by petetnt Tuesday Jul 14, 2015 at 09:01 GMT

This is really a killer feature, awesome :+1:

RE: Waiting for the indexing to complete: would it be possible to search the partial cache while the indexing is still going on? As in that the crawling would send periodical update events at which point the instant search would update itself from that point of the progress.

After everything is cached, the Indexing for instant search spinner would disappear and Instant search would work like it normally does. This way you wouldn't have to fallback to the old Find In Files even if the caching was on-going. (I can see how this makes cases where uses changes his search terms while the indexing is going on more tricky though)

core-ai-bot commented 3 years ago

Comment by nethip Tuesday Jul 14, 2015 at 09:46 GMT

RE: Waiting for the indexing to complete: would it be possible to search the partial cache while the indexing is still going on? As in that the crawling would send periodical update events at which point the instant search would update itself from that point of the progress.

@petetnt This is exactly is what is happening now. Searching and caching are now interleaved. As I when the first 100 results are got, they are sent back to the Brackets.@abose@rroshan1 can fill in more finer technical details about this.

After everything is cached, the Indexing for instant search spinner would disappear and Instant search would work like it normally does. This way you wouldn't have to fallback to the old Find In Files even if the caching was on-going.

Agree with the instant search spinner should disappear once the caching is complete.

(I can see how this makes cases where uses changes his search terms while the indexing is going on more tricky though)

These were the exactly the challenges with instant search I was mentioning about earlier. The team did lot of collaboration on this front, and made sure we don't have race conditions. e.g. the search results and the search terms should be in sync. Of course we still need to handle some conditions where search is taking some time to get the results in which case we need to have visual cues.

core-ai-bot commented 3 years ago

Comment by abose Tuesday Jul 14, 2015 at 14:36 GMT

Thanks all for the review, I have added the spinner as suggested by@larz0 on caching progress. @redmunds The sort order has been changed so that we see the current file, followed by other files in working set(that passes the filters), and then the other results. We did not sort the results as that was one of the larger performance hogs, but yes, we only sort the file list at the start of a search, so this should not affect the instant search when optimizes properly. I Like the current order though, it follows the project tree order more predictable that the other order- which i only came by when reading the code.

About the search too early, if you press enter after a search, the search would be forced until 100 results are available or all files are read. This would not block brackets though. If you press enter 2-3 times during this time, all the searched would be queued. We could pop a toast message when this happens telling that a search is already in progress and not queue the search.

The disclosure button thing is a work in progress which i just learnt from the review comments. Checking it out.

core-ai-bot commented 3 years ago

Comment by redmunds Tuesday Jul 14, 2015 at 15:34 GMT

@abose

I Like the current order though, it follows the project tree order more predictable that the other order- which i only came by when reading the code.

No, it's not in project tree order (@TomMalbran and I wish it was). If you been following the discussion in https://github.com/adobe/brackets/pull/11128 you know we've been looking at this.

For example, using the recipe I describe here, go to end of results, then page backwards, and you'll see that the results are not in file tree order. Hint: use Ctrl-click on disclosure arrow to collapse all file items to make it easier to see this.

Take a look at commits https://github.com/adobe/brackets/commit/607979620cc8ebc0ebe11856217d51b7fb56c021 / https://github.com/adobe/brackets/commit/0e85ed022610746a40f195d4b774fe30e64e43cc and https://github.com/adobe/brackets/commit/d5cf805a0a346da60cf17ebc0a47e58b6df08e8f for some sorting improvements. I also have a local (not yet pushed to branch) fix for sorting ProjectModel cache in ~1.1s if you want to see that (but would also need to fix clearing cache too often).

core-ai-bot commented 3 years ago

Comment by mackenza Tuesday Jul 14, 2015 at 15:47 GMT

@abose the visual indication of the spinner for caching the project is working for me. My Brackets project seems to take about 10 secs or so to complete caching on a HP Folio 9470m which has a Core i5-3437U and 4GB Ram on Windows 7 64-bit.

What triggers a cache refresh? I notice when I move away to another project and then back to Brackets, I don't seem to see the spinner anymore so I am assuming it is keeping the cache.

core-ai-bot commented 3 years ago

Comment by redmunds Tuesday Jul 14, 2015 at 15:59 GMT

What triggers a cache refresh?

Open src/project/ProjectModel.js and put breakpoints on this._allFilesCachePromise

This gets cleared on every file write. Unfortunately, this happens every time state.json is updated, which is on every file tree click or editor selection change.

core-ai-bot commented 3 years ago

Comment by abose Tuesday Jul 14, 2015 at 17:54 GMT

@mackenza in the node based implementation, the file cache is updated on project switch. The OS also does some file access optimizations, so the second time onwards, the file reads are much more faster[<3 secs on brackets src]- so the spinner won't be shown as the caching is already done.

@redmunds Assuming that the file list only needs to be sorted once, we should still be able to sort with instant search with some careful first-time flags around. But I think the sorting is not helping the search workflow in that I always thought the sort was pretty random until I read the code. Also having to click to scores of next buttons to reach a search is also not a good workflow. Instead of that, I suggest a different workflow; what if we maintain an unsorted search, But- We wire the Find in option to instant search, so that whenever someone selects find in from a folder in the project tree- we do an instant search with the new search scope?

core-ai-bot commented 3 years ago

Comment by mackenza Tuesday Jul 14, 2015 at 18:28 GMT

@abose I think you want to consider the impact of a random search results order. https://github.com/atom/find-and-replace/issues/64 shows it as a potential showstopper (as in switching back to ST3).

I am not sure I see your connection between Find In and Find in Files... maybe you could expand upon your proposed use case?

core-ai-bot commented 3 years ago

Comment by TomMalbran Tuesday Jul 14, 2015 at 18:34 GMT

@abose Even if the way the files are sorted seems random, is good to have them sorted, so that files are always added in the same order, and this is important when the results are updated while the panel is open after doing a search. Without sorting I could easily remove all the results from one file and re add some results making the file jump to the end of the list. And I believe that there are more examples that can do that.

Notice that the files are sorted every time the UI is shown. Maybe we don't need to sort the files before searching, but I think that we do need to sort them when displaying them.

core-ai-bot commented 3 years ago

Comment by abose Wednesday Jul 15, 2015 at 14:33 GMT

@redmunds@TomMalbran I have added a sort flag to ProjectManager.getAllFiles so that the files will be sorted during the tree traversal itself with file/dir names at each level. Please check this out. The collapse all big is under fix.

core-ai-bot commented 3 years ago

Comment by abose Thursday Jul 16, 2015 at 10:46 GMT

Implemented the collapse also. Since we are doing instant search, once a collapse is done, it will be retained on further instant searches and vice versa.

core-ai-bot commented 3 years ago

Comment by abose Tuesday Jul 21, 2015 at 12:20 GMT

instantfilter

Implemented new instant workflow: so that searching within a particular folder reflects live. This was the find in change i mentioned earlier.