Bug 413923 - Add new service APIs that support 2-way communication
Summary: Add new service APIs that support 2-way communication
Status: RESOLVED FIXED
Alias: None
Product: Orion (Archived)
Classification: ECD
Component: Client (show other bugs)
Version: 3.0   Edit
Hardware: PC Windows 7
: P3 normal (vote)
Target Milestone: 4.0 M2   Edit
Assignee: Mark Macdonald CLA
QA Contact:
URL:
Whiteboard:
Keywords: Documentation, noteworthy
Depends on:
Blocks:
 
Reported: 2013-07-29 10:07 EDT by Mark Macdonald CLA
Modified: 2013-09-26 13:08 EDT (History)
5 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Macdonald CLA 2013-07-29 10:07:22 EDT
Orion has an experimental feature enabling extensions to generate a syntax model (AST in other words) for a file type. Other extensions can receive the AST to perform language tooling work. The 2 extension points are:

* orion.edit.syntaxmodel.provider   -- Emits an AST by dispatching a service event containing the AST. 
  (An example that provides an Esprima AST for JavaScript files is in o.e.o.client.ui/web/plugins/jsSyntaxModelPlugin.js)
* orion.edit.syntaxmodel.listener   -- Implements a method that receives AST from a provider. 

However, these are inadequate for doing any real work, because a listener (who usually implements some other primary service like syntaxchecker, contentAssist, or outliner) has no way to request the AST when it's needed. The AST just gets delivered at some indeterminate time when #setSyntaxModel() is called. This makes it impossible to ensure you have an up-to-date AST before doing the primary service work.
Comment 1 Mark Macdonald CLA 2013-07-29 11:06:45 EDT
Idea: A listener could request an AST from the page -- eg. by dispatching a "modelRequest" event on itself. Orion would then consult AST providers to build the AST and deliver it to the requester. (Ideally Orion would also cache the AST and re-use it if no changes have been made to the file since last time.)

Here's a sketch of what a content assist + AST listener service would look like.
> astPromise: null
> 
> computeProposals: function() {
>   astPromise = new Deferred();
>   this.dispatchEvent({ type: "modelRequest" });    // request AST
> 
>   return astPromise.then(function(ast) {
>      // build the content assist proposals here using the AST
>   });
> }
> 
> // Orion calls this in response to a modelRequest event
> setSyntaxModel(ast) {
>   modelPromise.resolve(ast);
> }
Comment 2 Mark Macdonald CLA 2013-07-29 11:59:05 EDT
Cc'ing interested parties who may have better ideas..
Comment 3 Mark Macdonald CLA 2013-08-08 17:32:09 EDT
skaegi suggested for AST receivers to implement a modelUpdated() method to receive AST updates as providers compute them. That's simple enough, but the call order is tricky because a receiver will usually also implement some language tooling service (LTS) -- eg. outline, content assist, validation.

These services want an up-to-date AST before they do the LTS work. For example, before I compute content assist proposals, I want an AST representing the buffer text for which computeProposals() was invoked. 

Without some connection between modelUpdated() and the LTS method call, a provider can't tell if the most recent model it received is current, or if it should defer the work until a new AST has been delivered.


* Possible Options *

1) (see Comment 1) An LTS can workaround "don't call us, we call you" by dispatching a service event on itself, which causes an up-to-date AST to be delivered to that provider. The provider does this every time before fulfilling its LTS method call.

2) Orion wrap every call site of an LTS service -- (for example, when computeProposals() in contentAssist, generateOutline() in outliner, and getProblems() in syntaxchecker) -- with code that consults AST providers and invokes the receivers' modelUpdated() with the results. After that, we go ahead with the LTS service call. 
(Similar idea to the Managed Services contract, but implemented manually.)

3) We add an extra parameter to every LTS service method's signature. The extra param is a Service Object which the LTS can call to receive an up-to-date AST. 
(Service Objects are proposed objects, which can be passed to services, and have async methods that services can invoke. An SO is usable for the duration of a single service method call).
Comment 4 Silenio Quarti CLA 2013-08-09 13:09:39 EDT
Option 3 seems to be the best to me in the long run.

The signature of these services right now are:

provider.computeProposals(buffer, offset, context)

self.registry.getService(validator).checkSyntax(title, contents)

this._outlineService.emitOutline(editor.getText(), editor.getTitle());

And we are working on another one for mark occurrences which is:

occurrencesService.findOccurrences(editor.getText(), selection)

It does not make sense to keep sending the buffer to the service if it is not going to be used. Most services will use the AST.

