Skip to content

Websocket RPC infrastructure#233

Merged
joelrbrandt merged 8 commits intomasterfrom
iwehrman/websocket-rpc
Jul 23, 2014
Merged

Websocket RPC infrastructure#233
joelrbrandt merged 8 commits intomasterfrom
iwehrman/websocket-rpc

Conversation

@iwehrman
Copy link
Copy Markdown
Contributor

This adds Websocket-based RPC infrastructure to Generator. Much of this code originally came from Brackets-shell, but has been refactored and simplified for this use case.

This pull request adds the following methods to the Generator object:

  • Generator.prototype.getCustomOptions - Asynchronously stores a dictionary per-plugin in the Photoshop memory space. Accessible via ExtendScript.
  • Generator.prototype.setCustomOptions- Asynchronously retrieves the aforementioned dictionaries per-plugin.
  • Generator.prototype.startWebsocketServer - Starts the Websocket server per-plugin. The port can either be specified or dyamically assigned.
  • Generator.prototype.stopWebsocketServer - Stops the Websocket server per-plugin.

To test:

  1. Install the generator-rpc-example plugin
  2. Clone the node-connection-example
  3. Run bower install in the node-connection-example directory
  4. Visit file://.../node-connection-example/index.html in Chrome

The node-connection-example relies on a hard-coded port number that's shared between the example plugin and the example client code. Eventually the generator-connection repo will be updated so that Photoshop can (by some mechanism) be used as the intermediary to communicate dynamically assigned ports, but it doesn't do that just yet.

@iwehrman
Copy link
Copy Markdown
Contributor Author

(Also, I'm currently having trouble convincing Travis/NPM to correctly install the ws package when running npm install for some reason. Still investigating...)

@iwehrman
Copy link
Copy Markdown
Contributor Author

Updated to use adobe-photoshop/ws#v0.4.31-no-native.

lib/generator.js Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe the code in this function that does the Promise wrangling will work. However, I'm not totally sure why the complexity is necessary.

For a given pluginId, why can't we just do the following:

  1. If pluginConfig.websocketServerPromise is defined, just return the promise (regardless of the state of the promise).
  2. If it isn't defined, we need to make a new server. Immediately make a new promise, and assign it to websocketServerPromise. Kick off the server creation process, and return the (not yet resolved) promise.
  3. If server creation fails, reject the promise, and then delete pluginConfig.websocketServerPromise. That way, everyone will be notified, and we'll create a new server on the next call to startWebsocketServer
  4. If server creation succeeds, resolve the promise with the port number (and leave pluginConfig.websocketServerPromise set, while synchronously setting pluginConfig.websocketServer). Future calls to startWebsocketServer will return this promise.
  5. When stopping a websocketServer, check if we have pluginConfig.websocketServerPromise set. If it's set and resolved, just delete it. If it's set but not resolved, reject it and delete it. Also delete pluginConfig.websocketServer and start the shutdown sequence. Continue to use a completely separate promise for notifying when shutdown is complete.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One more points to make it appear like I understand this problem.

In step 4, the Promise wouldn't be resolved until the config option was updated. It would be rejected if updating the config option failed (and the websocket server would be shut down).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree the port stuff is unnecessary. I have (what I think is) a better simplification that I'll push.

@joelrbrandt
Copy link
Copy Markdown
Contributor

@iwehrman great work! I've tested on mac and win and all appears well. I've reviewed the new code in generator.js thoroughly -- as soon as you have a moment to give me your thoughts on the promise-wrangling in startWebsocketServer, we're ready to merge. (Note that I probably missed something, so thanks for putting up with me.)

I didn't review the code in the rpc directory too heavily, since a.) I wrote the very first version, and b.) it's been heavily reviewed / tested in Brackets. If there's something you think I should take a look at, let me know.

@joelrbrandt joelrbrandt self-assigned this Jul 23, 2014
@joelrbrandt joelrbrandt added this to the PS 15.2 milestone Jul 23, 2014
@iwehrman
Copy link
Copy Markdown
Contributor Author

I rewrote the websocket server control routines. Now they're perfect!? They're different at the very least. Ready for re-review.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this is the code of gods.

@joelrbrandt
Copy link
Copy Markdown
Contributor

screen shot 2014-07-23 at 2 30 49 pm

Somebody's gettin' a dark green square on their github contribution history today! 👍

joelrbrandt added a commit that referenced this pull request Jul 23, 2014
@joelrbrandt joelrbrandt merged commit 8298581 into master Jul 23, 2014
@joelrbrandt joelrbrandt deleted the iwehrman/websocket-rpc branch July 23, 2014 21:31
@joelrbrandt
Copy link
Copy Markdown
Contributor

Documented this in API changes.

Someday, we might want a wiki page that discusses how to use this.

(For all n, someday we might want a wiki page that discusses how to use n.)

@iwehrman
Copy link
Copy Markdown
Contributor Author

We also still very much need to clean up the node-connection-example repository and pull it into the adobe-photoshop org.

@marekhrabe
Copy link
Copy Markdown
Contributor

So this should be a replacement for usecase of me, creating my own ws server in my plugin, right? I like the idea of dynamically assigned port. Is there a way I can get this assigned port number from jsx in panel?

In my current "generator server + panel frontend" devstack (not yet documented, but I want to do that soon), I'm relying on randomly chosen high port number which is hardcoded in both generator plugin and panel. It would totally fail if the port is already assigned to someone else. It would be cool, if I can "outsource" this port handshake to this API.

How would you also create an express server, which I also need? Could this be (or is it already?) more abstract so that I can only get my guaranteed port number for plugin and do my own thing with that and have the ability to tell panel which port was assigned to my plugin so they can connect?

@iwehrman
Copy link
Copy Markdown
Contributor Author

iwehrman commented Aug 4, 2014

Hey @marekhrabe,
In theory, yes, this should obviate running your own Websocket server. Once the server has been started with startWebsocketServer, Generator stores the port number as an ExtendScript "custom option". You should be able to evaluate some ExtendScript similar to what's found in lib/jsx/getCustomOptions.jsx in your CEP panel. (You'll probably also need to study the source of setCustomOptions to figure out exactly how to do this. Making this simpler is on the ol' todo list.)

Once the Websocket server is started, you can connect to it, define remote functions or events, and execute or handle them using the generator-connection client module. So, for example, your Generator plugin could have a function that starts an express server, and that could be exposed to the client using a remote function definition, which could then be executed by the client in the CEP panel to start the server at the appropriate moment. Here's a simple client example and generator plugin that demonstrates how to use this infrastructure. Both of those repos are a little rough so, again, expect to read some code.

Although we've used this infrastructure previously in Brackets, you could well be the first external client for this in Generator. If you actually do dive in, and if have any questions or suggestions, then please let me or @joelrbrandt know!

@marekhrabe
Copy link
Copy Markdown
Contributor

@iwehrman Thanks for all the links and tips here. The repos are clean enough to be read, I got a lot of knowhow :)

