Community
Participate
Working Groups
We need an extension point as a way for third-party plug-ins to contribute content to Ctrl-E.
Created attachment 54110 [details] extension patch This patch has my most recent work on making CtrlEAction extensible. I've been having some issues with how we handle commands in it, but made a toString and compareTo method in AbstractElement that works for the sorted set. However, all I'm getting to display in the dialog are nodes and not their associated elements as well. Still have some work to do on it, but it's getting closer to being done. AF
Created attachment 54198 [details] extension patch This patch is closer to what we want, still some touchups needed though. AF
Created attachment 54203 [details] updated patch
Created attachment 54208 [details] extension patch This patch is a rewrite of the functionality of the CtrlEAction dialog using abstract classes that can be extended. AF
Created attachment 54365 [details] persistance and extension patch This patch has the functionality of the previous patch, but also includes persistance for bug 162005. AF
Patch (with some minor changes like Javadoc and visibility) released to HEAD >20061122.
Created attachment 54438 [details] persistance and extension patch This patch improves upon the last patch for adding persistance and extensibility to Quick Access. The nested loop in restoreDialog has been replaced by adding a getter method to the AbstractElement class so it is possible to obtain the id of its provider class. It also persists the AbstractProvider array across the same session so it is only created the first time Quick Access is used. This patch also contains changes that were made to CtrlEAction for bug 165375, so the patch for that bug should be applied before this. AF
Moving to M5.
Noticed that Ctrl+E doesn't currently include import/export wizards (in 3.3 M3). This would be a good candidate to test the extensibility mechanism ;-).
Launch configurations are another one we should have in there.
Taking ownership of Aaron's remaining bugs.
Deferring. It is unclear how we can do this without causing unwanted plug-in activation.
Remy is now responsible for watching bugs in the [QuickAccess] component area.
*** Bug 204356 has been marked as a duplicate of this bug. ***
*** Bug 217062 has been marked as a duplicate of this bug. ***
This is from a comment previously added to bug 433640: I created a new bookmark type and was hopping I would be able to add them to the scope of the QuickAccess list. The existing Eclipse code is really clean and straight-forward to extend. Aside from the broad issue of performance, does someone in the UI group have some preference as to what would be a acceptable way to extend the quickaccess? At the moment QuickAccessElement and QuickAccessProvider are internal classes. The road of minimal disruption to existing code seems to be to expose a couple of interfaces with a new extension point. Alternatively the classes could simply be moved org.eclipse. If the mechanism is limited to E4, then I would imagine the natural fit would be to implement a indexing service to which external contributors could be added/removed. Defining a IQuickAccessElement would have the added benefit of allowing external types to be adapted rather than mandating to contribute pre-backed QuickAccessElement instances. Either way, I would imagine that a preference page would be useful to enable/disable some of the categories from the result, again for obvious perf reasons. Enhancement might also include a decorator to give a hook into the presentation itself. ps: I started looking at this via "bug 436913"
(In reply to L. Mihalkovic from comment #16) > At the moment QuickAccessElement and QuickAccessProvider are internal > classes. The road of minimal disruption to existing code seems to be to > expose a couple of interfaces with a new extension point. This makes sense to me. Stating what I think is the obvious, I'm thinking IQuickAccessProvider and IQuickAccessElement as new public interfaces and an extension point "org.eclipse.ui.quickaccessprovider". Naturally, we'll also require some tests and documentation. > Alternatively the > classes could simply be moved org.eclipse. If the mechanism is limited to > E4, then I would imagine the natural fit would be to implement a indexing > service to which external contributors could be added/removed. That sounds like a separate issue. Or am I missing something? > Defining a IQuickAccessElement would have the added benefit of allowing > external types to be adapted rather than mandating to contribute pre-backed > QuickAccessElement instances. +1 > Either way, I would imagine that a preference page would be useful to > enable/disable some of the categories from the result, again for obvious > perf reasons. Enhancement might also include a decorator to give a hook into > the presentation itself. I'm inclined to think of this as a separate issue. Maybe, after we've implemented the first bit, we can decide if we really need to make this customizable. I could also envision a desire to encourage a particular ordering of results. I'm planning to take a stab at implementing this over the next few days. Unless, that is, somebody else has made any progress...
(In reply to Boris Bokowski from comment #6) > Patch (with some minor changes like Javadoc and visibility) released to HEAD > >20061122. I mistakenly marked the most recent attachment at iplog+. Based on this comment, one of the attachments needs to be so-marked. (In reply to Aaron Ferguson from comment #7) > This patch improves upon the last patch for adding persistance and > extensibility to Quick Access. The nested loop in restoreDialog has been > replaced by adding a getter method to the AbstractElement class so it is > possible to obtain the id of its provider class. It also persists the > AbstractProvider array across the same session so it is only created the > first time Quick Access is used. This patch also contains changes that were > made to CtrlEAction for bug 165375, so the patch for that bug should be > applied before this. After reading this more carefully, I realize that Aaron may have already done the extensibility work. I'll start from this patch.
(In reply to Wayne Beaton from comment #18) > After reading this more carefully, I realize that Aaron may have already > done the extensibility work. I'll start from this patch. There has been significant code drift since this patch was provided. I'm abandoning any effort to apply it. -- The ViewProvider subclass of QuickAccessProvider takes parameters from the SearchField in the constructor, and a listener tinkers with the CommandProvider subclass. This makes it challenging to fully generalize the acquisition of providers. Rather than fully generalizing the providers, it would be relatively easy to insert a some code to just add to the providers list using an extension point. That is, keep the existing ones where they are and hard-code their inclusion in the providers list; add extended providers to that hard-coded list. Pros: easy hack. Cons: we cannot handle providers coming and going as bundles are activated/deactivated. To handle the dynamic case, more significant restructuring of the code will be required. Maybe that's version 2. It also might be worth considering passing the context to all providers as they are created and when the SearchField gets focus. This should give ViewProvider and CommandProvider what they need and give other providers the opportunity to participate. Further, this would let us make the "build-in" provider dynamic should we decide that there's value in doing so.
I've pushed a change to Gerrit. https://git.eclipse.org/r/#/c/39340/ This commit introduces IQuickAccessProvider, IQuickAccessElement,and QuickAccessMatch into the public API along with a new quickAccess extension point. The two interfaces are, I think, pretty obvious. I needed to create the new type because quick access elements, as part of the match method had previously answered an instance of QuickAccessEntry which includes a lot of internal behaviour. The new type separates the matching parts from the measuring/displaying parts. I decided to keep the "built-in" quick access providers hard-coded as at least two of them are manipulated in very specific ways. Ideally, all quick access providers will be handled uniformly, but this will require some significant refactoring. The JUnit tests for Quick Access seem to be commented out. I will investigate is further to see if I can sort that out.
I've been stewing over a decision that I made to follow the lead of the ViewProvider class in setting up the IQuickAccessProvider interface. Instead of passing the provider the MApplication and MWindow separately, we should instead just pass the IEclipseContext (which I assume includes both the application and window). This conversation feels a little one-sided. Is anybody out there actually listening?
I pushed a small update to the patch. https://git.eclipse.org/r/#/c/39340/ I made a change to IQuickAccessProvider; instead of passing the MWindow and MApplication (as inspired by ViewProvider), I pass the the IEclipseContext to the providers instantiated by via the extension point.
Created attachment 250041 [details] Web search plug-in Attached is a plug-in that I've cobbled together to test the extension point. I still need to integrate this (or something like it) into an actual unit test. This plug-in provides two extensions related to web search: first, it adds a Quick Access provider that adds an option to search the web for whatever's been typed; second, it provides a selection handler that lets the user web search on any selected text (unrelated to quick access). The implementation is rough (with some duplicate code, some hard-coded stuff, missing file headers, and some other gaps). One thing that this example highlights, I think, is the need for a provider to be able to opt-out (or opt-in) to being remembered as a "previous pick"; it's a little weird to see the same option twice in the dialog (even though this is consistent with some of the other providers). A potentially handy other example might be to include a quick access to find references/declarations of a Java type or method; though I think some careful consideration of the UI impact would be in order (e.g. perhaps only show these options if a Java editor is on top).
(In reply to Wayne Beaton from comment #21) > This conversation feels a little one-sided. Is anybody out there actually > listening? I do enjoy seeing activity on this feature request. :-) Have you thought about the problem of plug-in activation mentioned in comment 12?
(In reply to Boris Bokowski from comment #24) > Have you thought about the problem of plug-in activation mentioned in > comment 12? I have not. The option that jumps immediately to mind is to avoid building the providers until they are actually required. We could address the issue of bundle activation/deactivation at the same time. I'll see what I can come up with.
Dani pointed out some flaws in my contribution. I'll fix them and address the activation issue. I'll commit to having a contribution by M5.
Quick update: I have a working implementation that addresses the bundle activation issue. I decided to inject an indirection layer as this gave me the ability to avoid a massive restructuring of the code and made it really easy to provide static information as attributes on the extension. It seems to work well, but I'm going to test it a little more and add better comments on the public interfaces before I resubmit. By leveraging attributes on the extension point, some of the methods on the interface provide that same information are now moot, so I'll need to sort out how to address that. I still need to sort out how to address dynamic bundle (un)installs. It also occurs to me that we might need to add another attribute that controls whether or not a provider's elements should ever be "previous picks"; my example with a websearch looks a little weird (the search appears twice the second time around).
Another quick update: > I decided to inject an indirection layer as this gave me the ability to > avoid a massive restructuring of the code and made it really easy to provide > static information as attributes on the extension. I realized that the creation of a class that represents the contribution gave me an opportunity to undo some of the changes that I had made in existing code. In particular, I managed to undo every change that I had made in the existing providers. > By leveraging attributes on the extension point, some of the methods on the > interface provide that same information are now moot, so I'll need to sort > out how to address that. I've managed to trim down the interfaces to a relatively small group of methods. > I still need to sort out how to address dynamic bundle (un)installs. This seems to be a relatively hard problem. At this point, I believe some significant refactoring is required to make this happen. I'm still confident that I'll be able to have a contribution by the self-imposed Feb. 13 deadline.
I had it in my head that M5 was on Feb. 13. (I guess that shows how much of my energy is required to make the simultaneous release successful). I'm pushing forward with a Feb. 13 contribution date. (In reply to Wayne Beaton from comment #26) > I'll commit to having a contribution by M5. (In reply to Wayne Beaton from comment #28) > I'm still confident > that I'll be able to have a contribution by the self-imposed Feb. 13 > deadline.
I think that I've addressed Dani's concerns with this latest patch. https://git.eclipse.org/r/#/c/39340/ Things to look for: The implementation avoids activating a bundle until it's required. I decided to capture a lot more information in the extension point to avoid needing to have access to class. I moved the extension point stuff into the QuickAccessContents class which is shared by both SearchField (e4) and QuickAccessDialog (3.x). I added registry listeners that clean up when implementations of IQuickAccessProvider are added or removed from the extension registry. I had to add two listeners: one on the quick access contents to control the list of providers available, and one on the SearchField/QuickAccessDialog (so, three in total actually) that clean up the previousPicks state when changes occur. I ended up hardcoding the plug-in id when I install the listeners since I couldn't find the value handy anywhere and didn't want to disturb the activator. I've tried to keep changes to a minimum. I did move a Map that was represented as a field to a local variable in the one location that it was used. The alternatives was to try and keep it up to date via the registry change listener and that seemed a little excessive. I've tested it live, using the console to dynamically kill off and then start bundles with contributions. I've used the debugger to verify that it's doing what it's supposed to be doing. I got a little bogged down with unit tests. The existing tests for QuickAccess have been removed from the test suite. When I run them independently, there are many failures without my changes (i.e. they were broken before I got here). I'll try to get them running. At the same time, I've started building some tests for the new functionality.
Also *if* we should ship this I would like that we also ship at least one (but better two) implementations for most prominent use cases for the new contribution: searching for files and searching for types (this one should be of course in JDT). Other thing coming to my mind immediately: would be cool if we could contribute here search for opened/recently closed editors too. Also I think it makes sense to provide a preference page where the user can disable "not interesting" QA providers (or those who are buggy or slow). Additionally I also imagine that each contributed provider could place it's own subpage to that QA preferences root, so that if someone want search for source types only in JDT, can select a checkbox and be happy. Another thing: let assume we will have lot of QA provider contributions, which deliver TONS of matches on a simple short keyword. I think users could also wish to restrict current search to the one specific provider, something like browsers it do: give each provider a key and start the search with that key. So if I'm looking for a file (and we have a "resources search" contribution associated with "res" key), the "res: thatFile" will restrict the QA search to the resources search only. To configure it, we would need an optional argument in the extension point and (of course) the preference page I mentioned above, where the users can configure that as they like. Wayne, what was the reason for you to stop working on this feature? Are you planning to drive your patch further (goal would be 4.7, since new API is too late for 4.6) or should we return it to the UI team bug pool?
(In reply to Andrey Loskutov from comment #31) > Wayne, what was the reason for you to stop working on this feature? Are you > planning to drive your patch further (goal would be 4.7, since new API is > too late for 4.6) or should we return it to the UI team bug pool? Laziness mostly. The code moved under my patch and I was looking at a messy merge. Then this squirrel ran past my window... I will, however, commit to pushing on the basic functionality. I like some of your suggestions in Comment 31. Let's explore that after the basic functionality is in place.
(In reply to Wayne Beaton from comment #32) > (In reply to Andrey Loskutov from comment #31) > > Wayne, what was the reason for you to stop working on this feature? Are you > > planning to drive your patch further (goal would be 4.7, since new API is > > too late for 4.6) or should we return it to the UI team bug pool? > > Laziness mostly. Not just that. I was supposed to review the latest patch set, but there was always something more important on my list. Sorry. Andrey, can you take up the review role?
(In reply to Dani Megert from comment #33) > (In reply to Wayne Beaton from comment #32) > > (In reply to Andrey Loskutov from comment #31) > > > Wayne, what was the reason for you to stop working on this feature? Are you > > > planning to drive your patch further (goal would be 4.7, since new API is > > > too late for 4.6) or should we return it to the UI team bug pool? > > > > Laziness mostly. > > Not just that. I was supposed to review the latest patch set, but there was > always something more important on my list. Sorry. > > Andrey, can you take up the review role? Yes, but for sure you mean targeting 4.7?
(In reply to Andrey Loskutov from comment #34) > (In reply to Dani Megert from comment #33) > > (In reply to Wayne Beaton from comment #32) > > > (In reply to Andrey Loskutov from comment #31) > > > > Wayne, what was the reason for you to stop working on this feature? Are you > > > > planning to drive your patch further (goal would be 4.7, since new API is > > > > too late for 4.6) or should we return it to the UI team bug pool? > > > > > > Laziness mostly. > > > > Not just that. I was supposed to review the latest patch set, but there was > > always something more important on my list. Sorry. > > > > Andrey, can you take up the review role? > > Yes, but for sure you mean targeting 4.7? Definitely.
I rebased and sorted out the merge conflicts. I've tested it locally, both by itself and with a test plug-in (I'll attach an update). I need to sort out how to get some tests going for this. AFAICT, all of the existing tests have been disabled. I'd also like to put together a real/useful extension that leverages this. I started investigating the potential to use it to maybe initiate a search within the workspace (e.g. search for a matching class). If I come up with something useful, I'll open a new bug.
Created attachment 262379 [details] Updated Websearch plug-in For testing purposes only.
(In reply to Wayne Beaton from comment #37) > Created attachment 262379 [details] > Updated Websearch plug-in > > For testing purposes only. This plug-in includes a command that's unrelated to the Quick Access extension. I noticed during testing that the command actually causes an exception when I uninstall the plug-in. I guess that I'd assumed that the command framework would be friendly to bundles loading and unloading, but I guess that it's not. My patch is friendly to bundles loading and unloading. Was including this in the implementation a mistake?
(In reply to Wayne Beaton from comment #38) > My patch is friendly to bundles loading and unloading. Was including this in > the implementation a mistake? I'm in the process of updating my patch. It'd be nice to have an answer to this question. Let me restate: With this patch, I'm adding a new extension point. Do I need to handle bundles coming and going?
(In reply to Wayne Beaton from comment #39) > With this patch, I'm adding a new extension point. Do I need to handle > bundles coming and going? I would say yes.
I tried a rebase, with https://git.eclipse.org/r/#/c/39340/16/ I verified this Introduces an NPE: at SearchField#restoreDialog(SearchField.java:678) at SearchField.createControls(SearchField.java:180) The structural problem: at SearchField.java:678 quickAccessContents is null. Analysis: QuickAccessContents is instantiated only @PostConstruct, when we can inject the context. See SearchField.java:143 : @PostConstruct void createControls(... See-also SearchField.java:220: quickAccessContents = new QuickAccessContents(providers, window.getContext()) Solution: Find a way to obtain an instance of context ASAP, to instantiate the QuickAccessContents as early as possible, passing an instance of IEclipseContext (ideally at startup?) Adding Brian in c/c as we discussed together the related bug 500773 .
Removing target milestone for all bugs that are not major or above.
> Removing target milestone for all bugs that are not major or above. Of course I meant "major or below". Sorry for the noise!
New Gerrit change created: https://git.eclipse.org/r/139736
New Gerrit change created: https://git.eclipse.org/r/139735
New Gerrit change created: https://git.eclipse.org/r/139739
New Gerrit change created: https://git.eclipse.org/r/139738
New Gerrit change created: https://git.eclipse.org/r/139737
New Gerrit change created: https://git.eclipse.org/r/139908
Gerrit change https://git.eclipse.org/r/139737 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=0cc9a0158918527916801633161a11787cc07e5d
Gerrit change https://git.eclipse.org/r/139735 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=ce78887d4537b192f216b3750a08e1aac22bccdd
Gerrit change https://git.eclipse.org/r/139736 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=c5a6a19c5b2c85001e251e47b19619e7a08514fa
Gerrit change https://git.eclipse.org/r/139738 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=a25d9a1c2138b86c2160b76a83454b43f895c0e0
Gerrit change https://git.eclipse.org/r/139908 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=ca624199af558689b3f341e5d91cbb59f641735f
*** Bug 443384 has been marked as a duplicate of this bug. ***
