Please review.

Looks good to me.
I'd only rename OpenUriHandler.handle(URI) to OpenUriHandler.open(URI).

Created attachment 150075 [details]
Proposed Patch v2

> I'd only rename OpenUriHandler.handle(URI) to OpenUriHandler.open(URI).
Renamed. Thank you, Martin.

In the current design the api is not really about communicating some intent, but facilitates opening uris. Creating new interfaces and their implementations for every intent we want to communicate, as proposed by the documentation, will result in a very big module from which clients only use the "intent" they introduced.

R01: Can we make the api more generic? Or is there an intent object from a third-party library we can use?

If we want to stay only with URI intents and no additional data sent to from intent it's OK tom me. The URI intent is probably enough for output. We want to use the Intent for things like missing main class, app server and other UI stuff currently done in ActionProvider. It's OK for this as well.

However if you want to make the generic Intent API. The Intent is rather an Event (possibly parametrised) sent to global channel by source which is processed by a registered sink.
You can look at Android Intent API allowing protocol, contentType, class or action events which can transfer parameters and possibly return values. Here is a good Android Intent API introduction: http://www.vogella.com/tutorials/AndroidIntent/article.html

(In reply to Ralph Ruijs from comment #4)
> In the current design the api is not really about communicating some intent,
> but facilitates opening uris.
Yes, this is currently the only goal of this module.

I considered three possibilities:
 1) Use a more descriptive name for the module (e.g. api.openuri, api.uri.actions)
 2) Make its name more generic, so that similar APIs/SPIs for requesting and 
    handling of some operations can be added in the future (something with 
    similar purpose as Intent class in Android).
 3) Find an existing module where to put the classes.

I weren't able to find any suitable module, and #2 seemed slightly better than #1 to me. But #1 seems also quit fine to me if you prefer it.

(In reply to Tomas Zezula from comment #5)
> If we want to stay only with URI intents and no additional data sent to from
> intent it's OK tom me. The URI intent is probably enough for output. We want
> to use the Intent for things like missing main class, app server and other
> UI stuff currently done in ActionProvider. It's OK for this as well.

It is probably OK for the ActionProvider stuff, but the usage wouldn't be very expressive, right?

> However if you want to make the generic Intent API. The Intent is rather an
> Event (possibly parametrised) sent to global channel by source which is
> processed by a registered sink.
> You can look at Android Intent API allowing protocol, contentType, class or
> action events which can transfer parameters and possibly return values. Here
> is a good Android Intent API introduction:
> http://www.vogella.com/tutorials/AndroidIntent/article.html

Jarda; the Android Intent API was the only one I could find quickly as well (although I expected to find a more generic library). Is there a reason not to shape the api towards the Android Intent classes?

> Jarda; the Android Intent API was the only one I could find quickly as well
> (although I expected to find a more generic library). Is there a reason not
> to shape the api towards the Android Intent classes?
No particular reason. I just did not have any use case for which URI opening is not sufficient. (The most common use case is opening file in editor using URI like "file:///path/to/file?line=123"). So I just chose generic module name so that we can add more sophisticated API later. But we can make it more generic directly.

In reply to Ralph:
>...usage wouldn't be very expressive, right?
Yes, the usage will be quite simple.
The thing is that the NB ActionProvider is an UI class which shows dialogs and expects response from the user, for example select active app server. On server side the ActionProvider or the replacement of ActionProvider will just emit an special URI to the console which shows in a browser a link, for example: Missing_Application_Server. When clicked the code on client or server side will handle it by action, for example let user select available app server.


In Reply to Jarda:
I vote for #2 but I would do these changes for easier extensibility in the future.
1st) Add a Intent class.

final class Intent<R> {
     static Intent forURI(URI){}
}

In future the Intent can have more factories like: forMimeType,forAction,forClass
and methods like addParameter.


2nd) Rename UriIntents to IntentManager.
class IntentManager {
    void execute(Intent<Void>);
    Future<? extends T> submit(Intent<T>);
}

Created attachment 150093 [details]
Proposed Patch v3

> In Reply to Jarda:
> I vote for #2 but I would do these changes for easier extensibility in the 
> future.
Thank you very much, Tomas, for your suggestions.
I made some slight modifications (commented below). If I understood some of your ideas incorrectly, please let me know.

> 1st) Add a Intent class.
> 
> final class Intent<R> {
>     static Intent forURI(URI){}
> }
The class is not final, but it has a package-private constructor, so that implementations can be defined in package-private classes.

Type parameter of the Intent returned by forURI is Boolean (true if the URI was handled, false otherwise).

> 2nd) Rename UriIntents to IntentManager.
> class IntentManager {
>     void execute(Intent<Void>);
>     Future<? extends T> submit(Intent<T>);
> }
The execute() method can take Intent with any type parameter. If its return type is not Void, the return value will be simply ignored.

Cannot return type of submit() be "Future<T>" instead of 
"Future<? extends T>"?


The SPI is OK?
I think making it generic would be quite difficult, as we cannot look instances up by class and type parameters (because of type erasure).

Thank you for reviewing.

>The class is not final:
OK for me.

>The execute() method can take Intent with any type parameter
Yes, it can be wildcard <?>

>"Future<T>" instead of  "Future<? extends T>"
Yes, you can.


