Only emit did-update when there are changes; set consistent classes on error #852
Conversation
Consider the following scenario: 1. Turn on regex 2. Search for something simple in the file, eg "this" 3. has-results class appears as expected 4. Add an opening parenthesis to the find pattern 5. Find box is still green?!?! Step 5 introduces contradicting information: a red error message yet a green find box with the has-results class. So now when we set an error message we make sure to remove the has-results class and add the has-no-results class. I'm also fairly certain that nulling a function is not a good idea, so I removed that line.
lib/buffer-search.js
Outdated
| @@ -224,6 +224,8 @@ class BufferSearch { | |||
| } | |||
|
|
|||
| const newMarkers = this.createMarkers(scanStart, scanEnd); | |||
| if(newMarkers === false || !newMarkers.length) return; // Invalid/missing find pattern or no new markers | |||
There was a problem hiding this comment.
Even if there are no new markers, we still might want to destroy some old markers, and we certainly want to continue processing the other entries in changes, because they might cause us to find new markers.
For example, suppose your editor was in this configuration (cursors represented by |):
1. one
2. two|
3. three
4. foo|ur
and you were searching for the pattern two|four and typed a backspace. That would destroy the match on line 2 but create a match on line 4.
In that situation, we wouldn't to return early from bufferStoppedChanging just because we didn't find any new markers in the first changed region.
There was a problem hiding this comment.
I think what we might want to do is make the createMarkers return an empty array if no matches were found in the given range.
There was a problem hiding this comment.
We've fixed this as part of #860. Didn't mean to steal your 🌩 but we're on a spree of uncaught exception fixes after publishing 1.14.
There was a problem hiding this comment.
I think the other part of this PR is still worthwhile though!
There was a problem hiding this comment.
Yeah I thought I noticed a regression related to that, but didn't realize what the steps to reproduce were.
I'll update this PR to only focus on the UX changes.
lib/buffer-search.js
Outdated
| @@ -237,7 +239,7 @@ class BufferSearch { | |||
| } | |||
| } | |||
|
|
|||
| this.emitter.emit('did-update', this.markers.slice()); | |||
| if(changes.length) this.emitter.emit('did-update', this.markers.slice()); | |||
There was a problem hiding this comment.
👍 This makes a lot of sense.
We don't have JS linting enabled on this repo, but usually we include a space after the word if.
winstliu
changed the title
| @@ -237,7 +237,7 @@ class BufferSearch { | |||
| } | |||
| } | |||
|
|
|||
| this.emitter.emit('did-update', this.markers.slice()); | |||
| if (changes.length) this.emitter.emit('did-update', this.markers.slice()); | |||
There was a problem hiding this comment.
Should this still be checking for changes then, or should it be checking for changed markers? (Not sure how to do that)
Requirements
Description of the Change
Consider the following scenario:
test.testand notice that the find box turns green (using the One themes).At this point, there is contradictory information: a red error message is presented, yet the find box is still green, as if there are results. This is because when an error message is set, it does not remove the
has-resultsclass, nor does it add thehas-no-resultsclass. Now we do both, and the find box turns red as expected.In addition, it should fix an edge case where changing nothing would cause error messages to disappear.
Finally, I removed what I think is an incorrect line that nulls out the
findErrorfunction as soon as markers are updated.Alternate Designs
None. This seems like a very clean solution to me.
Benefits
No more errors when typing text while there's an invalid find pattern, in addition to improved UX when switching over to an invalid find pattern.
Possible Drawbacks
I'm pretty confident that there will be none.
Applicable Issues
Fixes #851