Bug 209058

[JG01] Use Openable rather than OpenCookie.


[JG02] This utility method looks more complicated than you should need just to open a file at a specific line. There are plenty of other modules which need very similar functionality, which should not be obliged to copy all this stuff - especially the esoteric use of UserQuestionException.

Consider extracting this into a static utility method in e.g. DataEditorSupport, possibly with an overload taking a line & column number rather than offset. Search around for usages of LineCookie and find some other modules which could reuse the new API.

Created attachment 116333 [details]
patch for JG01 and JG02 comments

Added three static methods in DataEditorSupoprt and found some modules that could reuse this utilities methods. Is this what you had in mind?

BTW do not use the IDE's "Export Diff" feature for anything. Run 'hg diff' on the command line (making sure [diff] git=1 in your ~/.hgrc, i.e. --git), or equivalently use MQ.

Also note that Bugzilla lets you mark attachments as obsolete, which you should do when attaching a revised patch.


JG02 - looks nice to me! You need openide.loaders/{manifest.mf,apichanges.xml} edits of course, and @since tags.


[JG03] Javadoc miscellany:
- {@link org.openide.loaders.DataObject} is unnecessary when this type is mentioned in the method signature; can just talk about "a file".
- openDocument can refer to {@link #getStyledDocument} for discussion of UserQuestionException rather than repeating it.


[JG04] openDocument(DataObject,int,int) should document that the numbers are 0-based. Also that -1 is accepted for line to mean "none in particular" (at least the impl accepts that) - same in openDocument(DataObject,int).


[JG05] Probably better to explicitly pass in Line.ShowOpenType and Line.ShowVisibilityType, with Javadoc recommending OPEN & FOCUS. Some potential callers would want REUSE, etc.


[JG06] Catch and handle IndexOutOfBoundsException.

Created attachment 116481 [details]
gototest api review patch

Thanks for the tips about hg diff.

[JG03]-[JG06] are solved with the new patch. 
I am not sure about changes in openide.loaders/{manifest.mf,apichanges.xml} and @since tags though. Have I used them as they are supposed to?

If there are no more objections I would like to integrate tomorrow. Thanks

(Sorry for late comments - I was on a vacation.)

JL01: I welcome an openDocument(..., int offset, ...) utility. Note, however, that putting that inside loaders goes somewhat against efforts in bug #209231. Moreover, seems that most of the clients do not really care about DataObjects, they've got a FileObject and an offset or a line number. So it would be awesome if we could find a place outside loaders to place this utility, and make the utility take FileObject instead of DataObject. That would be even easier than the proposed state for most of the clients. The trouble is that appropriate place is not easy to find, which is the reason why this generic utility does not yet exist.

JL02: I think that the openDocument methods should be documented and implemented to be callable from any thread. This does not seem to be the case currently, as Line.show (and OpenCookie/Openable.open) must be called from AWT thread, AFAIK. Please see how this is solved in org.netbeans.api.java.source.UiUtils.doOpen. I am sure there are clients that invoke the open methods in java.source (UiUtils) and java.source.ui (ElementOpen) outside AWT thread, so unless the generic open method can be called outside AWT thread, we will never be able to rewrite UiUtils and ElementOpen to use it.

Re JL01: to make it clear, I can live with DataEditorSupport.openDocument(DataObject, ...), but I don't think it is a perfect solution.

(In reply to comment #6)

[JL01]: Would you recommend some other place for the new static methods? Jesse any thoughts on that?

[JL02]: Seems reasonable to me. I will implement it. Thank you for the tip.

JL01 - I was thinking about that too but did not think of any better place; openide.text is not supposed to depend on FileObject.

(It currently does so transitively, via e.g. openide.awt, but only because of LayerGeneratingProcessor, which is not a runtime dep; might be possible for such modules to remove <run-dependency> from project.xml, though I am afraid this might break some forms of annotation processing. The root problem is that we have two disjoint uses of openide.filesystems - the system filesystem, and user files such as masterfs and JarFileSystem handles - mashed together into one module.)

You could just leave it in DataEditorSupport for now; since this is just a static utility method, if we decide later on a better place to put it, it would be easy to define it in that lower layer; make the DES copy delegate to it; deprecate the DES variant; and supply a Jackpot refactoring for the switch.

Another option would be to have it take Lookup rather than FileObject/DataObject, since the Lookup is in fact all that is used from its argument. Assuming bug #209231 is accepted, you could then NbDocument.openDocument(fobj.getLookup()) rather than NbDocument.openDocument(dobj.getLookup()). Or just have it take Lookup.Provider, in which case NbDocument.openDocument(dobj) would work now and probably NbDocument.openDocument(fobj) later.


JL02 - it would certainly be good to be able to call these methods from any thread, with the implementation deciding which subcalls need to be in EQ, off EQ, or either.

But in such a case I would not recommend returning boolean, since you are then doing an implicit invokeAndWait, which is rather dangerous. Since there seem to be some use cases for taking alternate actions when the open fails, so returning void is not a good option, I recommend returning Future<Boolean> - then it is obvious that you can attach an action to be run when the task finishes, or you can try waiting for it but it is clear that you are blocking and need to handle InterruptedException.

Created attachment 116625 [details]
gototest api review patch

The new patch also fixes the comment from JL02

Created attachment 116661 [details]
gototest api review patch

This patch follows Jesse's suggestions.

DataObject is changed to Lookup.Provider and the doOpen methods wait for the file to be opened before returning true/false to the client.

I think this is better approach than using the solution from org.netbeans.api.java.source.UiUtils.doOpen as in the latter case if a client calls the DES methods from a thread other than the EDT then they would return immediately, even though the file might not be open in the editor.

What do you think?

If there are no more comments I plan to integrate tomorrow. Thanks.

From implementation point of view - as I read the code, it will invoke Line.show outside AWT, in a separate worker thread. I don't think it is allowed to call Line.show outside AWT. If it would, then it could more easily be invoked in the thread openDocument was invoked. From my point of view, invokeLater it would be fine (better than invokeAndWait for sure), but it truly has its drawbacks.

[JG07] There should be just one <change> in apichanges.xml, just mentioning the names of the new methods.


[JG08] Javadoc like "@param provider the Lookup.Provider" is not very helpful. You can see very well from the generated HTML what the type is. The text of the Javadoc tag should say what the argument means or what is expected, e.g. "@param provider for example a {@link DataObject}".

Also e.g. "OPEN should be used" is misleading English - means you should not use other values, which is not the case. Probably you mean "OPEN would typically be used".

Also e.g. "@see org.openide.text.Line.ShowOpenType" is pointless. Again the HTML will clearly indicate types of all parameters and link them.


[JG09] What is "#TXT_Question" supposed to be? Did you mean to use NbBundle.getMessage (or @Messages) here?


[JG10] Use RequestProcessor rather than Executors - can be used as a drop-in replacement but reuses the same thread pool as the rest of NB, etc.


[JG11] I do not understand how or why JL02 was addressed.

As to how - posting calls to show/open into an executor service do not cause that call to be made in EQ; to the contrary, it ensures that they are _not_ made in EQ even if the DES.openDocument call was!

As to why - neither Line.show nor Openable.open are documented to require being called in EQ.

In either event, it is TBD whether it is a good idea for DES.openDocument to block, as I discussed in comment #9. EditorCookie.openDocument and thus DES.getStyledDocument can block, yet many callers of DES.openDocument do not need any special handling for a failure to open a file - i.e. they would ignore any return value anyway, and would be better off making a nonblocking call that continues in the background. Some other callers would want some handling for a failure to open, but could do it asynchronously in callback fashion; still others need to block until they can return a success code, perhaps due to an existing API contract on them in turn.

(Unfortunately no single return type handles all these use cases well; Future<Boolean> works OK for a blocking caller, though throwing the inappropriate ExecutionException, but makes it very difficult to attach a listener; org.openide.util.Task permits a listener to be attached but makes it hard to provide a result; ExecutorTask does both but cannot be used from this module.)

If the current API signature is kept - returning boolean, thus blocking for all callers - it must at least be documented that openDocument blocks, and while it "can be called from any thread" it perhaps _should_ not be called from some threads.

[JG12] Part of the point of using Lookup.Provider to address JL01 was to move these methods down from openide.loaders, probably into openide.text, perhaps into NbDocument.


[JG13] The name "getStyledDocument" overemphasizes the distinction between Document and StyledDocument, which is rarely of interest for this kind of caller. While we might as well keep StyledDocument as the return type, since EditorCookie is obliged to produce it, simply "getDocument" would suffice for the name.

Created attachment 116819 [details]
patch for JG07-JG13 comments

Fixed JG07-JG13

Moved the methods from openide.loaders into openide.text (NbDocument)
Followed JL02 suggestions and documented that openDocument can block

BTW standard practice is to attach updated complete patches (using Bugzilla to mark the old ones as "obsolete"). It is difficult to really review a sequence of cumulative patches, and perhaps even you as the author are not checking what the cumulative effect is; for example, the patch from comment #16 does not remove any imports from DataEditorSupport.java, so is the cumulative patch adding some unused imports such as for UserQuestionException?


Remember that <date> should give the actual date of integration to trunk.


[JG14] Minor code simplification possible using Mutex.EVENT.writeAccess.

Created attachment 116918 [details]
gototest api review patch

Sorry for that Jesse. In this patch all the changes are shown (JG14 was also implemented)

If there are no more comments I would like to integrate tomorrow. Thank you

Ready to commit whenever you are ready, at least as far as I am concerned. Minor Javadoc notes:

{@link DataObject} will not work from NbDocument, but you can use e.g. {@code org.openide.loaders.DataObject}, or <a href="@org-openide-loaders@/org/openide/loaders/DataObject.html">{@code DataObject}</a>. (The latter uses a special token substituted by the NB Javadoc generation scripts and permitting links to APIs outside this module and its dependencies.)

BTW @return {@link javax.swing.text.StyledDocument} is pointless as in JG08; generated Javadoc will already link to the declared return type. Suffices to say e.g. "@return a document or null".

(In reply to comment #19)
> Ready to commit whenever you are ready, at least as far as I am concerned.
> Minor Javadoc notes:
> 
Fixed: http://hg.netbeans.org/main/rev/277a2c25a43a

{@link DataObject} fixed on getDocument but not on the two openDocument overloads. If you build Javadoc on this module, which you ought to do when preparing an API change, you should see a warning about this.

(In reply to comment #21)
so sorry. fixed now: http://hg.netbeans.org/main/rev/d785903d4d09

Integrated into 'main-golden', will be available in build *201203230400* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/f0fcd039c82f
User: Theofanis Oikonomou <theofanis@netbeans.org>
Log: #209058: patch from comment11