Hide popover when doc is edited or changes #3538
Conversation
| @@ -305,6 +307,21 @@ define(function (require, exports, module) { | |||
| return lastPreviewedImagePath; | |||
| } | |||
|
|
|||
| function onActiveEditorChange(event, current, previous) { | |||
| if (currentDocument) { | |||
There was a problem hiding this comment.
Need a comment here since we're referring to the old document in this if block and currentDocument is updated after this if block.
Or maybe you should use previous param to distinguish the old one from a new one.
There was a problem hiding this comment.
If we used previous.document, then the first time this method is called, we will call releaseRef() for a doc that we did not call addRef().
Keeping track of the doc ourself assures that we keep references (and event listening) accurate.
There was a problem hiding this comment.
I think in other places we cheat a little and don't bother to addRef() for listeners that are exactly tracking the current document (or active/focused editor) -- since as long as there's an editor we can be certain the Document isn't going away. If we did that here it would simplify the code a bit. But the way it is here seems fine too... your call.
There was a problem hiding this comment.
I'm all for cleaner code. So, you think it's ok to stop listening to previous.document and start listening to current.document? Doesn't seem as reliable.
There was a problem hiding this comment.
If you're not add/RemoveRef()'ing anymore then that should work fine. Lots of stuff relies on activeEditorChange, so I think we can trust the event to fire reliably. That said, the code as written here should work fine too.
There was a problem hiding this comment.
Aside: we repeat this pattern enough that I've been wondering if we should eventually factor this out into some sort of proxy object that's easier to listen to. Will definitely keep pondering that...
|
|
||
| if (currentDocument) { | ||
| currentDocument.addRef(); | ||
| $(currentDocument).on("change", hidePreview); |
There was a problem hiding this comment.
You can hover over an editor without it being active/focused, so you may wind up listening to the wrong editor here. But, I think it may be ok anyway.
For practical purposes, you can't modify an editor that's not focused without moving the mouse at all, so we won't miss any changes that would invalidate the popover. We could get a false positive -- hiding a popover in one editor when a change happens in a different, focused editor. But that doesn't seem too bad.
There was a problem hiding this comment.
Yes, just dismiss on any edit. I think this is good enough.
|
@peterflynn waiting for responses to 2 questions about your comments. |
|
@redmunds just posted responses back |
|
Code Review changes pushed. |
| } else { | ||
| editorHolder.removeEventListener("mousemove", handleMouseMove, true); | ||
| editorHolder.removeEventListener("scroll", hidePreview, true); | ||
|
|
||
| // Cleanup doc "change" listener | ||
| onActiveEditorChange(null, null, null); |
There was a problem hiding this comment.
This needs to get changed now so the last arg is EditorManager.getActiveEditor() -- otherwise the last-used listener won't get removed when it's toggled off.
…cument it was last attached to.
|
I've pushed up a fix for that one last issue since Randy is busy with #3540. Good to merge now! |
Hide Hover Preview popover when doc is edited or switched
This is for #3484 and #3513.