Bugzilla – Full Text Bug Listing
| Summary: | Add support for rich hovers in editor (like JDT) | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Modeling] TMF | Reporter: | Christoph Kulla <christophkulla> | ||||||||||||||||||
| Component: | Xtext | Assignee: | Project Inbox <tmf.xtext-inbox> | ||||||||||||||||||
| Status: | CLOSED FIXED | QA Contact: | |||||||||||||||||||
| Severity: | enhancement | ||||||||||||||||||||
| Priority: | P3 | CC: | b.kolb, jan, sebastian.zarnekow, sven.efftinge, thsoft, tmf.xtext-inbox | ||||||||||||||||||
| Version: | unspecified | Flags: | sebastian.zarnekow:
indigo+
|
||||||||||||||||||
| Target Milestone: | M4 | ||||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||||
| OS: | Mac OS X - Carbon (unsup.) | ||||||||||||||||||||
| Whiteboard: | |||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||
| Bug Blocks: | 320818 | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Christoph Kulla
Created attachment 174786 [details]
Patch for the hover
Created attachment 174788 [details]
hover without focus
Created attachment 174789 [details]
hover with focus
Created attachment 174790 [details]
hover after following link to "String".
*** Bug 284943 has been marked as a duplicate of this bug. *** Created attachment 176886 [details]
Hover with quickfixes
Created attachment 176887 [details] Patch for the hover (version 2) Attached a new version of the patch. Contains now an problem hover showing available quick fixes. This patch does also contains the java doc hover from https://bugs.eclipse.org/bugs/show_bug.cgi?id=320818 (https://bugs.eclipse.org/bugs/show_bug.cgi?id=320818 is now obsolete). This patch introduces the class DelegatingHover, which let's you register a number of hovers with a guice multibinder. I could get the multibinder working only with the non AOP guice version. With the regular guice version I get the following exception:
java.lang.IllegalAccessError: tried to access class com.google.inject.multibindings.Multibinder$RealMultibinder from class $com.google.inject.multibindings.Multibinder$RealMultibinder$$FastClassByGuice$$94efefc8
at $com.google.inject.multibindings.Multibinder$RealMultibinder$$FastClassByGuice$$94efefc8.invoke(<generated>)
at com.google.inject.internal.cglib.reflect.FastMethod.invoke(FastMethod.java:53)
at com.google.inject.SingleMethodInjector$1.invoke(SingleMethodInjector.java:59)
at com.google.inject.SingleMethodInjector.inject(SingleMethodInjector.java:91)
at com.google.inject.MembersInjectorImpl.injectMembers(MembersInjectorImpl.java:99)
at com.google.inject.MembersInjectorImpl$1.call(MembersInjectorImpl.java:76)
at com.google.inject.MembersInjectorImpl$1.call(MembersInjectorImpl.java:74)
at com.google.inject.InjectorImpl.callInContext(InjectorImpl.java:804)
at com.google.inject.MembersInjectorImpl.injectAndNotify(MembersInjectorImpl.java:74)
at com.google.inject.Initializer$InjectableReference.get(Initializer.java:145)
at com.google.inject.Initializer.injectAll(Initializer.java:92)
at com.google.inject.InjectorBuilder.injectDynamically(InjectorBuilder.java:171)
at com.google.inject.InjectorBuilder.build(InjectorBuilder.java:113)
at com.google.inject.Guice.createInjector(Guice.java:92)
You can reproduce this by adding a multibinder like this:
public void configureAdditionalHover(com.google.inject.Binder binder) {
Multibinder<org.eclipse.jface.text.ITextHover> hoverBinder = Multibinder.newSetBinder(binder, org.eclipse.jface.text.ITextHover.class);
hoverBinder.addBinding().to(org.eclipse.xtext.common.types.xtext.ui.JdtHover.class);
//binder.bind(org.eclipse.jface.text.ITextHover.class).annotatedWith(com.google.inject.name.Names.named(org.eclipse.xtext.ui.editor.hover.DelegatingHover.ADDITIONAL_HOVER)).to(org.eclipse.xtext.common.types.xtext.ui.JdtHover.class);
}
I guess this is the first time someone tried to use a multibinder within Xtext.
(In reply to comment #8) > I guess this is the first time someone tried to use a multibinder within Xtext. We have tried them but I like the current approach more, because with that I can override (remove) an addition to a multibinding in submodules again. It's not optimal to inject the injector, but as it is pretty much encapsulated it's not such a big problem. Btw.: Thanks for the contribution. This is a nice feature. We'll add it as soon as the SR1 is out and we have migrated to git. > Btw.: Thanks for the contribution. This is a nice feature. We'll add it as soon
> as the SR1 is out and we have migrated to git.
Thx, Sven. Just let me know if you have any issues with this patch I can help with.
*** Bug 320818 has been marked as a duplicate of this bug. *** postponed to M3 Created attachment 179760 [details]
Patch adapted to HEAD
Adapted the patch to HEAD. Will be revised shortly.
I've finished revising your patch. Very impressive work, thank you! I am sure I've missed a lot of points, but I have a few (as it is a big patch quite a few) observations, which I put in the end of this comment.
After all, it's a huge patch, covering multiple new features at once. I'd suggest to split it up into several smaller bugs, add a few tests :-), and discuss the individual issues in separately, e.g. EObject documentation, Hover delegation, HTML hovers, JDT hover integration, etc. Could you do that?
Here are my observations on the existing patch:
1) DelegatingHover:
I wouldn't add hovers using multibinding as you cannot specify their order. I'd rather provide a method in which the list of hovers is filled, such that the user can override it. I'd also skip the notions of default/priority hovers, as users might want to change that. Maybe we should skip the binding part for hovers for the sake of simplicity and inject instances by their concrete classes instead, e.g.
DelegatingHover {
@Inject SomeHoverClass hover0;
@Inject OtherHoverClass hover1;
…
protected List<ITextHover> createHovers() {
List<ITextHover> hovers = Lists.newArrayList();
hovers.add(hover0);
hovers.add(hover1);
return hovers;
}
...
2) You should not pass EObjects/AbstractNodes as a result of an IUnitOfWork, e.g. in AbstractXtextEditorHover.getXtextElementAt(ITextViewer, int) as these could change concurrently thus becoming invalid. You have to put all calculation on these inside the unit. If you just need offset and length, consider using a (Text)Region.
3) I'd expect AbstractXtextEditorHover (maybe better named AbstractTextHover) to inherit form AbstractHover. If there is nothing in common, we should rename AbstractHover or eliminate it.
4) AnnotationWithQuickFixesHover
Please document what is copied and what is changed.
5) AbstractHtmlHoverContentProvider
I don't understand the delegation mechanism. Do you use it? If not, skip it in the first place.
6) IHtmlHoverContentProvider:
Can there be non-EObject's ? If not, this interface looks pretty much like the IEObjectDocumentationProvider.
7) XtextBrowserInformationControlInput:
Please don't use internal API.
8) TypesGeneratorFragment
Class name should be JdtHover, not JDTHover.
9) How about some tests? :-)
(In reply to comment #14) > I've finished revising your patch. Very impressive work, thank you! I am sure > I've missed a lot of points, but I have a few (as it is a big patch quite a > few) observations, which I put in the end of this comment. Jan, thanks for looking at the patch and providing your comments. > After all, it's a huge patch, covering multiple new features at once. I'd > suggest to split it up into several smaller bugs, add a few tests :-), and > discuss the individual issues in separately, e.g. EObject documentation, Hover > delegation, HTML hovers, JDT hover integration, etc. Could you do that? Yep, good idea. Sven also proposed this some time ago. Don't know how fast I can provide those individual patches. What about creating an xtext clone on github where we can work on the hover stuff in separation instead of juggling with a lot of patches? > Here are my observations on the existing patch: > > 1) DelegatingHover: > I wouldn't add hovers using multibinding as you cannot specify their order. I'd > rather provide a method in which the list of hovers is filled, such that the > user can override it. I'd also skip the notions of default/priority hovers, as > users might want to change that. Maybe we should skip the binding part for > hovers for the sake of simplicity and inject instances by their concrete > classes instead, e.g. > > DelegatingHover { > @Inject SomeHoverClass hover0; > @Inject OtherHoverClass hover1; > � > > protected List<ITextHover> createHovers() { > List<ITextHover> hovers = Lists.newArrayList(); > hovers.add(hover0); > hovers.add(hover1); > return hovers; > } > ... The idea is to have the configuration decentralized and allow different generator fragments to contribute hovers. E.g. the TypesGeneratorFragment should be able to add the JdtHover. Not sure if this is still possible with the centralized method. > 2) You should not pass EObjects/AbstractNodes as a result of an IUnitOfWork, > e.g. in AbstractXtextEditorHover.getXtextElementAt(ITextViewer, int) as these > could change concurrently thus becoming invalid. You have to put all > calculation on these inside the unit. If you just need offset and length, > consider using a (Text)Region. Ok, I'll have to look at this. > 3) I'd expect AbstractXtextEditorHover (maybe better named AbstractTextHover) > to inherit form AbstractHover. If there is nothing in common, we should rename > AbstractHover or eliminate it. Yep. See initial comment (5). Potential clean-up: Remove XtextTextHover, remove AbstractHover, derive ProblemHover from AbstractXtextEditorHover I think this should be postponed until all changes on the hover stuff are completed. Then the clean up should happen. > 4) AnnotationWithQuickFixesHover > Please document what is copied and what is changed. Can do. > 5) AbstractHtmlHoverContentProvider > I don't understand the delegation mechanism. Do you use it? If not, skip it in > the first place. Just copied from AbstractLabelProvider, can be removed. > 6) IHtmlHoverContentProvider: > Can there be non-EObject's ? If not, this interface looks pretty much like the > IEObjectDocumentationProvider. The IEObjectDocumentationProvider provides the raw text documentation for EObjects. The IHtmlHoverContentProvider returns html content for an object and may transform the result from the IEObjectDocumentationProvider into html. You are correct, that the HtmlHoverContentProvider deals only with EObjects by default. A possible scenario where objects are used, is when following special links in the html content, which may lead to objects insted of eobjects. Have to look at this in more detail. > > 7) XtextBrowserInformationControlInput: > Please don't use internal API. Not sure how to solve this without using internal API. JDT's JavadocBrowserInformationControlInput is doing it the same way. Same problem at XtextHtmlHover. Any ideas? > 8) TypesGeneratorFragment > Class name should be JdtHover, not JDTHover. Yep, will change > 9) How about some tests? :-) Will make a lot of sense. (In reply to comment #15) > Yep, good idea. Sven also proposed this some time ago. Don't know how fast I > can provide those individual patches. What about creating an xtext clone on > github where we can work on the hover stuff in separation instead of juggling > with a lot of patches? Yes, let's do it. When the contribution is finished, it has to go through the IP process (because of its size). > Yes, let's do it. > When the contribution is finished, it has to go through the IP process (because > of its size). Setup an xtext clone at http://github.com/ckulla/xtext-hover/. There are two branches, master and hover. Master is a clone of the xtext repositroy, hover will provide changes regarding to this bug. This let's us update the master with changes from the regular xtext repository independent of changes in the hover branch. I applied the updated patch from Jan to the hover branch. Will do some changes according to Jan's comments in the near future. Issues 1)-6), 8) are fixed on http://github.com/ckulla/xtext-hover/commits/hover . Thanks for the review comments, the code is now much more concise. I don't see a solution for Issue 7) unless changing the jface code to make the package org.eclipse.jface.internal.text.html public available. The JDT code faces the same issue (see JavadocBrowserInformationControlInput). Tests are still missing, hope to add some in the next days. Created attachment 183535 [details]
Hover patch, refactored according to review, added tests
Added the result from refactoring according to Jan's comments. Please let me know if anything should be changed, if not this is the final version of the patch.
Thanks, will look into it tomorrow. I've filed a CQ for the patch (http://dev.eclipse.org/ipzilla/show_bug.cgi?id=4658) I authored 100% of the content (minus the reuse of Eclipse JDT source) I have the rights to contribute the content to Eclipse I contribute the content under the EPL Please note that the patch contains a commit history from https://github.com/ckulla/xtext-hover/tree/hover. I created a branch from the xtext repository and developed the patch there. The first commit message on that branch is from Jan Koehnlein. Jan did a review of my initial patch[1] and modified it only slightly [2]. I then started from there to resolve all review comments. The result is in [3]. [1] https://bugs.eclipse.org/bugs/attachment.cgi?id=174786 [2] https://bugs.eclipse.org/bugs/attachment.cgi?id=179760 [3] https://bugs.eclipse.org/bugs/attachment.cgi?id=183535 pushed the patch, made implementation use and lookup language specific hover providers. Closing all bugs that were set to RESOLVED before Neon.0 Closing all bugs that were set to RESOLVED before Neon.0 |