If live dev page leaves site, then terminate live dev #3141

Merged
merged 3 commits into from Mar 18, 2013

Conversation

Projects
None yet
4 participants
@redmunds
Contributor

redmunds commented Mar 14, 2013

This is a fix for #2277.

Note that this bug says that element highlighting should stop when another url is entered, but it seems like we should allow other urls as long as user stays within site. Once site is left, then highlighting is stopped.

Also, it only says that highlighting should stop, but it seems like everything should stop, so the live dev connection is killed.

This feels kind of quick & dirty, so I am curious what I'm missing here. If this is a good enough starting point, then I'll add an integration test.

@njx

This comment has been minimized.

Show comment Hide comment
@njx

njx Mar 14, 2013

Member

Took a quick look. I think the behavior makes sense (at least until/unless we redefine the overall live dev workflow anyway)--we don't currently really define what should happen if you navigate within the browser anyway.

The basic idea of your implementation seems fine too, but looking at the code, it looks like you might want to call close() instead of _closeDocument() if you really want to simulate the effect of the user shutting off live development completely. It looks like _closeDocument() doesn't actually close the connection--it just cleans up some stuff in preparation for opening another document on the same connection.

Member

njx commented Mar 14, 2013

Took a quick look. I think the behavior makes sense (at least until/unless we redefine the overall live dev workflow anyway)--we don't currently really define what should happen if you navigate within the browser anyway.

The basic idea of your implementation seems fine too, but looking at the code, it looks like you might want to call close() instead of _closeDocument() if you really want to simulate the effect of the user shutting off live development completely. It looks like _closeDocument() doesn't actually close the connection--it just cleans up some stuff in preparation for opening another document on the same connection.

@njx

This comment has been minimized.

Show comment Hide comment
@njx

njx Mar 14, 2013

Member

Actually, @iwehrman points out that close() might not be right either, because in this case you don't want to actually close the tab. You do want to disconnect the connection, though; and I'm not sure exactly what's responsible for closing the tab, so I don't know yet how to decouple the two.

Ian is in the middle of putting up some changes to how all this stuff works, so I think we should wait for that to land (or know what it looks like) before getting further into fixing this issue.

Member

njx commented Mar 14, 2013

Actually, @iwehrman points out that close() might not be right either, because in this case you don't want to actually close the tab. You do want to disconnect the connection, though; and I'm not sure exactly what's responsible for closing the tab, so I don't know yet how to decouple the two.

Ian is in the middle of putting up some changes to how all this stuff works, so I think we should wait for that to land (or know what it looks like) before getting further into fixing this issue.

@njx

This comment has been minimized.

Show comment Hide comment
@njx

njx Mar 14, 2013

Member

Oh, actually, the bit that closes the tab is the first if statement in close(). So my guess is that what you actually want to do is just skip that bit, but do the rest of the bits. Still, let's wait for @iwehrman's changes to land.

Member

njx commented Mar 14, 2013

Oh, actually, the bit that closes the tab is the first if statement in close(). So my guess is that what you actually want to do is just skip that bit, but do the rest of the bits. Still, let's wait for @iwehrman's changes to land.

@redmunds

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Mar 14, 2013

Contributor

Updated code that terminates live dev connection.

Contributor

redmunds commented Mar 14, 2013

Updated code that terminates live dev connection.

@iwehrman

This comment has been minimized.

Show comment Hide comment
@iwehrman

iwehrman Mar 14, 2013

Contributor

My proposed changes are in my repo on branch iwehrman/live-development-startup-refactoring. Each of the five commits fixes an independent issue, as described in the commit messages.

Contributor

iwehrman commented Mar 14, 2013

My proposed changes are in my repo on branch iwehrman/live-development-startup-refactoring. Each of the five commits fixes an independent issue, as described in the commit messages.

@redmunds

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Mar 14, 2013

Contributor

@iwehrman Those changes look awesome! I also put up pull request #3134 to change the unit (integration) tests to use LiveDevelopment.status instead of Inspector.connected() to test status, but I don't think it conflicts with any of your proposed changes.

Contributor

redmunds commented Mar 14, 2013

@iwehrman Those changes look awesome! I also put up pull request #3134 to change the unit (integration) tests to use LiveDevelopment.status instead of Inspector.connected() to test status, but I don't think it conflicts with any of your proposed changes.

@iwehrman

This comment has been minimized.

Show comment Hide comment
@iwehrman

iwehrman Mar 14, 2013

Contributor

Yep, that looks like the right change to make there. (Although one of the aforementioned commits would probably also mitigate the problem by making Inspector.connected() return false until the socket is actually ready.)

Contributor

iwehrman commented Mar 14, 2013

Yep, that looks like the right change to make there. (Although one of the aforementioned commits would probably also mitigate the problem by making Inspector.connected() return false until the socket is actually ready.)

