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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore pane items that aren't text editors #966

Merged
merged 12 commits into from Dec 28, 2018

Conversation

Projects
None yet
3 participants
@howyi
Copy link
Contributor

howyi commented Oct 23, 2017

This PR fixes #898 #815 #679 #649 #582

Description of the Change

Edited by @50Wliu
BufferSearch only works on TextEditors, but we were mistakenly trying to set an editor for BufferSearch for any pane item that implemented the getBuffer method. We now explicitly check to make sure the new pane item is a TextEditor before setting BufferSearch's editor.

Alternate Designs

Benefits

  • usable PlantUML Viewer

Possible Drawbacks

I don't see any 馃悗

Applicable Issues

Fixes #898
Fixes #815
Fixes #679
Fixes #649
Fixes #582

@realthargor

This comment has been minimized.

Copy link

realthargor commented Dec 12, 2018

@jasonrudolph Would be awesome if this would get a review and approval. Not sure who else to ping, couldn't find anything in contribution and you seem to be active here.

@50Wliu

This comment has been minimized.

Copy link
Member

50Wliu commented Dec 12, 2018

@howyi would it be more appropriate to use this.editor.getBuffer() in setEditor since that's what the conditional is checking for?

@howyi howyi force-pushed the howyi:master branch from 8be1283 to ab8b1f9 Dec 20, 2018

@howyi

This comment has been minimized.

Copy link
Contributor Author

howyi commented Dec 20, 2018

@50Wliu

use this.editor.getBuffer() in setEditor

Changed! 馃槂

@50Wliu

This comment has been minimized.

Copy link
Member

50Wliu commented Dec 27, 2018

Well, that fixes that error, but immediately leads to another one: Uncaught TypeError: this.editor.onDidAddSelection is not a function (from the very next line).

Looking into it.

50Wliu added some commits Dec 27, 2018

@50Wliu

This comment has been minimized.

Copy link
Member

50Wliu commented Dec 27, 2018

All right, so plantuml-viewer is creating a PlantumlViewerEditor that has a getBuffer method, but find-and-replace is assuming that any object with getBuffer is a TextEditor.

I think it would make sense to change the guard from paneItem?.getBuffer?() to paneItem instanceof TextEditor. Here's a diff:

diff --git a/lib/find.coffee b/lib/find.coffee
index 245a4fc..56a309e 100644
--- a/lib/find.coffee
+++ b/lib/find.coffee
@@ -1,4 +1,4 @@
-{CompositeDisposable, Disposable, TextBuffer} = require 'atom'
+{CompositeDisposable, Disposable, TextBuffer, TextEditor} = require 'atom'

 SelectNext = require './select-next'
 {History, HistoryCycler} = require './history'
@@ -30,7 +30,7 @@ module.exports =
     @resultsModel = new ResultsModel(@findOptions)

     @subscriptions.add atom.workspace.getCenter().observeActivePaneItem (paneItem) =>
-      if paneItem?.getBuffer?()
+      if paneItem instanceof TextEditor
         @findModel.setEditor(paneItem)
       else
         @findModel.setEditor(null)

@howyi If you agree with that, I'll start working on some updated specs for the new guard.

50Wliu and others added some commits Dec 27, 2018

Revert "Add spec"
This reverts commit 0d44fd5.
@howyi

This comment has been minimized.

Copy link
Contributor Author

howyi commented Dec 27, 2018

@50Wliu Thank you ! 馃

If you agree with that, I'll start working on some updated specs for the new guard.

I agree, and fixed if paneItem?.getBuffer?() to if paneItem instanceof TextEditor.
Please update specs!

50Wliu added some commits Dec 28, 2018

@50Wliu

This comment has been minimized.

Copy link
Member

50Wliu commented Dec 28, 2018

Okay, I can confirm that this new fix resolves the onDidStopChanging bug, and that the spec fails when the change is reverted. @howyi does everything look good to you?

@howyi

This comment has been minimized.

Copy link
Contributor Author

howyi commented Dec 28, 2018

@50Wliu Yes! this is a good fix for me.

@50Wliu 50Wliu changed the title Fix Uncaught TypeError:[Cannot read property 'onDidStopChanging'] Ignore pane items that aren't text editors Dec 28, 2018

@50Wliu 50Wliu merged commit ab075ac into atom:master Dec 28, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@50Wliu

This comment has been minimized.

Copy link
Member

50Wliu commented Dec 28, 2018

Thanks! 馃専

@realthargor

This comment has been minimized.

Copy link

realthargor commented Jan 4, 2019

@50Wliu @howyi Thanks a lot :)

@50Wliu 50Wliu referenced this pull request Jan 4, 2019

Merged

Update find-and-replace #18649

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.