Bug 154781 - Generic annotation editing solution
Summary: Generic annotation editing solution
Status: ASSIGNED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.2   Edit
Hardware: PC Windows XP
: P3 enhancement with 2 votes (vote)
Target Milestone: ---   Edit
Assignee: JDT-UI-Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on: 12352 156277 162079
Blocks:
  Show dependency tree
 
Reported: 2006-08-22 19:35 EDT by Neil Hauge CLA
Modified: 2009-06-03 03:04 EDT (History)
10 users (show)

See Also:


Attachments
Basic patch that implements Tabbed Properties in the JavaEditor (4.15 KB, patch)
2006-09-26 09:29 EDT, Karen Butzke CLA
no flags Details | Diff
tabbed properties support in the JavaEditor (4.31 KB, patch)
2006-09-27 10:39 EDT, Karen Butzke CLA
no flags Details | Diff
Patch to implement StructuredSelectionProvider and tabbed properties in the JavaEditor (7.53 KB, patch)
2006-10-11 13:08 EDT, Karen Butzke CLA
no flags Details | Diff
implementation of tabbed properties SelectionConverter for the JavaEditor (9.25 KB, patch)
2006-10-17 10:54 EDT, Karen Butzke CLA
no flags Details | Diff
Updated patch (8.71 KB, patch)
2006-10-19 10:53 EDT, Karen Butzke CLA
no flags Details | Diff
Improved patch (3.72 KB, patch)
2006-10-24 06:59 EDT, Dani Megert CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Neil Hauge CLA 2006-08-22 19:35:00 EDT
Since being introduced for general use in Java 5, annotations have become an increasingly prominent mechanism for specifying metadata that has a relationship with Java code.  Many technologies, including several Java EE technologies (for example EJB 3), now make use of annotations to allow configuration of related metadata.  

Given the level of complexity in defining this metadata in certain circumstances, tool providers such as the WTP-JPA project are interested in providing a way for end users to configure annotations via a UI.  Currently this is done with a custom view, as there is no generic annotation editing facility provided by Eclipse.  Others have created their own annotation editing views for the same purpose.

Since annotations are directly tied to the java source, adopters are limited in their ability to provide UI in a consistent fashion.  This is due to the current lack of ability to enhance the Java Editor or its Properties in an Eclipse friendly way.

I am not certain what the best solution is to this general issue.  It has been suggested that having the Java Editor activate the Tabbed Properties would allow different extenders to contribute tabs for configuring their annotations on a given type.  This solution offers consistency, extensibility, and provides the ability to configure annotations using rich UI if desired.  Another possibility would be to have a specific Annotation Properties View, which could be extended by multiple clients to provide context-based values for their particular annotations.
Comment 1 Martin Aeschlimann CLA 2006-08-29 11:39:49 EDT
Some questions:
- the extent of 'annotations': Is that only about the Java annotations in the source code ('@A(...) void foo()') or also about (custom) annotation information in meta files?
- in JDT UI, we like to be editor centric. So we prefer putting the tooling possible to where you type: Code completion, quick fixes, documentation hovers ect. If adding a property view for templates is a suggestion, what can such a view do better?