@ghost ghost assigned gruehle Mar 15, 2013

@@ -440,6 +440,7 @@ define(function LiveDevelopment(require, exports, module) {
/** Triggered by Inspector.connect */
function _onConnect(event) {
$(Inspector.Inspector).on("detached", _onDetached);
+ $(Inspector.Page).on("frameNavigated.DOMAgent", _onFrameNavigated);

This comment has been minimized.

Show comment Hide comment
@gruehle

gruehle Mar 15, 2013

Member

This line is causing JSLint to be unhappy since _onFrameNavigated hasn't been defined yet.

@gruehle

gruehle Mar 15, 2013

Member

This line is causing JSLint to be unhappy since _onFrameNavigated hasn't been defined yet.

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Mar 17, 2013

Contributor

Fixed.

@redmunds

redmunds Mar 17, 2013

Contributor

Fixed.

+ // No longer in site, so terminate live dev, but don't close browser window
+ Inspector.disconnect();
+ _setStatus(STATUS_INACTIVE);
+ _serverProvider = null;

This comment has been minimized.

Show comment Hide comment
@gruehle

gruehle Mar 15, 2013

Member

These same 3 lines are duplicated lines 637-639. These should be factored into a new function.

@gruehle

gruehle Mar 15, 2013

Member

These same 3 lines are duplicated lines 637-639. These should be factored into a new function.

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Mar 17, 2013

Contributor

That code is refactored in Ian's branch, so I'll fix this after his branch is merged.

@redmunds

redmunds Mar 17, 2013

Contributor

That code is refactored in Ian's branch, so I'll fix this after his branch is merged.

@gruehle

This comment has been minimized.

Show comment Hide comment
@gruehle

gruehle Mar 15, 2013

Member

Initial review complete.

Member

gruehle commented Mar 15, 2013

Initial review complete.

@gruehle

This comment has been minimized.

Show comment Hide comment
@gruehle

gruehle Mar 15, 2013

Member

Hmmm.. Live Development is broken with this change. If I click the lightning bolt, the page opens, but the connection is immediately dropped (and the lightning bolt turns gray).

Could this be caused by the interstitial page?

Member

gruehle commented Mar 15, 2013

Hmmm.. Live Development is broken with this change. If I click the lightning bolt, the page opens, but the connection is immediately dropped (and the lightning bolt turns gray).

Could this be caused by the interstitial page?

@redmunds redmunds closed this Mar 15, 2013

@redmunds redmunds reopened this Mar 16, 2013

@redmunds

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Mar 16, 2013

Contributor

Ooops. Clicked wrong button. Didn't mean to close pull request.

Contributor

redmunds commented Mar 16, 2013

Ooops. Clicked wrong button. Didn't mean to close pull request.

@redmunds

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Mar 16, 2013

Contributor

Seems to work for me on both Win7 and Mac 10.8 using the Citrus Cafe pages used by the smoke tests.

Did you update & rebuild brackets-shell for the latest changes?

Which page are you using? Are you using the node http server? If not, what?

Contributor

redmunds commented Mar 16, 2013

Seems to work for me on both Win7 and Mac 10.8 using the Citrus Cafe pages used by the smoke tests.

Did you update & rebuild brackets-shell for the latest changes?

Which page are you using? Are you using the node http server? If not, what?

@gruehle

This comment has been minimized.

Show comment Hide comment
@gruehle

gruehle Mar 16, 2013

Member

I'm using the lastest brackets-shell code on OSX 10.8. The Getting Started and Citrus Complete projects are both working fine, but one other project I have isn't. I'll email the project to you.

Member

gruehle commented Mar 16, 2013

I'm using the lastest brackets-shell code on OSX 10.8. The Getting Started and Citrus Complete projects are both working fine, but one other project I have isn't. I'll email the project to you.

@redmunds

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Mar 17, 2013

Contributor

Live Development is broken with this change. If I click the lightning bolt, the page opens,
but the connection is immediately dropped (and the lightning bolt turns gray).
Could this be caused by the interstitial page?

Good catch! It was caused by an iframe. Fixed.

Contributor

redmunds commented Mar 17, 2013

Live Development is broken with this change. If I click the lightning bolt, the page opens,
but the connection is immediately dropped (and the lightning bolt turns gray).
Could this be caused by the interstitial page?

Good catch! It was caused by an iframe. Fixed.

@gruehle

This comment has been minimized.

Show comment Hide comment
@gruehle

gruehle Mar 18, 2013

Member

Works great! Merging.

Member

gruehle commented Mar 18, 2013

Works great! Merging.

gruehle added a commit that referenced this pull request Mar 18, 2013

Merge pull request #3141 from adobe/randy/live-dev-nav
If live dev page leaves site, then terminate live dev

@gruehle gruehle merged commit f3419c1 into master Mar 18, 2013

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment