Bug 196810 - @MultiViewElement.Registration & EditorSupport.forMimeType("....")
@MultiViewElement.Registration & EditorSupport.forMimeType("....")
Status: RESOLVED FIXED
Product: platform
Classification: Unclassified
Component: Window System
7.0.1
PC Linux
: P1 (vote)
: 7.1
Assigned To: Jaroslav Tulach
issues@platform
: API_REVIEW_FAST, PLAN
Depends on: 199205
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-17 14:53 UTC by Jaroslav Tulach
Modified: 2011-06-07 16:06 UTC (History)
7 users (show)

See Also:
Issue Type: ENHANCEMENT
:


Attachments
JDO multiview (19.38 KB, patch)
2011-05-24 12:32 UTC, Tomas Stupka
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jaroslav Tulach 2011-03-17 14:53:06 UTC
In order to create loosely coupled and extensible cooperation between individual tabs in a multiview, it seems reasonable to create new annotation to replace MultiViewElementDescription.

Such annotation could be applied to any MultiViewElement implementation and register it for participation in multiviews for given mime type.

To simplify client usage of multiviews, there would be a factory method to create new cloneableeditorsupport for given mime type:

CloneableEditorSupport forMimeType(MimePath);

The, currently complex boiler plate code will be greatly simplified and possibly such decoupling would open the usage of these elements also in other systems not directly based on multiview.
Comment 1 Jaroslav Tulach 2011-03-22 10:56:22 UTC
Build job and javadoc is available at
http://deadlock.netbeans.org/hudson/job/prototypes-multiview-registration-196810/
there is a multiview-registration-196810 branch is core-main repository. Time for review of the overall approach.
Comment 2 Jaroslav Tulach 2011-03-22 12:00:18 UTC
Sample usage in form is shown at
http://hg.netbeans.org/core-main/rev/003b478bd130
Right now it still provides its own CloneableEditorSupport. I will try to eliminate that by providing 
EditorCookie org.netbeans.core.multiview.api.text.MultiTextViews.create(
  String mimeType, Lookup context
)
if possible.
Comment 3 Jesse Glick 2011-03-23 14:20:44 UTC
BTW getting branch diff is a little tricky due to use of '-':

hg di -r "ancestor(default,r'multiview-registration-196810'):r'multiview-registration-196810'"
Comment 4 Jesse Glick 2011-03-23 14:37:56 UTC
[JG01] ContextAwareDescription should extend MultiViewDescription. Or may be clearer for a given object to implement one or the other, in which case create{,Cloneable}MultiView should be written as:

Lookup lkp = MimeLookup.getLookup(mimeType);
List<MultiViewDescription> arr = new ArrayList<MultiViewDescription>(lkp.lookupAll(MultiViewDescription.class));
for (ContextAwareDescription d : lkp.lookupAll(ContextAwareDescription.class)) {
  arr.add(d.createContextAwareDescription(context));
}


[JG02] "org.openide.text.CloneableEditorSupport.Pane" could be written as CloneableEditorSupport.Pane.class.getCanonicalName() for find usages etc.