Comment 2 Gunnar Wagenknecht CLA 2006-08-29 12:00:38 EDT
(In reply to comment #1)
> - the extent of 'annotations': Is that only about the Java annotations in the
> source code ('@A(...) void foo()') or also about (custom) annotation
> information in meta files?

AFAIK the focus is on Java annotations.

> - in JDT UI, we like to be editor centric. So we prefer putting the tooling
> possible to where you type: Code completion, quick fixes, documentation hovers
> ect. If adding a property view for templates is a suggestion, what can such a
> view do better?

It adds another (very likely domain specific) view to a Java element. The advantage I see over an editor centric approach is that one would not have to remember what annotations to use to achieve a certain goal (i.e., abstraction) and all the possible details about an annotation (i.e., simplification).

I guess that everything could be implemented using the editor centric approach. However, allowing a different view on a Java element/annotation would definitely open the usability to folks that prefer adding/removing annotations by selecting or deselecting a check-box or selecting annotation values from a drop down or by clicking on a button that opens a more sophisticated dialog or even a wizard which may require expensive actions, such as database connectivity.

I would probably one of those guys that continue to hit "Ctrl+Space". But I have to admit that I also enjoy the possibility of saving a few keystrokes by using a nice Forms UI like view. ;)
Comment 3 Neil Hauge CLA 2006-08-29 16:13:37 EDT
(In reply to comment #1)
> Some questions:
> - the extent of 'annotations': Is that only about the Java annotations in the
> source code ('@A(...) void foo()') or also about (custom) annotation
> information in meta files?

I am interested in the Java annotations in the source code.

> - in JDT UI, we like to be editor centric. So we prefer putting the tooling
> possible to where you type: Code completion, quick fixes, documentation hovers
> ect. If adding a property view for templates is a suggestion, what can such a
> view do better?

In addition to Gunnar's comments, a property view can also display default values for annotations (where defaults are present, such as JPA and JAXB 2.0).  This means that without having even specified an annotation in the source, a view can display the implied annotation at a given location, and the context based defaults that apply to an implied or specified annotation's values.  

As an example, consider the following scenario in EJB 3 JPA:

private String firstName;

is equivalent to:

@Basic
@Column(name="firstName")
private String firstName;

It is easy to convey this default information to the user in a UI.  This is a very simple example of defaults.

Annotation complexity is another area where a dedicated view could prove helpful.  Consider the following example in JPA to specify a Many-to-Many in annotations.

@ManyToMany
@JoinTable(
  name="EMP_PROJECT",
  joinColumns={
    @JoinColumn(name="EMP_COUNTRY", referencedColumnName="COUNTRY"),
    @JoinColumn(name="EMP_ID", referencedColumnName="EMP_ID")},
  inverseJoinColumns=@JoinColumn(name="PROJECT_ID"))
private Collection<Project> projects;

A UI is able to logically group the 3 annotations used for this "mapping", provide default values for the configuration, and provide efficient controls for organizing the metadata (for example: an Add/Edit/Remove List for join columns and one for inverse join columns).  Many other options are available for this mapping of the projects Collection.  All of these options can be displayed together in a UI, along with their default values.  Other settings for this particular mapping could include Cascade Type, Fetch Type, and Ordering.

This type of UI can be very helpful for someone getting started with a technology like JPA, and also helpful to the more advanced user in some situations.  If this UI was an extension of the Properties view, it could easily be opened or closed by the user.  Use of the Tabbed Properties would also insure consistent L&F and extensibility.


Comment 4 Martin Aeschlimann CLA 2006-08-31 10:46:33 EDT
I see the benefit for domain specific annotations. I think to use the properties view for this makes sense.
But this is very hard to drive from JDT.UI without a practical use cases. Can for example WTP-JPA take the lead here and let us know what exactly they need so that for example the properties view plays nicely together with the editor?

Comment 5 Neil Hauge CLA 2006-09-06 16:27:14 EDT
An update - we have been prototyping the Tabbed Properties solution in order to determine what problems there might be in making this work with the Java Editor. So far we have discovered one issue that would need to be addressed in the Tabbed Properties View before this solution would work.  Bug 156277 has been opened to address this.

We will continue to investigate.
Comment 6 Martin Aeschlimann CLA 2006-09-20 12:18:56 EDT
Can I move this bug to you, Neil?
Comment 7 Neil Hauge CLA 2006-09-20 13:57:37 EDT
I think we are ready to submit a patch that demonstrates what we need from the JDT UI.  We were hoping to get the associated bug addressed, but this hasn't happened as of yet, so we can just go ahead and submit a patch for the issue discussed here so that you guys understand what it is we need (which is not much).  

So, you can assign it to me if you want, but I plan to assign it back within the next day.  :)
Comment 8 Dani Megert CLA 2006-09-21 05:23:24 EDT
>This is due to the current lack of ability to enhance the Java Editor or its >Properties in an Eclipse friendly way.
What Java Editor Properties are you referring? Note that there are ways to extend the Java editing and the editor. We open it up piece by piece. It would be good to know what you are looking for in detail.
Comment 9 Karen Butzke CLA 2006-09-26 09:29:31 EDT
Created attachment 50902 [details]
Basic patch that implements Tabbed Properties in the JavaEditor

I have attached a basic patch that demonstrates what we need from the JDT UI.  All this patch does is provide the necessary extension point for supporting the Tabbed Properties in the Java Editor.  As a side effect of adding this support, the JavaOutline also support tabbed properties.  I implemented the Advanced tab as suggested by the tabbed properties framework to show the old properties table. 

There are 2 bugs logged against the Tabbed Properties framework which will affect the use in the java editor and extensions that want to add new properties:

Bug 156277 will allow the JavaEditor to fully utilize the tabbed properties. Currently, the JavaEditor does not implement any properties in the Properties view.  If this bug was fixed, the JavaEditor could implement the basic properties just like they are implemented in the Java Outline.  This bug being fixed will also allow extenders to deal with the TextSelections from the JavaEditor to add context specific tab properties.

Bug 158607 will also need to be fixed to allow extenders to appropriately extend the java editor tabbed properties without overriding them or other extenders that add tabs.
Comment 10 Dani Megert CLA 2006-09-27 06:00:31 EDT
!!! This patch will break source compatibility of clients due to bug 148844 and hence JDT Text will not commit it unless bug 148844 has been fixed. If bug 148844 won't fixed then the inheritance based extension via ITabbedPropertySheetPageContributor has to be replace by an adapter i.e. via JavaEditor.getAdapter(...).

>As a side effect of adding this support,
>the JavaOutline also support tabbed properties. 
Yes, but in a strange way: after the patch the file's properties appear in an 'Advanced' tab which is wrong. I would expect to see something like that:
- 'Resource' tab with the file's resource properties
- 'Java' or 'Java Element' with the Java element properties

'Advanced' is a poor name in my opinion.

Karen, can you attach a workspace patch next time? Thanks. Also, note that due to bug 158945 you should currently not put a blank after the ',' when specifying the bundle version in the MANIFEST.MF.
Comment 11 Dani Megert CLA 2006-09-27 08:20:57 EDT
Note regarding source compatibility:
1) this can be broken in rare must-do situations but must be clearly documented in the migration guide. Nothing I'd like to do for this change.
2) the problem is not just for those that reference JavaEditor but also for those
   that reference any subclass JavaEditor
Comment 12 Karen Butzke CLA 2006-09-27 10:39:38 EDT
Created attachment 51002 [details]
tabbed properties support in the JavaEditor

I've attached a new patch that incorporates Daniel's suggestions.  I renamed the 'Advanced' tab to 'Java'.  The patch is a workspace-level patch on the 20060918-1200 integration build.

I don't think there should be a 'Resource' tab in the properties for the Java Outline.  Currently, when you select a Class you get resource information about the .java file, you get this information even if you select a Class in the file other than the top-level type.  I think you should only get this information by selecting the .java file or the .class file from the package explorer.  Regardless, this isn't directly related to this bug, and the JDT UI can approach their implmentation of the tabbed properties as they see fit.  My main goal was to not break existing functionality and to support adding new properties in my plugin.
Comment 13 Karen Butzke CLA 2006-10-11 10:25:39 EDT
After more investigation, this bug is not dependent on 158607, it would just be nice if it was implemented.  It is still dependent on 156277 and I am going to post another patch that directly depends on the patch I've posted for 156277
Comment 14 Karen Butzke CLA 2006-10-11 13:08:21 EDT
Created attachment 51771 [details]
Patch to implement StructuredSelectionProvider and tabbed properties in the JavaEditor

This patch is dependent on the patch given in bug 156277.  I believe I have fixed the problem related to bug number 148844 with this patch, am I wrong about that?
Comment 15 Dani Megert CLA 2006-10-12 05:01:14 EDT
-1 for that patch:
- I don't want to an inner class in the Java editor that isn't used there at all
- the name IStructuredSelectionProvider is wrong since it is not a selection
  provider (at least not as it is commonly used within Eclipse) but rather a
  simple selection converter. Also note, that there is already
  SelectionConverter.getStructuredSelection(...) which you could probably use.