Now I see that I implemented similar concept to this RPC, but using simpler approach with EventEmitters on client and server sides which are interconnected by ws on background. Something like:

// panel - cep
var bridge = new Bridge
bridge.emit('myEvent')

// photoshop backend - generator
var bridge = new Bridge(require('./package.json'), generator)
bridge.on('myEvent', fuction () {
  generator.alert('myEvent received')
})

I will probably change my implementation from hardcoded port number to random unassigned port, start my express+ws server and use get/setCustomOptions to deliver port number to panel. Is app.putCustomOptions in jsx already available in Photoshop.app 15.0 or do I need a beta build for this? I'm currently going to stick with this (as we call it) "bridge" architecture instead of switching to full RPC just yet. One of points is that we don't really want any generator specific code in panels, because we are making them platform-agnostic, capable to run in both Photoshop and Sketch (not the one from Adobe :)) with the exact same code on frontend. I can share this with you if you are interested, once I get some time to cleanup the code and make a little docs here and there

Just one more thing - can you think about better way to get the port number from getCustomOptions rather than polling it once a second or something after you open a panel? Sometimes the panels are opened sooner than our generator plugin is activated and listening and mainly before the port number will be written to memory. Can the options be watched for changes from panel or something like that?

@iwehrman
Copy link
Copy Markdown
Contributor Author

iwehrman commented Aug 4, 2014

@marekhrabe What you're doing sounds perfectly reasonable. I think that getCustomOptions has been around for a while, but I'm not sure off the top of my head. Here's a hint that it's been out in the open at least since January 2013. Maybe our ExtendScript expert @isonno knows at what version it was first introduced? And maybe he also knows if there is some ExtendScript notifier that indicates when custom options have been set? My guess is "no" and that polling is the best you can do for now, but maybe @isonno will enlighten us.

And, yes, if you ever get around to cleaning up your bridge enough to post it somewhere, I'd love to take a look!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants