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

Fix Live Preview for Docs not in Working Set #8605

Merged
merged 6 commits into from
Aug 6, 2014
Merged

Conversation

redmunds
Copy link
Contributor

The simple recipe in #7886 uncovers a big design flaw in live preview where docs not in the working set don't work. I suspect that many reported problems and open issues are related to this subtle difference.

The issue is that live preview caches HTMLDocument objects, but the Document and Editor objects that it references are only maintained when doc is in the working set. When doc is not in the working set, a new Document and Editor object is created every time it is selected in project tree.

This PR makes 2 changes:

  1. HTMLDocument now ref counts this.doc Document so it persists while Live Preview is connected.
  2. HTMLDocument now listens for activeEditorChange event to handle Editor changes. This includes proper updating of event listeners and HTMLInstrumentation.

@JeffryBooher JeffryBooher self-assigned this Aug 4, 2014
@JeffryBooher
Copy link
Contributor

@redmunds can you try this with the jeff/splitview-1x2 branch to get an idea if anything will break? I have a feeling that some of the functionality is not going to work.

@JeffryBooher
Copy link
Contributor

Ooops. I meant to remove that line about having the feeling that it not working. I reviewed the code at a cursory level and it look like it shouldn't be a problem with splitivew but you wanna give it a try?

@redmunds
Copy link
Contributor Author

redmunds commented Aug 4, 2014

@JeffryBooher Good point -- using the jeff/splitview-1x2 branch, there are new scenarios that are not possible in master.

When both the HTML and CSS files are open in different views, and the HTML doc is not in the "Working Set" (or whatever new terminology is). The bug doesn't happen when focus switches to the CSS doc such that the HTML doc is still displayed in the other view because both the Editor and Document objects are maintained for the HTML doc until it gets focus back.

But, when documents and focus changes are arranged such that the HTML doc is no longer displayed in either view, and it is then reselected, then the issue remains.

@JeffryBooher
Copy link
Contributor

@redmunds I think in the splitview case you need to listen to the new editor event deleted which will tell you when an editor is being destroyed. That happens when a temporary view (an editor open in splitview but not in the 'workingset') is destroyed as well as when editors are closed via File > Close, File > Close All, etc...

var self = this;
this.editor = editor;

if (this.editor) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved this check from all the the ctor to here because it seemed safer to always do it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

you still don't need it. attachEditor is only called when it has an editor and the docs for the ctor say that the editor argument is required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@JeffryBooher
Copy link
Contributor

done with review

@redmunds
Copy link
Contributor Author

redmunds commented Aug 4, 2014

in the splitview case you need to listen to the new editor event deleted ...

I see the beforeDestroy event -- that's probably what you are referring to. That change can't be done in master, so I guess we'll have to keep issue #7886 open (or open a new issue with new recipe) until this also gets fixed for Split View.

@redmunds
Copy link
Contributor Author

redmunds commented Aug 4, 2014

Pushed changes. Ready for another review.

@JeffryBooher
Copy link
Contributor

ah, right beforeDestroy that used to be destroy but was changed per code review feedback.

self._onChange(event, editor, change);
});

// Experimental code
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this is still experimental since it's been in there and working for quite some time now :)

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, this is still experimental code. The LiveDevelopment experimental highlighting is different from Live Preview Highlighting -- I think that's the code that allows you to click in browser and highlight in brackets.

With that said, all of the LiveDevelopment experimental code is so old that it probably no longer works and should be ripped out.

@JeffryBooher
Copy link
Contributor

Done with review. Just a few final cleanup items to consider and this is ready to merge.

@redmunds
Copy link
Contributor Author

redmunds commented Aug 6, 2014

Ready for another review.

@JeffryBooher
Copy link
Contributor

LGTM. Merging.

JeffryBooher added a commit that referenced this pull request Aug 6, 2014
Fix Live Preview for Docs not in Working Set
@JeffryBooher JeffryBooher merged commit 45bbc38 into master Aug 6, 2014
@JeffryBooher JeffryBooher deleted the randy/issue-7886 branch August 6, 2014 21:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants