discussion how to achieve showing context around search results

[dirk-thomas]

Since the lack of context around the search results is really limiting sometime I looked into this topic a bit more myself.

Option A: I have seen the discussion in #190 as well as the proposed PR #702 which achieves this goal. It does so by creating a buffer for each match and then read the additional lines around the match to show them as context. I am not sure if that is potentially a "problem" but opening buffers for each match seems to be imply quite some resource usage if the number of search results or the file sizes in the result are large. There I thought about a different way to achieve the same goal.

Option B: An alternative approach (which wouldn't require to open buffers for each match) would require the result of the regular expression to already contain the context. This is related to the feature request on scandal to allow searching across newlines (atom/scandal#5). If the regex would allow searching across line boundaries the search pattern could be easily extended to prepend / append the necessary lines for context. Then the returned match would already contain the context lines and they could be visualized without the need to open buffers. The downside might be that since the result is only available as plain text it might be difficult to apply syntax highlighting in the future.

@lee-dohm Some time ago I brought this topic up on slack. You recommended to get some feedback from one of the Atom developers about the question if the resources (in terms of memory / IO) from option A would pose a problem in cases of large scale. It would be great if someone with more knowledge of the Atom internals could comment which option would be preferred to ensure the solution will scale well.

[iolsen]

@maxbrunsfeld do you like one of these options, or want to propose a third?

[maxbrunsfeld]

I don't think that we should open a buffer for each file containing search results. I would be in favor of adding an explicit contextLines option to Workspace.scan that would cause each match object to have additional textBeforeMatchLine and textAfterMatchLine fields.

Regarding syntax highlighting search results, that's a tricky problem. I'm not sure that it's worth doing, because in general, syntax highlighting can depend on content much earlier in the file, so it's going to come at a serious performance cost.

[dirk-thomas]

Option B sounded more efficient to me too. Glad we agree on that.

Regarding the implementation: Workspace.scan calls DefaultDirectorySearcher.search which uses DirectorySearch which uses scan-handler which then uses scandal. Do you imagine one of the higher levels to look at the scandal match result, load the file again and extend the match result with the context or do you think my proposal to extend the regex into a multiline regex would be the way to go (so that the match result already contains the context)?

[maxbrunsfeld]

I think that rather than trying to modify the regex (and then work backwards to figure out the original match), we should change scandal to explicitly pass context back to the caller.

[dirk-thomas]

Ok, sounds even better if "context" is just known across all involved levels. Is this something you would like to have a PR for or do you think it doesn't make much sense for me to try implementing this and you rather want to implement it yourself?

[dirk-thomas]

@maxbrunsfeld Can you let me know what you think about the implementation approach of this. Preference for an external PR (we could iterate on the API beforehand) or rather do it yourself?

[maxbrunsfeld]

@dirk-thomas I would say that we'd be open to a PR on scandal to add this feature. Unfortunately, nobody on the team is very familiar with that codebase anymore, so there's going to be some work involved on our part to do the requisite testing, which might slow the process down.

[dirk-thomas]

Ok, I will try to find some time for it. I will likely first create a PR to propose the API and once that looks good fill it with the actual implementation.

[dirk-thomas]

@maxbrunsfeld It took me a while to find the time but I finally created a PR for scandal adding options to provide additional context lines. Since I had to learn quite a bit about how scandal works internally (and more about coffeescript in general) I ended up filling the API with an implementation in one step - also to convince myself that it will work. Please see atom/scandal#41.

[michaeljonathanblack]

I'd love to help see this feature to the finish line, if there's anything another set of hands will get you.

[dirk-thomas]

Please see #847 for the current state of the PRs.

[dirk-thomas]

I will close this since the discussion has let us to the currently pending set of PRs.