We could have a service object that can answer three things (the text buffer, the AST and title. The title (fileName) could be passed in the context object instead. The LTS decides which one it needs.

Could we make these signatures the same? Something like:

function serviceXXX(serviceObject, context)
Comment 5 Mark Macdonald CLA 2013-08-09 15:18:34 EDT
(In reply to comment #4)
I like this idea. 

Service Objects would clean up our method signatures. As we expose more editor functionality, we can just add more methods to the editor Service Object that we provide.

Data that is directly related to the service call (and not large or costly to compute) can go directly in the 'context' parameter.
Comment 6 Manu Sridharan CLA 2013-08-22 12:12:58 EDT
Just as a thought: would this API be easily extended to the case where a service needed to provide ASTs for multiple different files?  At some point, at least the content assist will likely have to handle analyzing / indexing multiple files, so it might be good to keep that case in mind.
Comment 7 Mark Macdonald CLA 2013-09-11 14:15:10 EDT
(In reply to comment #6)
> Just as a thought: would this API be easily extended to the case where a
> service needed to provide ASTs for multiple different files?  At some point,
> at least the content assist will likely have to handle analyzing / indexing
> multiple files, so it might be good to keep that case in mind.

My plan for 4.0M2 is to support methods that apply only to the file being edited, starting with these:
 * getAST()
 * getText()

There's nothing technically stopping us from adding more methods that expose other files' ASTs (and other file/directory contents, and so on). But, I don't think we should attempt that before we have some story for limiting plugin permissions. (It's already dodgy that any installed plugin can get the editor contents without explicit authorization, but crawling around other files in your workspace is worse.)

Also, these service APIs are probably not the right place to expose such functionality. skaegi is working on an API allowing plugins to obtain services from the framework. That approach would be cleaner IMO than adding callbacks for various different purposes to our individual service APIs.

So realistically anything beyond the current buffer is likely to be Orion 5.0 work.

[1] https://bugs.eclipse.org/bugs/show_bug.cgi?id=374575
Comment 8 Mark Macdonald CLA 2013-09-11 17:08:02 EDT
Released the first part of the fix. It refactors the existing AST provider API. Providers now implement 1 method named `getModel()` -- will document this on the Orion wiki later. Also the plugin implementing this API for Esprima's AST, is now installed by default. An ASTManager service keeps track of the providers.

Code & tests:
http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=0cb6a4e

Build changes (esprimaAstPlugin minification)
http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=4ce4387
http://git.eclipse.org/c/orion/org.eclipse.orion.server.git/commit/?id=8d91423

Currently working on the 2nd part. It will implement the API changes sketched out in Comment 4. 

I will retain support for the current versions of "orion.edit.contentAssist", "orion.edit.validator", and "orion.edit.outliner" so that existing plugins will not break. The new APIs will take the form of new service methods, and if a service doesn't have the new method, we'll fall back to the old one.
Comment 9 Silenio Quarti CLA 2013-09-11 18:01:40 EDT
Mark, I noticed you are using ContentTypeChanged event in ASTManager, but I have removed it in from InputManager. Use InputChanged instead.
Comment 10 Mark Macdonald CLA 2013-09-12 11:23:10 EDT
(In reply to Silenio Quarti from comment #9)
> Mark, I noticed you are using ContentTypeChanged event in ASTManager, but I
> have removed it in from InputManager. Use InputChanged instead.

Thanks-- that event was actually unnecessary so I removed it.
Comment 11 Mark Macdonald CLA 2013-09-20 18:58:56 EDT
This is now fixed. Here are the final commits that were released (split up roughly by service API):

orion.core.astprovider
======================
* http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=68812cf
> 2013-09-16 | [Bug 413923] Esprima AST plugin: fix exception when serializing errory AST

orion.edit.command
======================
* http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=4914948
> 2013-09-20 | [Bug 413923] add new API for 'orion.edit.command'

orion.edit.contentassist (note lowercase 'a' now recommended)
========================
* http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=38b1ed9
> 2013-09-11 | [Bug 413923] support new API for contentassist providers

* http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=d45cec8
> 2013-09-13 | [Bug 413923] refactor Esprima JS content assist to use common AST [

orion.edit.outliner
======================
* http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=61af006
> 2013-09-17 | [Bug 413923] Make jslint outliner use new API


orion.edit.validator
======================
* http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=f16cf9c
> 2013-09-19 | [Bug 413923] New API for 'orion.edit.validator' -Convert plugins to use

Naming cleanup
======================
* http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=117e680
> 2013-09-19 | [Bug 413923] rename new API methods to remove 'withContext'


The new APIs will be documented in detail for 4.0 on the wiki page: http://wiki.eclipse.org/Orion/Documentation/Developer_Guide/Plugging_into_the_editor
Comment 12 Mark Macdonald CLA 2013-09-26 13:08:50 EDT
Update summary to better reflect what was done in this bug.