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

AppInit.appReady fires before document is loaded #1526

Closed
jdiehl opened this issue Aug 30, 2012 · 15 comments
Closed

AppInit.appReady fires before document is loaded #1526

jdiehl opened this issue Aug 30, 2012 · 15 comments
Assignees

Comments

@jdiehl
Copy link
Contributor

jdiehl commented Aug 30, 2012

AppInit.appReady(function () {
    console.log(DocumentManager.getCurrentDocument());
});

will result in:

null

If I remember correctly, brackets.ready behaved differently and waited for the document to load before firing. It is very useful to have an entry point into the app where everything (including the document) is loaded. Currently, the only work-around, I could find, is to attach a handler to DocumentManager.currentDocumentChange and detach it after a little while (this is clearly not a good way to do it).

Could we either get a new ready function that fires when the document is loaded or change appReady accordingly?

@ghost ghost assigned jasonsanjose Aug 31, 2012
@jasonsanjose
Copy link
Member

appReady should behave the same as brackets.ready did previously. appReady is fired after all core brackets modules are loaded and all extension modules are loaded. The change to LoadEvents (before we renamed it to AppInit is in pull request #1425. Is there something else I'm missing?

@peterflynn
Copy link
Member

@jdiehl, I'm curious what makes the initially open document special? currentDocument could change again at any time, so won't you have to listen to currentDocumentChange and detach/reattach listeners anyway?

Also, bear in mind there's no guarantee there will be any currentDocument on load -- sometimes, the user has no documents open at all when they re-launch Brackets.

@jdiehl
Copy link
Contributor Author

jdiehl commented Sep 1, 2012

@jason-sanjose, I haven't tested this extensively - it just didn't occur with the earlier builds (luck?)

@peterflynn, this is needed if your initialization code depends on the current document but you do not want that code to be triggered multiple times. Live-Development's autoconnect function uses this and the work-around is fairly ugly (see commit 35a1071).

Maybe this is a border-case, and there is no need to change this.

@pthiess
Copy link
Contributor

pthiess commented Sep 4, 2012

Reviewed - labeled medium priority since it was recently added.

@peterflynn
Copy link
Member

@jdiehl: I think I'm still confused :-) If your code depends on the current document, your initialization would look something like this, right?

function attachToCurrentDocument() {
    ...
}

// Initialize
if (DocumentManager.getCurrentDocument()) {
    attachToCurrentDocument();
}
$(DocumentManager).on("currentDocumentChange", function () {
    attachToCurrentDocument();
});

Would anything need to change about that code if your init runs before the first "current document" is loaded? It seems like it'd still work fine as-is. (You could maybe even simplify it by removing the 'if' in that case, but it seems safer not to bank on that).

@jdiehl
Copy link
Contributor Author

jdiehl commented Sep 5, 2012

Are you 100% certain that currentDocumentChange always fires after appReady? I remember vaguely that I had problems with that in the past.

@peterflynn
Copy link
Member

If you're seeing a case where currentDocumentChange, doesn't fire when something is opened, definitely file a bug. (The only case you shouldn't get that event during app load is when there is no initially open document).

@DennisKehrig
Copy link
Contributor

@peterflynn The idea seems to be that if Brackets starts with an open document (because it was open when you closed Brackets the last time), live preview should automatically be activated again (assuming it was active when you exited). However, this activation needs to wait until that file is actually re-opened by Brackets, and should not happen for files manually opened some time after launch.
In other words there needs to be a way to distinguish restored files (from a previous session) from manually opened files (in the current session). Previously Brackets fired the ready event when it was completely done with automatic events like restoring documents and loading extensions, i.e. everything after that would be iniated by the user. Now this is no longer the case.

@jdiehl: Even if the ready event worked like before, it is unclear what should happen if Brackets is launched with a file as a parameter (like mate foo.js, subl foo.js, etc.). I think currently this isn't possible but I think in this case the ready event should also fire AFTER the file was loaded. But then it's not the file last used for the live preview, so automatically reestablishing live preview would be inappropriate.

@DennisKehrig
Copy link
Contributor

I think

_initExtensions().always(AppInit._dispatchReady(AppInit.APP_READY));

should be

_initExtensions().always(function () {
    AppInit._dispatchReady(AppInit.APP_READY)
});

The way it is now, _dispatchReady will be run BEFORE the extensions are loaded and the return value of _dispatchReady will be run afterwards. But that return value is always undefined.

@DennisKehrig
Copy link
Contributor

Alright, so in brackets.js, ProjectManager.openProject is called, returning a promise. When that promise is done, extensions are loaded, and afterwards the ready event should be fired.

Just prior to the promise being resolved, ProjectManager triggers the projectOpen event. The DocumentManager listens to that, restoring the working set for that project. Since that is an asynchronous operation, the project manager's promise is likely resolved BEFORE the documents (and therefore the active document) are loaded.

So it was only by coincidence that the document was loaded when brackets.ready was fired. Roughly speaking, this happens when loading the extensions takes longer than loading the active document. In other words: it was never completely reliable.

#1854 restores the intended behavior of the ready event. I leave it up to you guys to decide whether that is good enough or whether we should add an event to detect when basically restoring the previous Brackets state is complete.

@jasonsanjose
Copy link
Member

Confirmed fixed. Assigning to @jdiehl to close.

@jdiehl
Copy link
Contributor Author

jdiehl commented Oct 18, 2012

Works like a charm, thanks for fixing this!

@DennisKehrig
Copy link
Contributor

You're welcome!

@peterflynn
Copy link
Member

Note: per @DennisKehrig's comment, this is actually not fixed -- just much less likely due to a shift in timing. You'll only hit this bug if restoring the working set (in particular the initially open document) takes longer than loading all extensions, since the two now happen simultaneously.

@peterflynn
Copy link
Member

I've filed #1951 about the potential confusion with the ordering of startup (including the race condition here).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants