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.

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

Don't watch TextEditors outside of the current project in Project Find #988

merged 5 commits into from Jan 25, 2019


None yet
5 participants
Copy link

anderoonies commented Jan 10, 2018

Description of the Change

workspace.scan only searches the active project. However, ResultsModel was subscribing to all TextEditors for updating the results of a search, including TextEditors that don't belong to the project.

To reproduce this, create a hierarchy something like

 |- 1.txt
  |- 2.txt
  |- 3.txt

and open atom with atom nested—specifically setting the project path to be root/nested. A Project Find will only include buffers in nested (2.txt, 3.txt).
Open 1.txt and modify its contents to include the search term. It will appear in the Results View.

This PR adds a simple check to the ResultsModel workspace subscription to prevent this from happening.

Alternate Designs

The subscriptions could be exclusively created with TextEditors that exist in the current project, but managing creating/disposing of those adds complexity and room for error.


Removes confusion/unintuitive behavior around finding and replacing.

Possible Drawbacks

May have become an anti-pattern, removal of which could cause confusion.
Specs became more complex because of the introduction of a level of nesting.

Applicable Issues



This comment has been minimized.

Copy link

bhj commented Jan 10, 2018

@anderoonies Thanks and just to clarify, this occurs between sibling folders as well - for example, assuming the project folder is root:

 nested A
  |- 1.txt
 nested B
  |- 2.txt
  |- 3.txt

Doing a Search in Directory on nested A would return results from nested B if 2.txt or 3.txt were modified.

Sorry if your PR already addresses this, just wasn't sure.


This comment has been minimized.

Copy link
Contributor Author

anderoonies commented Jan 10, 2018

@bhj this covers that case, too! good note, thanks for catching it

@asheren asheren referenced this pull request Oct 31, 2018


Iteration Plan: October 29 - November 9 #18368

4 of 5 tasks complete

daviwil and others added some commits Jan 10, 2019

Copy link

smashwilson left a comment

👍 Makes sense. Good catch, thank you 🙇

@smashwilson smashwilson merged commit bd34f9b into atom:master Jan 25, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
continuous-integration/travis-ci/pr The Travis CI build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.