I removed bug 148844 as blocker since the latest patch resolved this.
Comment 16 Karen Butzke CLA 2006-10-17 10:54:00 EDT
Created attachment 52128 [details]
implementation of tabbed properties SelectionConverter for the JavaEditor

This is a new patch based on the provided feedback.  I was unable to use  org.eclipse.jdt.internal.ui.actions.SelectionConverter because this provides different StructuredSelections that are meant for code assist.  This is not what you would want to display in the Properties view.
Comment 17 Karen Butzke CLA 2006-10-17 10:56:36 EDT
Forgot to mention that I also updated the patch in bug 156277 that this patch depends on.
Comment 18 Dani Megert CLA 2006-10-18 10:18:35 EDT
Karen,

it seems that your selection converter has lots of code that could be removed if you used helpers like SelectionConverter.getElementAtOffset(...)

Also getting the active page is easier:
JavaPlugin.getActivePage()

Another problem is the for loop: I guess you do this to check that the selection only covers one element, right? If so, simply check the elements range against the selection.

HTH
Dani
Comment 19 Karen Butzke CLA 2006-10-18 14:33:24 EDT
(In reply to comment #18)
> Another problem is the for loop: I guess you do this to check that the
> selection only covers one element, right? If so, simply check the elements
> range against the selection.

I am not sure of the functionality one would want in this situation.  If you had a text selection that starts in the IType and the end of the selection is still in the IType what properties would you show?  What about the fact that there are IFields and IMethods included in your selection?  Should this be considered a multi-selection as it would be in the Outline?  Or would you just show the properties for the IType?
Comment 20 Dani Megert CLA 2006-10-19 02:39:41 EDT
I didn't yet think of what should be shown there but only commented on what and how the patch does things ;-)

Up to now JDT Text never converts a text selection into a multi-element selection. We normally have to ways to deal with text selections:
1) we only take the caret location and compute the enclosing element
2) if length == 0 we extend to the word at the given offset then we take the 
   selection and check whether it *contains* an identifier and try to resolve 
   that to a Java element. (see also bug 75353)

I would implement 2) because we do the same for the Declarations and Javadoc view ==> if I select a Java element inside a method then the Properties view would give me information about that element. Not sure whether that fits/helps your model.
Comment 21 Dani Megert CLA 2006-10-19 02:45:13 EDT
Two things I previously missed to mention:
1) the patch is not yet complete: it only handles ICompilationUnit but must also 
   handle IClassFile

2) if there is any change that a properties computation can take a while (e.g.
   because it needs the AST) then you must do the same as we do in the 
   Javadoc and Declarations view: the information must be computed in a separate
   job (or thread). Failing to do so will force us to disable the feature again
   since we cannot accept degradation in typing speed. This however is not part
   of this patch but has to be considered in the tabbed Properties view 
   architecture. NOTE: it is not good enough to say that clients must ensure
   the performance. As an example take the Java hover extension point: clients
   can contribute hovers and we ensure that they are called in the background.

Comment 22 Karen Butzke CLA 2006-10-19 10:53:54 EDT
Created attachment 52319 [details]
Updated patch

I've updated the patch taking into account your suggestions including supporting displaying properties for class files.
Comment 23 Dani Megert CLA 2006-10-24 06:59:54 EDT
Created attachment 52582 [details]
Improved patch

This patch improves the previous one by removing code duplication adding null-checks where needed.
Comment 24 Dani Megert CLA 2006-10-24 07:25:45 EDT
Just saw that bug 12352 is marked as blocking bug and hence noticed that the current solution is a no-go regarding performance because when a user does a selection (e.g. Shift + mouse drag) it continuously notifies the selection converter (assuming the Properties view is open) and also updates the view. While this might be acceptable for views it is not for the Java editor and I strongly suggest to also change this for all other use cases. The trick here is to listen for post selection changed and not for selection changed events. This has to go into the (tabbed) Properties view code. This should probably be put into a separate bug which can then replace bug 12352 as blocking bug. 

Performance could be further improved by not updating the Properties view while
- in a stacked pane but not in front
- hidden as fast view
but that's another issue.