>The SPI is OK?
It can stay as it is in case of another Intent type a new SPI will be created.
Or it can be changed into generic SPI which intact will be the Intent.invoke, the IntentManager will lookup all IntentHandlers and ask them if they support given Intent
the first one which does will handle it.
But I have no opinion about this. Feel free to choose any implementation.

Y01 Please attach or point to generated Javadoc. It is usually better, for completely new modules to read the Javadoc than a diff.

Y02 I don't understand much the purpose of IntentManager in the API. The methods could be moved into Intent class. They would be easier to find for the API clients.

Y03 Please prevent instantiation of all URI handlers, when only some of them are needed. E.g. I'd suggest to create @OpenUriHandler.Registration where one could specify a URI scheme (with regular expression?) that is required to be matched before the handler even gets instantiated.

Y04 Sometimes there may be multiple handlers registered for the same URI. It would be nice if the the API could somehow display them (icon+displayname) and let the user choose from them.

Created attachment 150264 [details]
Proposed Patch v4

Patch updated for current jet-main sources.
It is now independent on patch for api.io.

> Y02 The methods could be moved into Intent class.
Methods from IntentManager moved to Intent class.

> Y03 Please prevent instantiation of all URI handlers.
@OpenUriHandler.Registration added. It specifies URI pattern as regular expression.

Created attachment 150265 [details]
Generated JavaDoc (ZIP file)

> Y01 Please attach or point to generated Javadoc.
Attached.

Thank you for your comments.

Summary of review meeting:

 - Use intent as final class holding pair (String, URI)
 - Remove Intent.execute()
 - Make Intent.submit() not static and rename it to execute()
 - Remove Handler to IntentAction
 - Make Intent.getAvailableHandlers() not static and rename it to getIntentActions()
 - Registration should apply to a static method, not interface
 - SPI method should take a parameter similar to FutureTask instead of
   returning Object.
 - IntentAction.execute() should return Future<Object>

Created attachment 150429 [details]
Proposed Patch v5

> - Use intent as final class holding pair (String, URI)
> - Remove Intent.execute()
> - Make Intent.submit() not static and rename it to execute()
> - Remove Handler to IntentAction
> - Make Intent.getAvailableHandlers() not static and rename it to 
>   getIntentActions()
> - Registration should apply to a static method, not interface
> - IntentAction.execute() should return Future<Object>
Done.

> - SPI method should take a parameter similar to FutureTask instead of
>   returning Object.
FutureTask doesn't seem suitable for this, its set() method is protected.
I'm considering adding "ResultHandle" class with methods "setResult(Object)" and setException(Throwable).

We can also consider introducing variant of Intent.execute() that takes a callback as argument.

Created attachment 150431 [details]
Generated JavaDoc (ZIP file)

Cannot download and unzip the Javadoc.

Please ignore my previous comment. I managed to do it. Looking at it:

Y11 no package.html description for API and SPI
Y12 no Javadoc for Intent class
Y13 return type of getIntentActions should be declared immutable (if it is?)
Y13a possibly the return type of getIntentActions could be SortedSet<? extends IntentAction>
Y14 usecase is out of date. still talks about static method.
Y15 javadoc of IntentHandlerRegistration should contains a sample code (e.g. not only usecase should have it). Again usecase is out of date.
Y16 IntentHandlerRegistration does not talk about threading

Created attachment 150730 [details]
Proposed Patch v6

Added support for callbacks and handlers that take Result parameter.
Please see attached JavaDoc.

> Y11 no package.html description for API and SPI
Added package-info.java files.
> Y12 no Javadoc for Intent class
Added.
> Y13 return type of getIntentActions should be declared immutable (if it is?)
Fixed.
> Y13a possibly the return type of getIntentActions could be
> SortedSet<? extends IntentAction>
Fixed.
> Y14 usecase is out of date. still talks about static method.
Updated.
> Y15 javadoc of IntentHandlerRegistration should contains a sample code
> (e.g. not only usecase should have it). Again usecase is out of date.
I've added sample code to package info. It is referenced from class JavaDoc.
> Y16 IntentHandlerRegistration does not talk about threading
Added to package info.

Thank you for the comments.

Created attachment 150731 [details]
Generated JavaDoc (ZIP file)

Updated JavaDoc for patch v6.

I would link  API types in code samples with <a href="@TOP@/...Intent.html">Intent</a> their definition using the <a> tag. But otherwise OK. I don't have any other comments.

Created attachment 150841 [details]
Proposed Patch v7

I would link  API types in code samples [...] with their definition [...]
Fixed.

Thank you.

Created attachment 150842 [details]
Generated JavaDoc (ZIP file)

Updated JavaDoc for patch v7.

I would like to integrate the patch next Tuesday (Dec 9), providing there are no objections.

Seems good to me, feel free to integrate.

I'll integrate the patch tomorrow afternoon (unless any objection is raised).
Thank you very much for reviewing.

Integrated as http://hg.netbeans.org/core-main/rev/4b8bfb738638.

Integrated into 'main-silver', will be available in build *201412111033* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)

Changeset: http://hg.netbeans.org/main-silver/rev/4b8bfb738638
User: Jaroslav Havlin <jhavlin@netbeans.org>
Log: #248126: Intent API