Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Only emit did-update when there are changes; set consistent classes on error #852

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Only emit did-update when there are changes; set consistent classes on error #852

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
64b6a20
Don't try to update markers if there's no new markers
winstliu Feb 7, 2017
f1aa436
Fix deceptive class when an error occurs
winstliu Feb 7, 2017
53db29a
Add a spec
winstliu Feb 9, 2017
5fa4b3b
Add spec for has-no-results
winstliu Feb 9, 2017
e81f8df
Add tests to prevent superfluous update events
winstliu Feb 9, 2017
e4c3c4b
Revert "Don't try to update markers if there's no new markers"
winstliu Feb 15, 2017
0d07d56
Revert "Add tests to prevent superfluous update events"
winstliu Feb 15, 2017
9e8e75b
:art:
winstliu Feb 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion lib/buffer-search.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the other part of this PR is still worthwhile though!

Copy link
Contributor Author

@winstliu winstliu Feb 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


const oldMarkers = this.markers.splice(spliceStart, (spliceEnd - spliceStart) + 1, ...newMarkers);
for (let oldMarker of oldMarkers) {
oldMarker.destroy();
Expand All @@ -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());
Copy link
Contributor

@maxbrunsfeld maxbrunsfeld Feb 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 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.

this.currentResultMarker = null;
this.setCurrentMarkerFromSelection();
}
Expand Down
3 changes: 2 additions & 1 deletion lib/find-view.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,6 @@ class FindView extends View
atom.beep()

markersUpdated: (@markers) =>
@findError = null
@updateResultCounter()
@updateReplaceEnablement()

Expand Down Expand Up @@ -342,6 +341,8 @@ class FindView extends View
@descriptionLabel.text(infoMessage).removeClass('text-error')

setErrorMessage: (errorMessage) ->
this.removeClass('has-results')
this.addClass('has-no-results')
@descriptionLabel.text(errorMessage).addClass('text-error')

clearMessage: ->
Expand Down
19 changes: 19 additions & 0 deletions spec/buffer-search-spec.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,25 @@ describe "BufferSearch", ->

expect(scannedRanges()).toEqual []

describe "when no markers are affected in a change event", ->
it "does not emit an update event", ->
editor.setCursorBufferPosition([0, 0])
editor.backspace()
advanceClock(editor.buffer.stoppedChangingDelay)
expectNoUpdateEvent()

describe "when the find pattern is invalid and a change occurs", ->
it "does not attempt to update markers", ->
model.search "a(",
caseSensitive: false
useRegex: true
wholeWord: false

editor.insertText(".")
advanceClock(editor.buffer.stoppedChangingDelay)

expectNoUpdateEvent()

describe "replacing a search result", ->
beforeEach ->
editor.scanInBufferRange.reset()
Expand Down
23 changes: 23 additions & 0 deletions spec/find-view-spec.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -506,6 +506,29 @@ describe 'FindView', ->
expect(findView.descriptionLabel).toHaveClass 'text-error'
expect(findView.descriptionLabel.text()).toContain 'Invalid regular expression'

it "does not reset when the buffer is updated", ->
# This is a rather specific test: the error message only disappeared
# when there were no changes. Backspacing nothing can reproduce this,
# as well as saving or undoing and then redoing.
editor.setCursorBufferPosition([0, 0])
# Since we're at the beginning of the file, backspacing will do nothing but still send a change event
editor.backspace()
advanceClock(editor.buffer.stoppedChangingDelay)

expect(findView.descriptionLabel).toHaveClass 'text-error'
expect(findView.descriptionLabel.text()).toContain 'Invalid regular expression'

it "adds the has-no-results class", ->
findView.findEditor.setText 'it'
atom.commands.dispatch(findView.findEditor.element, 'core:confirm')
expect(findView).toHaveClass 'has-results'
expect(findView).not.toHaveClass 'has-no-results'

findView.findEditor.setText 'i[t'
atom.commands.dispatch(findView.findEditor.element, 'core:confirm')
expect(findView).toHaveClass 'has-no-results'
expect(findView).not.toHaveClass 'has-results'

it "will be reset when there is no longer an error", ->
expect(findView.descriptionLabel).toHaveClass 'text-error'

Expand Down