Community
Participate
Working Groups
When using the extension page of the plugin editor it would be handy if the types returned when pressing the browse button were restricted to those that implement/extend the class specified in the basedOn filed of the attribute (if it has one). This would make it harder to put in classes that would fail runtime type checks.
will look into this as I think JDT provides facilities for this
I don't think so, Chris. This requires creating type hierarchies for every single type in the workspace. It takes seconds per type. For example, try to create a type hierarchy (F4) on any type and see how long it takes. So this operation is not desirable.
Couldn't you use the SearchEngine with a type heirarchy scope to do this? You only need to perform the search once for the base class and one each for the required interfaces.
*** Bug 157079 has been marked as a duplicate of this bug. ***
Even a single type hierarchy can take a long time, when you have big workspaces. I have seen it take a minute or so in some cases. Brian, have we quantitated ho bad it is for the Browse button scenario for a single type hierarchy? I know the performance would be beyond unacceptable bad for plugin.xml validation.
I don't think we ever got clear numbers on how bad the performance is. I did try to mock this up at one point. If I remember right, it took something over 5 times slower just to find classes that directly implemented an interface. On top of that, we would need to research for classes that extended each one of our search results (since subclasses should be valid). I think we estimated the performance to be worst than atleast 10 times, in order to get a pretty accurate list. That would mean waiting for more than 15-20 seconds when you hit browse, not very useful.
Seems to me that the performance would be == to opening the Hierarchy (F4) on the base type. Indeed, for the scenario mentioned in defect 157079 (browsing for an extension point implementation class) having a dialog that re-used the current code from the upper tree of the Hierarchy View would be preferable to what we have...
I am reopening the bug to take a look at what we might be able to come up with. If nothing comes of it, I will try to get quantified data.
thanks Brian. this would be a pretty useful function, so we need to get raw numbers using large workspaces/targets and then we can judge if the performance is prohibitive.
Created attachment 86628 [details] incomplete patch towards filtering I'm coming from #173428 ;-) First: how about adding something like "[Manifest][Editors]" to this bug's summary so it shows up in queries relating to the extension editor? Concerning building a subtype hierarchy: why do you write about searching all of the workspace? Doesn't it suffice to use the project scope for searching? An extension shouldn't try to provide classes from other plug-ins, right? Upper part of F4 is more than what you need, because it shows the structured tree, whereas the browse button only needs a flat list. I've played a little with the sources and it seemed something like the attached changes to ClassAttributeRow#doOpenSelectionDialog() could actually solve the issue. From my observation only two problems remain, which should be solved in cooperation with JDT: - the patch uses org.eclipse.jdt.internal.core.search.HierarchyScope => internal class, usage should perhaps be encapsulated in the JDT? perhaps JDT could offer a modified version of the new selectType() method? - this uses a FilteredTypesSelectionDialog (new in 3.3), which makes an assumption about search scopes, which in this scenario seems NOT to hold. As a symptom my modified dialog showed too many types (at least I've seen inner types of matching types) See comment in method FilteredTypesSelectionDialog#fillContentProvider(): /* * Setting the filter into match everything mode avoids filtering twice * by the same pattern (the search engine only provides filtered * matches). For the case when the pattern is a camel case pattern with * a terminator, the filter is not set to match everything mode because * jdt.core's SearchPattern does not support that case. */ While debugging I had to use one of the following two workarounds: 1) in method fillContentProvider() manually set typeSearchFilter.fMatchEverything to false, or 2) add one line at the bottom of org.eclipse.jdt.internal.core.search.TypeNameMatchRequestorWrapper#acceptType: if (scope.encloses(type)) (right before calling acceptTypeNameMatch()). Those surely are only workarounds, but if a JDT person looks at this, he might quickly provide a sound solution, I'd hope. Finally some error handling is still missing (e.g., in case the specified super type is not found..). In no situation did I observe delays that are anything close to a show stopper.
omg, it's back from the dead :) cc'ng Markus as he may be insights on how to deal with performance The PDE team has prototyped ages ago and wasn't happy with performance.
I will try to take a look at this tomorrow. This would be a very handy bug to fix.
Created attachment 86635 [details] patch to TypeNameMatchRequestorWrapper.java With minor revisions I intergrated my patch into our system (see below), meaning: it seems safe to me and works just nice. (One fine point: the dialog includes the focus type (basedOn) and its super types up-to Object, which it should not, but that's well tolerable, I think ..) For your convenience I attach an improved patch for TypeNameMatchRequestorWrapper.java, which IMHO failed to test the scope wrt. member types. FYI: The next release of our tool (OTDT www.objectteams.org/#distrib) will already contain this feature, which we can do by adapting the PDE using the adaptation technique of OT/Equinox.
Created attachment 86740 [details] update to incomplete patch towards filtering Stephan, was this working on your machine? I could not seem to get the dialog to actually find any classes. I am attaching an updated patch of your work. I went ahead and filled in the basedOn value and update PDEJavaHelperUI so we don't have to copy as much code into ClassAttributeRow. My test was to create a simple PDE project with the views template. Then go to the plugin.xml and remove the class that defined the view. Then clicked browse. Since a view must implement ViewPart (which SampleView did), I expected to see it in the dialog. Please let me know if I am testing this incorrectly. Thanks for your help with this bug. This is looking extremely promising and I am very much excited since I think this will be some very useful functionality!
(In reply to comment #14) > Stephan, was this working on your machine? I could not seem to get the dialog > to actually find any classes. One curiosity I observed: you have to type a '.' to get the full list of matching classes. An empty input field will cause the dialog to display only classes that have been selected previously (cached). Typing some letters should work as expected: show only types which match the hierarchy search AND the name pattern. I'm not sure, where the '.' interpretation is implemented and whether this is some (new?) standard for different filtered dialogs. Perhaps one could initialize the String filter to '.' rather than filling in the previous value?? I might also remember that the very first time I used the new version the search was not working, like it was using some stale cached info (showing a list which in now way related to the search criterion). But after the first use this behavior was no longer reproducable. Other than that, yes, it works really nice on my machine. Hope this helps. PS: cool thing you found the factory method for hierarchy scopes ;-)
Two updates: The '.' trick seems to work by accident only. Seemingly "**" would be the correct pattern that should be passed as the 'filter' argument to JavaUI.createTypeDialog. The hierarchy scope still contains too many elements. I've opened bug 215139 against JDT/Core [search] as a wish for more fine grained API. If that is granted, the call should be SearchEngine.createHierarchyScope(javaProject, superType, true); rather than SearchEngine.createHierarchyScope(superType); This will constrain the search to the current project and to subtypes of the given type (currently the whole hierarchy down from Object is used).
"**" is the recommended pattern if you really want to show all types initially. You could add this in ClassAttributeRow.doOpenSelectionDialog(), e.g. like this: String filter = text.getText(); if (filter.length() == 0) filter = "**"; //$NON-NLS-1$ I filed bug 215196 for JDT/Core for open issues with hierarchy scopes in the presence of nested types. These are a bit hard to track down unless you know exactly when the FilteredTypesSelectionDialog uses the cache and when it uses the search engine... Otherwise, I think the patch from comment 14 would be nice, but only when the API requested in bug 215139 can be used (i.e. I would rather live with too many types from the right project than with subtypes from other projects).
Fantastic job Stephan! I thought it was not working on my computer because I didn't start typing and it only brought up the entries for the history only :) I went ahead and submitted the code. When bug 215139 is fixed, we can modify the code as needed. The only other outstanding issue that we might have is when a schema specifies a class that must be extended and an interface which must be implemented. For right now, I wrote the logic to build the hierarchy for the class, but it would be nice to handle this case some day. I think to handle this correctly, we would need an enhancement from the JDT. Not sure if you saw, but this bug has been open for 3.5 years. Thanks for a lot for your help in implementing it! If you would like to join the elite team of PDE contributors (http://www.eclipse.org/pde/pde-ui/committers/committers.php), please send me a head shot with a leafy back ground and we will make sure we put you up there.
Thanks Stephan!
I am going to back out the changes we made for bug 61185 due to bug 218223. I think the fix for this bug is to check to see if the superclass/interface is java.lang.Object. If so, then go ahead and use the "old" way of search. Otherwise, search with the enhanced functionality.
(In reply to comment #20) > I think the fix for this bug is to check to see if the superclass/interface is > java.lang.Object. If so, then go ahead and use the "old" way of search. > Otherwise, search with the enhanced functionality. I totally agree. Creating the full type hierarchy for java.lang.Object makes no sense, so a simple if (superTypeName != null && !"java.lang.Object".equals(superTypeName)) { should do the trick, shouldn't it? Well, thinking twice there are a few other candidates that *could* cause trouble: java.io.Serializable java.lang.Cloneable perhaps even java.lang.Throwable other? After a few quick experiments, these may indeed create a relevant delay, but I don't think in the order of blowing an OOME. Would setting a timeout be overkill?
> Would setting a timeout be overkill? I think that might be overkill for right now. Lets start simple and see if we can satisfy the communities needs. If we cannot achieve this with a simple algorithm, we can then make it more advanced when we need to.
Brian, after bug 215139 has missed the train at 3.4 API freeze, wouldn't you like to re-enable this patch with just the additional check re java.lang.Object as discussed in comment 21? I guess it makes sense to treat basedOn="java.lang.Object" as no bound at all. I just made an experiment with basedOn=":java.io.Serializable" and found that you probably meant to strip off the leading ":" in ClassAttributeRow#doOpenSelectionDialog ? Like if (index == 0) superName = superName.substring(1); (the case where only an interface is specified). Well, doing the search for Serializable takes a long time. But neither does it crash Eclipse nor does Serializable feel like a useful bound in 'basedOn'? For the records: ---------------- This patch still finds too many types: + super types of the focus type + types in other projects + enclosing types of relevant types Yet, it significantly improves the situation over providing a full unfiltered list of types.
Created attachment 95942 [details] org.eclipse.pde.ui.patch An updated patch... to include ignoring java.lang.Object when creating hierarchy...
I think we can get away with just ignoring java.lang.Object here. I just went through the SDK and tried various extensions along with creating some of my own using things like java.lang.Serializable and other java.lang variants. In 3.5, we should revisit this when JDT drops changes in bug 215139 or bug 215196
Created attachment 95950 [details] mylyn/context/zip
Reopening to raise visibility, since recent activity on bug 215139 and related looks like a chance to fix remaining issues listed in comment 23.
No plans to work on this.
Created attachment 148812 [details] patch for consuming bug 215139 As bug 215139 has reached the home stretch, here comes the tiny patch by which we would like to consume the new API. Once committed this fully resolves this bug.
Thanks Stephan :)
done. > 20090914
Verified in I20091027-0100.