[JG03] "You must specify either mimeType" is not grammatical; drop "either".
Comment 5 Jaroslav Tulach 2011-04-01 23:26:38 UTC
New and close to final round review: http://hg.netbeans.org/core-main/rev/8dc4adc00217 time to integrate.
Comment 6 Jesse Glick 2011-04-04 16:13:33 UTC
I think JG01 was addressed in a strange way. createContextAwareDescription should return MultiViewDescription, not ContextAwareDescription. And lkp.lookupAll(ContextAwareDescription.class) will omit any non-context-aware MultiViewDescription's. Was this really intentional?
Comment 7 Jaroslav Tulach 2011-04-07 05:39:57 UTC
(In reply to comment #6)
> I think JG01 was addressed in a strange way. createContextAwareDescription
> should return MultiViewDescription, not ContextAwareDescription. And
> lkp.lookupAll(ContextAwareDescription.class) will omit any non-context-aware
> MultiViewDescription's. 

Well, JG01 wanted to use "lkp.lookupAll(ContextAwareDescription.class)". I did it. Right, it rules out traditinally implemented MultiViewDescriptions and thus the only way to register an element is using the @Registration annotation. I guess such restriction is not bad (view without a context is strange anyway) and can be released in future.

The actual signature of ContextAwareDescription is not that important, as for now, it is not part of the API. It is just an implementation detail that can be changed later.
Comment 8 Jesse Glick 2011-04-07 11:31:04 UTC
To JG01 - right, this is in a private package for now so it does not matter much. Just looked suspicious when examining code.
Comment 9 Jaroslav Tulach 2011-05-16 12:53:18 UTC
New changes to review: 
http://hg.netbeans.org/core-main/rev/a61e12d7e00a
New javadoc:
http://deadlock.netbeans.org/hudson/job/prototypes-multiview-registration-196810/javadoc/
Unless there are objections I'd like to integrate next Monday.
Comment 10 Geertjan Wielenga 2011-05-16 17:24:04 UTC
Looks like a great API to me, blogged about it here: http://blogs.oracle.com/geertjan/entry/multiview_of_the_next_release

I suspect many will wonder how all this related to XMLDataObject and the XML MultiView API.
Comment 11 Geertjan Wielenga 2011-05-18 09:14:13 UTC
GW01: The reason people want to create a MULTIview is because they want to have more than one view, so the wizard should create one extra view (i.e., a JPanel implementing MVE). Then the wizard will be really useful and developers will have a clear example of what each visual view in the multiview should contain.
Comment 12 Jan Lahoda 2011-05-19 14:47:50 UTC
JL01: What about existing DataObject? Is there (or will there be) some documentation how to upgrade existing DataObjects to the new approach and extend them with multiview? Or does the user need to generate a dummy DO and guess how to update his/hers DO?


JL02: (re MultiViewEditorElement) I really appreciate that the intent is to make integration of editors in multiview easier - it was IMO the biggest problem in MV. The code from the apisupport template does not work, however. When I create a MV DO, start the app, create a new file of the new type and edit it, I cannot save it, and the File/Save and File/Save As... actions are shaded. I would suggest to fix these before integration to ensure that no API changes are needed to resolve this.

JL04: MultiViewEditorElement.java is missing @since


JG05: I would consider MultiViews.create* to be more part of the SPI (as was the original MultiViewFactory) - what clients (other then DOs) are going to call them?

JG06: (re MultiViews.create*) (question/idea) it might make sense to make many DOs multiview by default (e.g. JavaDataObject), allow registration of providers that would provide the additional panels if appropriate. For example, the BeanInfo support could ideally add another panel to "plain" Java editor without having its own DO (this is not practically doable now, because of guarded blocks and possibly other stuff). This would of course also mean that the multiview "toolbar" wouldn't be shown if there would be only one multiview element. Ideas?


JG04: (re MultiViewElement.Registration) For what use cases will MultiViewDescription and current methods in MultiViewFactory be used? If none, deprecate them, otherwise add note to MVD and MVF javadocs refering to the new A|SPIs and when should the new/old A|SPI be used.
Comment 13 Jaroslav Tulach 2011-05-19 16:44:26 UTC
JL01: The current patch shows how to migrate FormDataObject. Tomáš Stupka is about to create a patch to migrate JavaDataObject to MVE as he is willing to use the new infrastructure for local history module (and demonstrate that on JDO).
JL04: c8e0ab128b1f
JL(G)05: The new factory methods don't deal with SPI interfaces at all. They are targeted to clients that want to use multiview, but need not donate any content. In order to shield such API users from the uninteresting complexity of the SPI, I believe it is proper to place the factory methods into (much simpler) API.
JL(G)06: Right, local history is about to use this feature. Teams shall get ready for migration to use MultiView. But that is not in scope of this API change. That is something local history rewrite needs to keep an eye on.
JL(G)04: The old methods will be used for the same purpose they have been used so far and they will work. Sometimes it just may not make sense to read MVEs from layer.

Tasks:

GW01: OK, I'll generate a form implementing MultiViewElement next to it.
JL02: OK. 

Good comments, guys. Thanks.
Comment 14 Jan Lahoda 2011-05-22 20:35:03 UTC
(In reply to comment #13)
> JL01: The current patch shows how to migrate FormDataObject. Tomáš Stupka is
> about to create a patch to migrate JavaDataObject to MVE as he is willing to
> use the new infrastructure for local history module (and demonstrate that on
> JDO).

Well, if we are going to change virtually all DOs in whole NB IDE (as will be needed to support the localhistory), then we should have a step-by-step guide on how to do it, so that the DO owners can change the DO quickly without learning all the twists of MV (see also JLO07). Unless, of course, someone will upgrade the whole IDE.

> JL(G)04: The old methods will be used for the same purpose they have been used
> so far and they will work. Sometimes it just may not make sense to read MVEs
> from layer.

Fine. Please still add note(s) about the new SPIs to the old SPIs.

JL07: Seems that the MultiViews.create method's header should be:
public static <T extends Serializable & Provider, R extends CloneableTopComponent & Pane> R createCloneableMultiView(String mimeType, T context);
to eliminate the strange cast in clients.

JL08: Please add @NonNull, @CheckForNull, etc annotations for all (new) API methods.
Comment 15 Jesse Glick 2011-05-23 15:00:32 UTC
(In reply to comment #14)
>> Tomáš Stupka is
>> about to create a patch to migrate JavaDataObject to MVE as he is willing to
>> use the new infrastructure for local history module
> 
> if we are going to change virtually all DOs in whole NB IDE (as will be
> needed to support the localhistory)

This looks wrong. We will never convert every single DO type to a new API, so a local history tab will then be available on most, but not all, files. Some other approach seems necessary.
Comment 16 Tomas Stupka 2011-05-24 12:32:32 UTC
Created attachment 108477 [details]
JDO multiview

see attached patch for changes in java.source - a working version of how JDO could be rewritten: 
- override JavaEditorSupport.createPane() to return a CloneableMultiView
- register a factory method to return MultiViewEditorElement (to be used instead of JavaEditor)

TS01 - when using o.n.c.spi.multiview.text.MultiViewEditorElement instead of JavaEditor then even closing an unmodified java file opens a question dialog - "Cannot safely close component for following reasons:"

TS02 - (cosmetic) overriding JavaEditorPane.createPane() to create CloneableMultiView - the javadoc in CES.createPane() states that the method typicaly shouldn't be overriden. Now this seems to become typical...

TS03 - see JavaEditorPane.createPane() in attached patch - had to fix the CloneableMultiView into editor mode - it would be more convenient to get this setup directly via a call in MultiViews 

TS04 - when invoking the "Show LH" action on a file, the files editor TC should be opened and the History tab activated if it's available. At the moment there is no correct way how the LH should find out if there is a MultiView registered for the relevant dataobject or not. Maybe it would make sence to explicitely couple with the History tab when registering a dataobjects multiview. No matter if in a generic implementation as suggested by Jessie or for every relevant dataobject.
Comment 17 Jaroslav Tulach 2011-05-25 15:06:28 UTC
New diff for review: http://hg.netbeans.org/core-main/rev/fb50e84d55e8
I'll increment spec numbers and fix dependencies and integrate by end of the week.

Re. JL04. OK, added links to new methods.

Re. JL07. We are trying to avoid showing openide.text interface in multiview API (except in text subpackage) with the hope that one day multiview will be able to live without openide.text (naive, but that was always the plan). Moreover there is already existing factory method with CTC only. Thus I want to keep CTC only in this case as well.

Re. JL08: As far as I can tell I am following project defaults (@NonNull unless specified). Thus I am not going to use such annotation per every method and parameter. If there is an easy way to use them to express project defaults, I may be willing to do it.

Tomáš, thanks for testing my implementation in real use case. It shows some issues that need to be fixed. However I'd like to remind that the actual conversion of history to a multiview tab is out of scope of this change and needs to be reviewed separately.

TS01 & JL02 - Fixed and verified when a new object is created using the File Type wizard.

TS02 - Changed javadoc.

TS03 - Moving to editor by default.

TS04 - Your solution in the patch (e.g. getOpened() and seek through them) is the best I can imagine right now. It may be discussed deeply while reviewing the "history tab" change.
Comment 18 Jan Lahoda 2011-05-25 15:28:00 UTC
(In reply to comment #17)
> Re. JL07. We are trying to avoid showing openide.text interface in multiview
> API (except in text subpackage) with the hope that one day multiview will be
> able to live without openide.text (naive, but that was always the plan).
> Moreover there is already existing factory method with CTC only. Thus I want to
> keep CTC only in this case as well.

Well, I still have troubles to accept that virtually everybody will need to do a strange, and not really necessary, cast. Creating a new API that is ugly from the very beginning and forces to do casts without a *real* good reason seems bad to me. What about moving the factory into spi...text? That would fulfill all current requirements and would leave us options for the future.

> 
> Re. JL08: As far as I can tell I am following project defaults (@NonNull unless
> specified). Thus I am not going to use such annotation per every method and
> parameter. If there is an easy way to use them to express project defaults, I
> may be willing to do it.

Well, do bug detection tools understand these project defaults? Or are these tools simply unusable on this code and code that uses this API/SPI?
Comment 19 Jesse Glick 2011-05-25 18:39:19 UTC
(In reply to comment #18)
> do bug detection tools understand these project defaults?

No, of course not.

> Or are these
> tools simply unusable on this code and code that uses this API/SPI?

They will be unless annotations are added, so please add them.
Comment 20 Jaroslav Tulach 2011-05-30 07:33:16 UTC
(In reply to comment #18)
> (In reply to comment #17)
> bad to me. What about moving the factory into spi...text? That would fulfill
> all current requirements and would leave us options for the future.

The problem is that the factory methods belongs into API. There is no api...text yet. It seems like an overkill to create new package for a single new class to me. Re. "options for the future" - the current state gives us the same and few more options (e.g. decide to do nothing).

> Well, do bug detection tools understand these project defaults? 
> + Jesse: No, of course not.

I googled around and found @DefaultAnnotationForMethods at http://findbugs.sourceforge.net/manual/annotations.html
I am sure findbugs understands it. I don't see such annotation at our http://bits.netbeans.org/dev/javadoc/org-netbeans-api-annotations-common/ however.

As soon as such annotation is added I can annotate package-info.java with it. Meanwhile I am going to review all the newly added methods and annotate all that differ from project defaults by appropriate annotation and integrate all the changes tomorrow.
Comment 21 Jaroslav Tulach 2011-05-31 07:59:25 UTC
Merged as ergonomics#c9752bf56e87. @NullAllowed added in ergonomics#631fe4492d29
Comment 22 Quality Engineering 2011-06-02 20:51:32 UTC
Integrated into 'main-golden', will be available in build *201106021001* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/c9752bf56e87
User: Jaroslav Tulach <jtulach@netbeans.org>
Log: Merge of #196810: @MultiViewElement.Registration


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo