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 crash when using find view in tab that’s not an editor #1100

Merged
merged 5 commits into from Nov 1, 2019

Conversation

@ariasuni
Copy link
Contributor

ariasuni commented Sep 28, 2019

Description of the Change

  • Checks for the presence of an editor before trying to search text.
  • Make find and find all buttons disabled when their feature is not available and add an helpful tooltip on them.

Alternate Designs

It is implement in buffer-search.js because filters need to be updated, and stopping the search in find-view.js would prevent that.

Benefits

The fact that now the buttons are disabled and provide a hint of why it is disabled will help people understand why the find/find all feature is unavailable here.

Possible Drawbacks

I can’t see any.

Applicable Issues

Fix #998


I think these changes need unit tests but I find it more difficult to find what should go where and how to implement these tests properly, so any help to get started is appreciated.

Also during my work, I found that I was able make filters unmodifiable (i.e. clicking filters did nothing) when pane was opened on a non-editor page, so that will need to be tested too.

@ariasuni

This comment has been minimized.

Copy link
Contributor Author

ariasuni commented Sep 29, 2019

I spotted several bugs introduced by my changes:

  • When opening for the first time on a non-editor page (e.g. Settings), the find field does not display “No results”. Found thanks to automated tests.
  • When opened on a non-editor page, filters can’t be changed. This case lacks automated tests.
@ariasuni

This comment has been minimized.

Copy link
Contributor Author

ariasuni commented Sep 29, 2019

I don’t understand why those tests don’t work, because the behavior they describe is not what I get by doing by hand what the test describe with stable find-and-replace, so I don’t even know what code path give this result.

@rsese

This comment has been minimized.

Copy link
Member

rsese commented Oct 2, 2019

Thanks so much for for opening this pull request @ariasuni

I think these changes need unit tests but I find it more difficult to find what should go where and how to implement these tests properly, so any help to get started is appreciated.

...

I don’t understand why those tests don’t work, because the behavior they describe is not what I get by doing by hand what the test describe with stable find-and-replace, so I don’t even know what code path give this result.

Though it's possible someone might jump in with suggestions here, the best place to get help with testing questions is the Atom Slack - not sure if you're on there already, but you can request an invite as detailed in the link.

@rsese rsese added the needs-testing label Oct 2, 2019
spec/find-view-spec.js Outdated Show resolved Hide resolved
@ariasuni

This comment has been minimized.

Copy link
Contributor Author

ariasuni commented Oct 4, 2019

I guess it’s good now.

@rsese

This comment has been minimized.

Copy link
Member

rsese commented Oct 8, 2019

Thanks @ariasuni 🙇 We can't promise a specific timeframe for when this will be reviewed but we'll ask the team to take a look.

@rsese rsese added triaged and removed needs-testing labels Oct 8, 2019
@head898 head898 mentioned this pull request Oct 29, 2019
10 of 11 tasks complete
@darangi darangi merged commit 0888bc2 into atom:master Nov 1, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.