Added Reverse Inspect in Live Preview using WebSockets, and now forwa… #13044

saurabh95 · 2017-01-17T04:39:50Z

Now you can click on any element in Live Preview and then corresponding HTML element will be highlighted and the cursor in code view will shift to the corresponding HTML element.

Features that need to be incorporated in this PR

Get Socket Port Number from preferences.
Add commands to create and destroy server and create server should be called only when user clicks on Live Preview button.

I am currently working on these. Will update the PR soon.

…rd inspect brings the html element in focus to the viewport in Live Preview

…eview and also now port is read from preferences file

ficristo

This seems to have broken MultiBrowserLivePreview. At least using Firefox does throw some errors.
Also I thought that standard LivePreview was sort of deprecated and we should not add features to it.

ficristo · 2017-01-17T09:58:23Z

src/LiveDevelopment/Agents/RemoteAgent.js

@@ -130,7 +131,7 @@ define(function RemoteAgent(require, exports, module) {
        _stopKeepAliveInterval();

        // inject RemoteFunctions
-        var command = "window._LD=" + RemoteFunctions + "(" + LiveDevelopment.config.experimental + ");";
+        var command = "window._LD=" + RemoteFunctions + "(" + LiveDevelopment.config.experimental + "," + PreferencesManager.get("livedev.wsPort") + ");";


#12949 will change this to pass a config object, maybe you can already change it.

I have run all tests related to Live Preview, and now MultiBrowserPreview is also working fine.
Regarding passing of config object, once #12949 is merged, I will do the respective changes.

ficristo · 2017-01-17T10:01:16Z

src/LiveDevelopment/Agents/RemoteFunctions.js

@@ -319,6 +319,10 @@ function RemoteFunctions(experimental) {
            if (this.trigger) {
                _trigger(element, "highlight", 1);
            }
+
+            if (!window.event) {
+                element.scrollIntoViewIfNeeded();


According to the current version of MDN, this function is supported only in Chrome, Opera and Safari.
I'm not familiar with LivePreview but this file is injected only in case of LivePreview with Chrome, or it is also used with MultiBrowserLivePreview?
In the second case, is it possible to change it to use scrollIntoView instead?

This script is injected in case of MultiBrowserLivePreview, for now the reverse-inspect doesn't work with the multibrowser, I will take up this task once this PR is merged.(I tried a hacky solution to make it work for multibrowser and it was working. I will raise another PR once I am done with proper solution)
Now, I am using window.scrollTo instead of scrollIntoViewIfNeeded.
I didn't use scrollIntoView because it aligns the element either to top or bottom, but I wanted to align it to center, so I have now used window.scrollTo which is supported by all browsers.

ficristo · 2017-01-17T10:03:20Z

src/LiveDevelopment/Agents/RemoteFunctions.js

+
+        _ws.onmessage = function (evt) {
+            var received_msg = evt.data;
+        };


If you don't use it, remove it?

ficristo · 2017-01-17T10:05:42Z

src/LiveDevelopment/LiveDevelopment.js

+
+    PreferencesManager.definePreference("livedev.wsPort", "number", 8125, {
+        description: Strings.DESCRIPTION_LIVEDEV_WEBSOCKET_PORT
+    });


Since you are doing for LiveDevelopment, maybe you should do the same for MultiBrowserLivePreview too. See also #11957

Yeah once this is completed, I will try to do the same for multibrowser also.

ficristo · 2017-01-17T10:06:04Z

src/LiveDevelopment/transports/WebSocketTransport.js

+// This transport provides a WebSocket connection between Brackets and a live browser preview.
+// This is just a thin wrapper around the Node extension (WebSocketTransportDomain) that actually
+// provides the WebSocket server and handles the communication. We also rely on an injected script in
+// the browser for the other end of the transport.


This probably should be a JSDoc.

ficristo · 2017-01-17T10:09:43Z

src/LiveDevelopment/transports/node/WebSocketTransportDomain.js

+        false,                      // this command is synchronous in Node
+        "Creates the WS server",
+        [
+            {name: "port", type: "number", description: "Port on which server needs to listen"}


Put every param on its own line.

ficristo · 2017-01-17T10:10:42Z

src/LiveDevelopment/transports/node/package.json

+{
+  "name": "brackets-livedev-server",
+  "dependencies": {
+    "ws": "~0.4.31"


I think you can put it on the root package.json and update the shrinkwrap file.
Right now, I don't think this works.

Yeah, right now we have to run npm install inside the node directory, to run this.
I am trying to figure out a way to move this to root package.json as suggested by you.

ficristo · 2017-01-17T10:11:23Z

src/language/HTMLInstrumentation.js

+        var marks = editor._codeMirror.getAllMarks(),
+            i,
+            markFound,
+            position;


position is not used.

ficristo · 2017-01-17T10:11:44Z

src/language/HTMLInstrumentation.js

+            markFound,
+            position;
+
+        for (i = 0; i < marks.length; i++) {


I think you can replace this for with lodash

ficristo · 2017-01-17T10:12:24Z

src/language/HTMLInstrumentation.js

+        }
+        if (markFound) {
+            return markFound.find().from;
+        }


If markFound can be false maybe you should add a return value, otherwise add a console.error or something.

ficristo · 2017-01-17T10:38:06Z

I'm not really familiar with LivePreview to judge the approach. However I left some comments.

petetnt · 2017-01-17T12:13:34Z

src/LiveDevelopment/transports/node/WebSocketTransportDomain.js

+            {name: "port", type: "number", description: "Port on which server needs to listen"}
+        ],
+        []
+    );


NIt: newline here

petetnt · 2017-01-17T12:17:16Z

src/LiveDevelopment/Agents/RemoteFunctions.js

+    function onDocumentClick(event) {
+        var element = event.target,
+            currentDataId,
+            newDataId;


Nit: newline here

petetnt · 2017-01-17T12:18:37Z

src/LiveDevelopment/LiveDevelopment.js

@@ -1362,6 +1369,7 @@ define(function LiveDevelopment(require, exports, module) {
            // wait for server (StaticServer, Base URL or file:)
            prepareServerPromise
                .done(function () {
+                    WebSocketTransport.createWebSocketServer(PreferencesManager.get("livedev.wsPort"));


See @ficristo's comment above about the MultiBrowserLivePreview 👍

petetnt · 2017-01-17T12:19:18Z

src/LiveDevelopment/transports/node/WebSocketTransportDomain.js

+        [
+            {name: "msg", type: "string", description: "JSON message from client page"}
+        ]
+    );


Nit: Newline here

petetnt · 2017-01-17T12:19:22Z

src/LiveDevelopment/transports/node/WebSocketTransportDomain.js

+    _domainManager = domainManager;
+    if (!domainManager.hasDomain("webSocketTransport")) {
+        domainManager.registerDomain("webSocketTransport", {major: 0, minor: 1});
+    }


Nit: Newline here

petetnt · 2017-01-17T12:19:57Z

Left some small comments too, looking good so far 👍

redmunds · 2017-01-17T15:39:57Z

src/LiveDevelopment/Agents/RemoteFunctions.js

+        if (element && element.hasAttribute('data-brackets-id')) {
+            _ws.send(JSON.stringify({
+                type: "message",
+                message: element.getAttribute('data-brackets-id')


The message is just the element id, correct? When sending messages in other direction (from Brackets to browser), the message has the format:

{ method: method, id: id, params: params }

It would be nice to do the same here so that's it's easier to add any new messages in the future.

Actually in multi-browser also we are sending the message from browser to brackets in the same format(NodeSocketTransportRemote.js), that's why I also used the same format, so that in future when we implement the same thing for multi-browser, we can have the same message to interpret in the brackets side.

redmunds · 2017-01-17T15:41:51Z

src/LiveDevelopment/transports/WebSocketTransport.js

@@ -0,0 +1,64 @@
+/*
+ * Copyright (c) 2014 - present Adobe Systems Incorporated. All rights reserved.


Copyright date for new files should be year that file was added (2017).

…ViewIfNeeded, added a check for port number in RemoteFunctions and some minor changes

saurabh95 · 2017-01-18T08:33:31Z

I have fixed some issues related to MultiBrowserLivePreview as RemoteFunctions.js was also included in MultiBrowserLivePreview. I have run the tests also, on both Firefox and Chrome.

ficristo · 2017-01-18T09:43:59Z

src/LiveDevelopment/Agents/RemoteFunctions.js

@@ -22,15 +22,15 @@
 */

 /*jslint forin: true */
-/*global Node */
+/*global Node, document */


Use window.document instead of adding it as a global.

ficristo · 2017-01-18T09:47:25Z

src/LiveDevelopment/Agents/RemoteFunctions.js


+    window.document.addEventListener("click", onDocumentClick);


I think it is better to move this call in the onopen callback below and
add a removeEventListener in the onclose callback.
Otherwise the handler will be called also when the LivePreview is terminated.

…ctions.js

ficristo · 2017-01-19T12:41:13Z

There are still a couple of unanswered comments.
Also in HTMLInstrumentation.js can you use lodash instead of the for.
_.find seems what you need.
Finally, you should check the tests and maybe add one?

saurabh95 · 2017-01-21T18:01:15Z

@ficristo, I just uploaded new changes, regarding the testing part, I was thinking of writing tests once I am done with MultiBrowserLivePreview as well(I am almost done with it), then I will write tests for both of them.
I have run the current LivePreview tests, and all are passing.

@petetnt @zaggino @marcelgerber @abose Please review

…h instead of for loop in HTMLInstrumentation.js

ficristo · 2017-01-24T14:46:51Z

This does not work on distribution.
To verify

run grunt build
open Brackets with the Shift keyword pressed
select the index.html under dist/ folder

You have to adjust the copy task in Gruntfile.js

saurabh95 · 2017-01-25T07:17:39Z

Thanks @ficristo for pointing out. I have fixed this and also updated the PR :)

ficristo · 2017-01-25T10:42:13Z

I looked enough at this so approving but I see some failures on Live Preview tests, so please take a look.

saurabh95 · 2017-01-25T13:58:26Z

@ficristo Actually there is some problem with the timeout of the Live Preview tests, I increased the timeout and all the Live Preview tests are passing.

ficristo · 2017-02-10T13:34:44Z

@saurabh95 the increase of timeout should be part of this PR?

saurabh95 · 2017-02-10T14:53:21Z

@ficristo Actually without my changes also some LivePreview tests(the same ones) were failing and on increasing timeout they were passing, so I thought we can take change it in some other set of changes, as it is independent of this change?
Anyway I will be taking up the task to write tests for my changes, we can then incorporate the changes related to timeout there itself.

petetnt

LGTM with few comments

petetnt · 2017-02-13T22:24:11Z

src/LiveDevelopment/Agents/RemoteFunctions.js

@@ -319,6 +336,14 @@ function RemoteFunctions(experimental) {
            if (this.trigger) {
                _trigger(element, "highlight", 1);
            }
+
+            if (!window.event && !isInViewport(element)) {


Question: not sure why the window.event check is needed here? Something to do with legacy IE's?

I am checking if there is any window event (mostly click event) is there on the LivePreview instance, then we don't need to bring the element in viewport, as it is already present in the viewport.

petetnt · 2017-02-13T22:26:10Z

src/config.json

@@ -38,7 +38,8 @@
    "dependencies": {
        "anymatch": "1.3.0",
        "chokidar": "1.6.0",
-        "lodash": "4.15.0"
+        "lodash": "4.15.0",


Lodash has 4.17.4 out now, but feel free to left as is

swmitra

Good to be merged.

swmitra · 2017-02-14T05:51:36Z

Great work @saurabh95. This is indeed a great addition, keep more coming 👍

Added Reverse Inspect in Live Preview using WebSockets, and now forwa…

7ac32b9

…rd inspect brings the html element in focus to the viewport in Live Preview

saurabh95 requested review from zaggino, marcelgerber, abose, petetnt and ficristo January 17, 2017 04:49

Added commands to start and close the WebSocketServer used in Live Pr…

8c141ce

…eview and also now port is read from preferences file

ficristo suggested changes Jan 17, 2017

View reviewed changes

petetnt reviewed Jan 17, 2017

View reviewed changes

petetnt suggested changes Jan 17, 2017

View reviewed changes

redmunds reviewed Jan 17, 2017

View reviewed changes

Addressed review comments- used window.scrollTo instead of scrollInto…

06b5418

…ViewIfNeeded, added a check for port number in RemoteFunctions and some minor changes

ficristo reviewed Jan 18, 2017

View reviewed changes

saurabh95 added 2 commits January 18, 2017 17:14

Added dependency of ws in the root directory

71c4148

Used window.document instead of defining global document in RemoteFun…

dbe1329

…ctions.js

Added JSDoc description in WebSocketTransportDomain.js and used lodas…

aae3943

…h instead of for loop in HTMLInstrumentation.js

saurabh95 force-pushed the saurabh95/ReverseInspect branch from 54e1e83 to aae3943 Compare January 21, 2017 18:30

Added copy task of copying LiveDevelopment/transports to dist

819d122

ficristo approved these changes Jan 25, 2017

View reviewed changes

saurabh95 added this to the Release 1.9 milestone Feb 3, 2017

saurabh95 added the F Live Preview label Feb 3, 2017

petetnt approved these changes Feb 13, 2017

View reviewed changes

swmitra approved these changes Feb 14, 2017

View reviewed changes

swmitra merged commit 8b1ff4b into master Feb 14, 2017

saurabh95 deleted the saurabh95/ReverseInspect branch February 23, 2017 12:47

saurabh95 mentioned this pull request Mar 1, 2017

Feature suggestion - 'reverse live view': link from browser to editor #12815

Closed

This was referenced Aug 30, 2021

[CLOSED] Feature suggestion - 'reverse live view': link from browser to editor brackets-archive/bracketsIssues#10913

Open

[CLOSED] Added Reverse Inspect in Live Preview using WebSockets, and now forwa… brackets-archive/bracketsIssues#11085

Open

		@@ -0,0 +1,64 @@
		/*
		* Copyright (c) 2014 - present Adobe Systems Incorporated. All rights reserved.

Added Reverse Inspect in Live Preview using WebSockets, and now forwa… #13044

Added Reverse Inspect in Live Preview using WebSockets, and now forwa… #13044

Conversation

saurabh95 commented Jan 17, 2017 • edited by boopeshmahendran Loading

ficristo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ficristo Jan 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ficristo commented Jan 17, 2017

Choose a reason for hiding this comment

petetnt Jan 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petetnt commented Jan 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saurabh95 commented Jan 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ficristo commented Jan 19, 2017

saurabh95 commented Jan 21, 2017 • edited Loading

ficristo commented Jan 24, 2017

saurabh95 commented Jan 25, 2017

ficristo commented Jan 25, 2017

saurabh95 commented Jan 25, 2017

ficristo commented Feb 10, 2017

saurabh95 commented Feb 10, 2017 • edited Loading

petetnt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swmitra left a comment

Choose a reason for hiding this comment

swmitra commented Feb 14, 2017

saurabh95 commented Jan 17, 2017 •

edited by boopeshmahendran

Loading

ficristo Jan 17, 2017 •

edited

Loading

petetnt Jan 17, 2017 •

edited

Loading

saurabh95 commented Jan 21, 2017 •

edited

Loading

saurabh95 commented Feb 10, 2017 •

edited

Loading