Don’t crash when using find view in tab that’s not an editor

atom / find-and-replace

Find and replace in a single buffer and in the project

MIT License

242 stars 196 forks source link

Don’t crash when using find view in tab that’s not an editor #1100

Closed ariasuni closed 4 years ago

ariasuni commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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.

ariasuni commented 5 years ago

I guess it’s good now.

rsese commented 4 years ago

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