Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

add current and previous documents to currentDocumentChange event #7509

Merged
merged 2 commits into from
Apr 28, 2014

Conversation

zaggino
Copy link
Contributor

@zaggino zaggino commented Apr 14, 2014

Useful nit to avoid calling DocumentManager.getCurrentDocument() everytime when hooked on the event and also the only way to know what was the previous document I think.

_currentDocument = doc;
$(exports).triggerHandler("currentDocumentChange");
$(exports).triggerHandler("currentDocumentChange", [_currentDocument, previousDocument]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't you use an associative array?
I'd take {current: _currentDocument, previous: previousDocument}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

triggerHandler gets arguments in an array and then spreads them to the callback like this:
https://github.com/zaggino/brackets-git/blob/43d62647ca8d8747f1cc649019f08bc451440fcf/src/BracketsEvents.js#L51-L56

@lkcampbell
Copy link
Contributor

+1 for this. I actually wish every Change event we have provided a newState and an oldState. Would save on maintaining cached states.

@peterflynn
Copy link
Member

@zaggino This looks good -- but please just update the docs at the top of the module to indicate the new 2nd & 3rd args.

@zaggino
Copy link
Contributor Author

zaggino commented Apr 28, 2014

@peterflynn done

@@ -59,7 +59,8 @@
* - documentRefreshed -- When a Document's contents have been reloaded from disk. The 2nd arg to the
* listener is the Document that has been refreshed.
*
* - currentDocumentChange -- When the value of getCurrentDocument() changes.
* - currentDocumentChange -- When the value of getCurrentDocument() changes. 1st argument to the listener
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be the second and third arguments, since the first is always the event itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sry. Early morning here :) Fixed

@peterflynn
Copy link
Member

Looks good now -- thanks!

peterflynn added a commit that referenced this pull request Apr 28, 2014
Add current and previous document args to currentDocumentChange event
@peterflynn peterflynn merged commit 3aea013 into master Apr 28, 2014
@peterflynn peterflynn deleted the zaggino/currentDocumentChange branch April 28, 2014 22:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants