Community
Participate
Working Groups
We've built a "subwords" completion engine for JDT (see bug #350000 and http://goo.gl/1MzbO for details). The engine scores the overlap between the user-entered prefix token and the proposal text using "bigrams". In a nutshell, it counts the number of similar pairs between a prefix token and a proposal to determine its relevance. This engine works well if the user triggers code completion and picks a proposal w/o any further typing. However, if a user continues typing with an open completion popup window, the score of each subwords proposal changes because of the changing amount of similar pairs a completion shares with the prefix. JDT completion system does not support such relevance updates. When a user enters a new character it just checks whether the completion is still applicable, and if not removes it from the view. To make subwords completion work well with JDT the completion system must reorder proposals by their updated relevance, i.e., re-run the relevance sorter on the available proposals. I would like to request such a change to JDT completion system. Best, Marcel
For JDT, the motivation of computing a relevance number for each proposal is based on say, whether it the proposed type or variable is final, keyword, static, exact match, etc. depending on the context. There is currenly no scoring for say, no. of matching letters of a subword, etc. So, even when the user continues to type more letters, the relevance for a given proposal will only be affected, if say, user has typed ABC and there exists a type ABC (it becomes an exact match here). So, i'm not too sure how JDT/Core can help here. Did you explore the option of relevance scoring for subwords in your requestor that you use to collect proposals from JDT/Core. Or do you use JDT/UI's completion requestor and do something on top of it?
Note that JDT/UI, after collecting all proposals from JDT/Core, also performs more actions on the proposals, such as sorting alphabetically, etc.
(In reply to comment #1) > Or do you use JDT/UI's completion requestor and do something on top of it? That's what we do. We use "cu.codeComplete(offsetBeforeTokenBegin, requestor);" and collect all proposals that match the prefix-as-regex. Then we add our relevance scoring on top of the jdt proposals. The relevant code is in this package: http://goo.gl/rXL53 It's totally clear to me that JDT had no need for this kind of re-scoring so far. For subwords as implemented by now, however, this is essential. I see two ways to continue: 1. we stick with the existing JDT approach and use simple regex or exact subwords matching with constant relevance score. 2. instead of only _filtering_ proposals when typing continues, jdt also re-runs the ordering mechanism. Solution 1 is straight-forward and simple. Solution 2 has the potential to better fit what a developer wants. I wonder how difficult it is to add a call to re-order proposals in addition to filtering?
Ayushman, your statement "So, i'm not too sure how JDT/Core can help here." made me curious and I digged a bit deeper into the completion system. The code that does the filtering is in org.eclipse.jface.text.contentassist.CompletionProposalPopup.computeFilteredProposals(int, DocumentEvent). As a first guess, this is the location where a re-sorting by relevance should take place. Unfortunately, #getRelevance is defined in IJavaCompletionProposal and not in any ICompletionProposalExtension. Thus, I couldn't write a simple resorter w/o adding a dependency to JDT (which wouldn't work at all). So here I get stuck. Implementing a re-scoring would require to replace or change CompletionProposalPopup, right?
(In reply to comment #4) > Ayushman, your statement "So, i'm not too sure how JDT/Core can help here." > made me curious and I digged a bit deeper into the completion system. > > The code that does the filtering is in > org.eclipse.jface.text.contentassist.CompletionProposalPopup.computeFilteredProposals(int, > DocumentEvent). As a first guess, this is the location where a re-sorting by > relevance should take place. Yup. This is what I meant. JDT/Text is a more relevant area for this. > Implementing a re-scoring would require to replace or change > CompletionProposalPopup, right? Dani, what do you think is the best way to implement the re-scoring mechanism?
JDT Core can't do anything here. They should not start to change the relevance based on outside context. One question: is recomputing the ranking enough or do you also get more proposals when the user types more characters? I assume re-ranking means resorting? From a usability standpoint this might be tricky and use to a noisy UI.
(In reply to comment #6) > One question: is recomputing the ranking enough or do you also get more > proposals when the user types more characters? It's a regex match that is a "monotone" filter: every new character leads to the same or less proposals. > I assume re-ranking means resorting? Yes. The score of a proposal changes the more characters are written and the bettet the subwords matching is. > From a usability standpoint this might be > tricky and use to a noisy UI. Yes, it might be. Completion engines should use this feature sparsely.
(In reply to comment #7) > (In reply to comment #6) > > One question: is recomputing the ranking enough or do you also get more > > proposals when the user types more characters? > > It's a regex match that is a "monotone" filter: every new character leads to > the same or less proposals. That fits well into the architecture. Having said that, the way content assist works is: 1. get the proposals from the processors in a certain order. 2. let the pop-up filter based on typed characters Step 1 also uses the contributed sorters which we would have to rerun somehow. The main problem is that the general content assist framework doesn't deal with sorting at all. To fix this, we would have to introduce the concept of sorting to the content assist framework by adding several new APIs on the content assistant and the processor. From here you would have two choices: 1) If you use your own proposals then you can return a different value when the proposal is asked for its relevance when re-sorting. 2) If you use the existing (JDT Text) proposals you could contribute your own relevance sorter that takes the sub-word matching into account. What do you think?
(In reply to comment #8) > From here you would have two choices: > 1) If you use your own proposals then you can return a different value when the > proposal is asked for its relevance when re-sorting. > > 2) If you use the existing (JDT Text) proposals you could contribute your own > relevance sorter that takes the sub-word matching into account. > > What do you think? Both works. However, I would prefer the former (allow proposals to update their relevance value. In most cases this may be solved by another run of the existing relance sorter. A new sorter (i.e., externalizing the score computation out of the completion proposal) may also work out well. In this case, however, JDT proposals should provide more access their internal state. For instance, I need access the to internal proposal created from the completion engine in many cases. BTW: making this accessible from JavaCompletionProposal would be nice. It doesn't need to be part of the IJavaCompletionProposal interface.
(In reply to comment #9) > (In reply to comment #8) > > From here you would have two choices: > > 1) If you use your own proposals then you can return a different value when the > > proposal is asked for its relevance when re-sorting. > > > > 2) If you use the existing (JDT Text) proposals you could contribute your own > > relevance sorter that takes the sub-word matching into account. > > > > What do you think? > > Both works. However, I would prefer the former (allow proposals to update > their relevance value. OK, but this is something you can already do now assuming the sorting is triggered.
(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > From here you would have two choices: > > > 1) If you use your own proposals then you can return a different value when the > > > proposal is asked for its relevance when re-sorting. > > > > > > 2) If you use the existing (JDT Text) proposals you could contribute your own > > > relevance sorter that takes the sub-word matching into account. > > > > > > What do you think? > > > > Both works. However, I would prefer the former (allow proposals to update > > their relevance value. > OK, but this is something you can already do now assuming the sorting is > triggered. I think so too. This should be the minimal intrusive change to get this working.
Is there anything I could/should deliver?
(In reply to comment #12) > Is there anything I could/should deliver? As mentioned in comment 8 we have to introduce the concept of sorting to the content assist framework by adding several new APIs on the content assistant and the processor. If you know how/where to do this, a patch would be welcome otherwise I'll try to do it for 3.8 - but no promise.
What (In reply to comment #13) > If you know how/where to do this, a patch would be > welcome otherwise I'll try to do it for 3.8 - but no promise. What's the deadline for this to make it into 3.8? I had a look on the code. Do you have any thoughts on how to do it? I find it difficult to propose a solution without knowing which new concepts I'm allowed to add and modify.
(In reply to comment #14) > What (In reply to comment #13) > > If you know how/where to do this, a patch would be > > welcome otherwise I'll try to do it for 3.8 - but no promise. > > What's the deadline for this to make it into 3.8? M6 (March 16) is the deadline for new APIs, but we code freeze a week before and I'm away that week, hence I should have a patch around February 24 to allow polish. Also, the patch needs to be small (< 250 LOC), otherwise we have to postpone it to the next release as the IP deadline was last Friday. I think this should be doable. > I had a look on the code. Do you have any thoughts on how to do it? I find it > difficult to propose a solution without knowing which new concepts I'm allowed > to add and modify. There are two parts to it (if I didn't miss anything in your scenario): 1) Enable sorting. I think we can just add a ContentAssistant.setSorter(IProposalSorter) and then pass it to the popup which would invoke the sorter when typing (instead of just filtering). IProposalSorter needs the methods we have on org.eclipse.jdt.ui.text.java.AbstractProposalSorter. 2) Decide when to set the sorter. I would not want this to happen by default i.e. we need a way to enable it. It looks like a certain processor is forcing this. I guess if we add an API to get the current sorter from JDT, then the processor could handle this.
Is there a recent jface.text git repository? I tried org.eclipse.jface.text.git but this seems to be the same as in CVS which is in version 3.7.100. I'll create a patch against this repository later but would like to discuss my current approach (although a patch would make it easier) > There are two parts to it (if I didn't miss anything in your scenario): > 1) Enable sorting. > I think we can just add a ContentAssistant.setSorter(IProposalSorter) and > then pass it to the popup which would invoke the sorter when typing (instead > of just filtering). IProposalSorter needs the methods we have on > org.eclipse.jdt.ui.text.java.AbstractProposalSorter. In CompletionProposalPopup I added: private ICompletionProposalSorter fSorter; private void sortProposals(final ICompletionProposal[] proposals) { if (fSorter != null) { // fSorter.beginSort... Arrays.sort(proposals, fSorter); // fSorter.endSort... } } public void setSorter(ICompletionProposalSorter sorter) { fSorter= sorter; } + a call to sortProposals in setProposals() In ContentAssist I added: public void setSorter(final ICompletionProposalSorter sorter) { if(fProposalPopup!=null) fProposalPopup.setSorter(sorter); } it basically delegates the sorter to the popup. Added ICompletionProposalSorter: // generics requires BREE to be set to 1.5 <- is that ok? public interface ICompletionProposalSorter extends Comparator<ICompletionProposal> { // omitted begin/end methods due to keep changes in JDT's existing proposal sorters low public int compare(ICompletionProposal p1, ICompletionProposal p2); } > > 2) Decide when to set the sorter. > I would not want this to happen by default i.e. we need a way to enable it. > It looks like a certain processor is forcing this. I guess if we add an API > to get the current sorter from JDT, then the processor could handle this. In jdt.ui: public abstract class AbstractProposalSorter implements ICompletionProposalSorter {} ^- added implements ICompletionProposalSorter In org.eclipse.jdt.internal.ui.text.java.ContentAssistProcessor: public final ICompletionProposal[] computeCompletionProposals(final ITextViewer viewer, final int offset) { try { // getSorter() became public: final AbstractProposalSorter sorter= ProposalSorterRegistry.getDefault().getCurrentSorter().getSorter(); fAssistant.setSorter(sorter); } catch (final Exception e) { // TODO } But I'm not sure that you meant this with " we need a way to enable it".
(In reply to comment #16) > Is there a recent jface.text git repository? There is no jface.text git repository. JFace Text is in http://git.eclipse.org/c/platform/eclipse.platform.text.git/ > I'll > create a patch against this repository later but would like to discuss my > current approach (although a patch would make it easier) > > > There are two parts to it (if I didn't miss anything in your scenario): > > 1) Enable sorting. > > I think we can just add a ContentAssistant.setSorter(IProposalSorter) and > > then pass it to the popup which would invoke the sorter when typing (instead > > of just filtering). IProposalSorter needs the methods we have on > > org.eclipse.jdt.ui.text.java.AbstractProposalSorter. > > In CompletionProposalPopup I added: Looks good. > > private ICompletionProposalSorter fSorter; > > private void sortProposals(final ICompletionProposal[] proposals) { > if (fSorter != null) { > // fSorter.beginSort... > Arrays.sort(proposals, fSorter); > // fSorter.endSort... > } > } > > public void setSorter(ICompletionProposalSorter sorter) { > fSorter= sorter; > } > > + a call to sortProposals in setProposals() > > In ContentAssist I added: > > public void setSorter(final ICompletionProposalSorter sorter) { > if(fProposalPopup!=null) > fProposalPopup.setSorter(sorter); > } This is not good as a call to this API would depend on whether the content assistant is in installed state or not. > Added ICompletionProposalSorter: > > // generics requires BREE to be set to 1.5 <- is that ok? No. > public interface ICompletionProposalSorter extends > Comparator<ICompletionProposal> { > > // omitted begin/end methods due to keep changes in JDT's existing proposal > sorters low But in that case your sort method above won't compile. > > 2) Decide when to set the sorter. > > I would not want this to happen by default i.e. we need a way to enable it. > > It looks like a certain processor is forcing this. I guess if we add an API > > to get the current sorter from JDT, then the processor could handle this. > > > In jdt.ui: > public abstract class AbstractProposalSorter implements > ICompletionProposalSorter {} > ^- added implements ICompletionProposalSorter > > > In org.eclipse.jdt.internal.ui.text.java.ContentAssistProcessor: > > public final ICompletionProposal[] computeCompletionProposals(final ITextViewer > viewer, final int offset) { > try { > // getSorter() became public: > final AbstractProposalSorter sorter= > ProposalSorterRegistry.getDefault().getCurrentSorter().getSorter(); > fAssistant.setSorter(sorter); > } catch (final Exception e) { > // TODO > } > > But I'm not sure that you meant this with " we need a way to enable it". I meant, that I don't want to always set the sorter as you just do in your code snippet above. This causes the SDK to sort while typing and this is not good. I see two ways to achieve our goal: - We add an additional attribute in the extension point (e.g. "needSortingAfterFiltering") which triggers the code that actually sets the sorter. - We add IJavaCompletionProposalComputerExtension which has one method: boolean needsSortingAfterFiltering(); The naming might need some tweaking ;-).
(In reply to comment #17) > > In ContentAssist I added: > > > > public void setSorter(final ICompletionProposalSorter sorter) { > > if(fProposalPopup!=null) > > fProposalPopup.setSorter(sorter); > > } > > This is not good as a call to this API would depend on whether the content > assistant is in installed state or not. I'm not sure I got this right. I understood: "store as field && set the sorter here BUT also in install() if field is not null at execution time". > > > 2) Decide when to set the sorter. > > > I would not want this to happen by default i.e. we need a way to enable it. > > > It looks like a certain processor is forcing this. I guess if we add an API > > > to get the current sorter from JDT, then the processor could handle this. > > [...] > > In org.eclipse.jdt.internal.ui.text.java.ContentAssistProcessor: > > > > public final ICompletionProposal[] computeCompletionProposals(final ITextViewer > > viewer, final int offset) { > > try { > > // getSorter() became public: > > final AbstractProposalSorter sorter= > > ProposalSorterRegistry.getDefault().getCurrentSorter().getSorter(); > > fAssistant.setSorter(sorter); > > } catch (final Exception e) { > > // TODO > > } > > > > But I'm not sure that you meant this with " we need a way to enable it". > > I meant, that I don't want to always set the sorter as you just do in your code > snippet above. This causes the SDK to sort while typing and this is not good. I > see two ways to achieve our goal: > - We add an additional attribute in the extension point (e.g. > "needSortingAfterFiltering") which triggers the code that actually sets the > sorter. > - We add IJavaCompletionProposalComputerExtension which has one method: > boolean needsSortingAfterFiltering(); ContentAssistProcessor.collectProposals(ITextViewer, int, IProgressMonitor, ContentAssistInvocationContext) then might change to: [...] boolean requiresSortingOnPrefixChange= false; for (final CompletionProposalCategory cat : providers) { List<ICompletionProposal> computed= cat.computeCompletionProposals(...); proposals.addAll(computed); requiresSortingOnPrefixChange|= cat.requiresSortingOnPrefixChanges(); [...] } if (requiresSortingOnPrefixChange) { installInteractiveProposalSorter(); } with that approach, category has the responsibility to collect the information if an engine needs resorting of its proposals, i.e., CompletionProposalCategory might be changed as follows: public void sessionStarted() { fRequiresSortingOnPrefixChange= false; List<CompletionProposalComputerDescriptor> descriptors= ...; for (CompletionProposalComputerDescriptor desc: descriptors) { if (desc.getCategory() == this) { desc.sessionStarted(); fRequiresSortingOnPrefixChange|= desc.requiresSortingOnPrefixChange(); } [...] } public boolean requiresSortingOnPrefixChanges() { return fRequiresSortingOnPrefixChange; } and the CompletionProposalComputerDescriptor might get the additional property requiresSortingOnPrefixChange() which is fetched from the extension point (or proposal computer) engine). Is this the intended solution? Related question: Would you like might enable sorting only iff the requesting computer actually contributes to the category?
(In reply to comment #18) > (In reply to comment #17) > > > In ContentAssist I added: > > > > > > public void setSorter(final ICompletionProposalSorter sorter) { > > > if(fProposalPopup!=null) > > > fProposalPopup.setSorter(sorter); > > > } > > > > This is not good as a call to this API would depend on whether the content > > assistant is in installed state or not. > > I'm not sure I got this right. I understood: "store as field && set the sorter > here BUT also in install() if field is not null at execution time". +1. > > > > > 2) Decide when to set the sorter. > > > > I would not want this to happen by default i.e. we need a way to enable it. > > > > It looks like a certain processor is forcing this. I guess if we add an API > > > > to get the current sorter from JDT, then the processor could handle this. > > > > [...] > > > In org.eclipse.jdt.internal.ui.text.java.ContentAssistProcessor: > > > > > > public final ICompletionProposal[] computeCompletionProposals(final ITextViewer > > > viewer, final int offset) { > > > try { > > > // getSorter() became public: > > > final AbstractProposalSorter sorter= > > > ProposalSorterRegistry.getDefault().getCurrentSorter().getSorter(); > > > fAssistant.setSorter(sorter); > > > } catch (final Exception e) { > > > // TODO > > > } > > > > > > But I'm not sure that you meant this with " we need a way to enable it". > > > > I meant, that I don't want to always set the sorter as you just do in your code > > snippet above. This causes the SDK to sort while typing and this is not good. I > > see two ways to achieve our goal: > > - We add an additional attribute in the extension point (e.g. > > "needSortingAfterFiltering") which triggers the code that actually sets the > > sorter. > > - We add IJavaCompletionProposalComputerExtension which has one method: > > boolean needsSortingAfterFiltering(); > > > ContentAssistProcessor.collectProposals(ITextViewer, int, IProgressMonitor, > ContentAssistInvocationContext) then might change to: > > [...] > boolean requiresSortingOnPrefixChange= false; > for (final CompletionProposalCategory cat : providers) { > List<ICompletionProposal> computed= cat.computeCompletionProposals(...); > proposals.addAll(computed); > requiresSortingOnPrefixChange|= cat.requiresSortingOnPrefixChanges(); > [...] > } > if (requiresSortingOnPrefixChange) { > installInteractiveProposalSorter(); > } > > > with that approach, category has the responsibility to collect the information > if an engine needs resorting of its proposals, i.e., CompletionProposalCategory > might be changed as follows: > > public void sessionStarted() { > fRequiresSortingOnPrefixChange= false; > List<CompletionProposalComputerDescriptor> descriptors= ...; > for (CompletionProposalComputerDescriptor desc: descriptors) { > if (desc.getCategory() == this) { > desc.sessionStarted(); > fRequiresSortingOnPrefixChange|= desc.requiresSortingOnPrefixChange(); > } > [...] > } > > public boolean requiresSortingOnPrefixChanges() { > return fRequiresSortingOnPrefixChange; > } > > > and the CompletionProposalComputerDescriptor might get the additional property > requiresSortingOnPrefixChange() which is fetched from the extension point (or > proposal computer) engine). > > > > Is this the intended solution? Yes, along these lines, but I would choose: either the info comes from an API or the extension point. > > Related question: > Would you like might enable sorting only iff the requesting computer actually > contributes to the category? Yes.
Created attachment 211946 [details] Patch for jface.text to enable reordering
Created attachment 211947 [details] patch for jdt.ui to enable reordering
These patches summarize the results of discussion above. The patch was successfully tested with our subwords completion. To run local tests, you may install subwords completion from this update site http://download.eclipse.org/recommenders/updates/head/e42/ (build is running atm) or may import the sources from the recommenders git repository.
I quickly peeked into the patch and saw the missing copyright, Javadoc, @since tags, etc. Applying this patch would cause API Tools errors in my workspace. Please correctly setup API Tools. For details, see http://dev.eclipse.org/mhonarc/lists/eclipse-dev/msg09310.html
(In reply to comment #23) > I quickly peeked into the patch and saw the missing copyright, Javadoc, @since > tags, etc. Applying this patch would cause API Tools errors in my workspace. > Please correctly setup API Tools. For details, see > http://dev.eclipse.org/mhonarc/lists/eclipse-dev/msg09310.html Few more questions. License: I added license header to the newly created Interface ICompletionProposalSorter. You don't want me to add a contributor line to the other files, right? Formatting: It seems that applying the project's formatter on it's source code leads to many single whitespace additions. You are going to reject a patch if it contains 98% whitespace changes, right? Since I assume that my local project settings are intact, can you check if your formatter has changed or would also lead to changes? Most changes are caused be added linebreaks, removal of single or double white-spaces etc. Javadocs: I've added javadoc comments to all added methods. Public ones and fields should have @since tags. I hope, this will integrate smoothly with your workspace.
Created attachment 211966 [details] patch for jdt.ui to enable reordering
Created attachment 211967 [details] Patch for jface.text to enable reordering
Created attachment 211968 [details] patch for jdt.ui to enable reordering Sorry. lost in whitespace hell I filtered away the important change..
(In reply to comment #24) > You don't want me to add a contributor line to the > other files, right? You should add the contributor line to all modified files. > Formatting: > It seems that applying the project's formatter on it's source code leads to > many single whitespace additions. You are going to reject a patch if it > contains 98% whitespace changes, right? Since I assume that my local project > settings are intact, can you check if your formatter has changed or would also > lead to changes? Most changes are caused be added linebreaks, removal of single > or double white-spaces etc. See http://wiki.eclipse.org/JDT_UI/How_to_Contribute#Configuring_the_workspace. Essentially we format only edited lines.
Created attachment 212115 [details] patch for jdt.ui to enable reordering Added missing license headers; removed unnecessary whitespaces
Created attachment 212116 [details] Patch for jface.text to enable reordering added missing license headers; removed unnecessary whitespaces
Created attachment 212117 [details] patch for jface.text to enable reordering added missing license headers; removed unnecessary whitespace changes
please let me know what you think about this patch. I'd greatly appreciate to have this patch in for Juno and to make subwords work properly.
Marcel, how exactly did you create the patches? They have some strange endings: -- 1.7.5.4 which the apply patch wizard doesn't like (see bug 373975). You don't have to resubmit the patches as I manually fixed the patch for now.
(In reply to comment #33) > Marcel, how exactly did you create the patches? git format-patch master --stdout > bug350991.patch git version 1.7.5.4
(In reply to comment #34) > (In reply to comment #33) > > Marcel, how exactly did you create the patches? > > git format-patch master --stdout > bug350991.patch > > > git version 1.7.5.4 Please use EGit so that we get compatible patches.
The patches are almost good but there are several smaller issues that need to be addressed first (see list below). If I have the new patches with those issues ironed out by Wednesday 8:00 CET we can put it into Juno. Otherwise we have to postpone it to Kepler. - ICompletionProposalSorter - has compile warnings because the Javadoc for the parameters is missing and because the Javadoc for the return type is missing. Did you not see the warnings in your workspace? That would be strange - copyright date is wrong. Must either be "2012" or "2011, 2012" - Javadoc doesn't start with a "<p>" - ContentAssistant - setSorter(...) should allow 'null' - registerSorterWithProposalPopup is overkill (we didn't add such helpers before, so we shouldn't start with this). - we also add @since tags to non-public members - Javadoc ends with a "." - CompletionProposalPopup - we also add @since tags to non-public members - only sort the proposals if really needed, i.e. directly before "fFilteredProposals= proposals;". This also makes sure you don't get an NPE (which shouldn't happen, but you never know) - the Javadoc for setProposals should mention the sorting - javaCompletionProposalComputer schema - I don't like the attribute name (it should have "sort" in it, like the new API, .e.g "needSortingAfterFiltering") - the description has a type (processessor) - add "@since 3.8" to the end of the description - general comments for the JDT patch - adjust the member names and comments to the attribute name change (see comment above) - we also add @since tags to non-public members - make sure the copyright date is updated everywhere - requiresReordering() sounds like an order. We normally use the is* pattern for boolean methods - I don't like the bit-wise OR (|=) - please use "||" - CompletionProposalCategory - fRequiresReordering is set on each session start but it is never reset. Either cache that info, or make sure it gets reset. - ContentAssistProcessor - the sorter installation needs more work: we must guarantee that in the next run the correct sorter is used (just assume it got changed by the user). Most likely, we can simply set the sorter to 'null' - exceptions must be correctly caught and logged (see other clients of getSorter()) - initial sorting (if required) is now done twice (once in the processor and once in the popup). We should try to avoid this. We can also look at this item during M7. - begin/endSorting is not working as expected with this approach. I can live with that but it needs to be documented in the Javadoc of the affected APIs.
Created attachment 212506 [details] patch for jdt.ui to enable proposal resorting after filtering
Created attachment 212507 [details] patch for jface.text to enable proposal resorting after filtering
(In reply to comment #36) > - ContentAssistant > - setSorter(...) should allow 'null' Not sure I understood this. Since the code supports null (AFAICS) I guess you wanted the comment to explicitly state this information. I might be wrong. > - ContentAssistProcessor > - the sorter installation needs more work: we must guarantee that in the > next run the correct sorter is used (just assume it got changed by the > user). Most likely, we can simply set the sorter to 'null' In CompletionProposalPopup#hide I added "fContentAssistant.setSorter(null);" > - exceptions must be correctly caught and logged (see other clients of > getSorter()) I increased the visibility of the existing methods in ProposalSorterHandle to package visibility. > - initial sorting (if required) is now done twice (once in the processor and > once in the popup). We should try to avoid this. We can also look at this > item during M7. I assumed that the flag isFilteredSubset in CompletionProposalPopup#setProposals(ICompletionProposal[], boolean) would be the right place to do this - but that*s just wrong. Can you remove the if statement the guards the filtering? Any idea where this should be addressed?
(In reply to comment #39) > (In reply to comment #36) > > > - ContentAssistant > > - setSorter(...) should allow 'null' > > Not sure I understood this. Since the code supports null (AFAICS) I guess you > wanted the comment to explicitly state this information. I might be wrong. The Eclipse API rules explicitly forbid 'null' unless specified on the param or return tag. Your latest patch does not specify this on the 'sorter' param in ContentAssistant, hence passing 'null' is not allowed. > > - initial sorting (if required) is now done twice (once in the processor and > > once in the popup). We should try to avoid this. We can also look at this > > item during M7. > > I assumed that the flag isFilteredSubset in > CompletionProposalPopup#setProposals(ICompletionProposal[], boolean) would be > the right place to do this - but that*s just wrong. Can you remove the if > statement the guards the filtering? > > Any idea where this should be addressed? The processor is the one who sorts and also sets the sorter, hence we should fix it there. We could then even specify, that begin/end will *never* be called if one of the proposal computer requires resorting.
Created attachment 212525 [details] patch for jface.text to enable proposal resorting after filtering updated method comment; removed if(isFilteredSubset) check in setProposals.
(In reply to comment #41) > Created attachment 212525 [details] [diff] > patch for jface.text to enable proposal resorting after filtering > > updated method comment; removed if(isFilteredSubset) check in setProposals. Ooops! That patch is worse than the last one I reviewed ;-). You can't destroy the sorter information which is stored in the content assistant. Also, I don't see why it touches 'computeProposals(int offset)'. Small side note regarding the Javadoc for ICompletionProposalSorter.compare(...): please take a look at AbstractProposalSorter.compare(...) on how it can be improved (e.g. no '.' at the end of param descriptions, don't repeat return info twice). Javadoc in AbstractProposalSorter is malformed. The following from comment 36 is still missing: - CompletionProposalPopup - the Javadoc for setProposals should mention the sorting - ContentAssistProcessor - the sorter installation needs more work: we must guarantee that in the next run the correct sorter is used (just assume it got changed by the user). Most likely, we can simply set the sorter to 'null'
(In reply to comment #42) > (In reply to comment #41) > > Created attachment 212525 [details] [diff] > > patch for jface.text to enable proposal resorting after filtering > > > > updated method comment; removed if(isFilteredSubset) check in setProposals. > > Ooops! That patch is worse than the last one I reviewed ;-). You can't destroy > the sorter information which is stored in the content assistant. > Also, I don't > see why it touches 'computeProposals(int offset)'. accident. sorry. > Small side note regarding the Javadoc for > ICompletionProposalSorter.compare(...): please take a look at > AbstractProposalSorter.compare(...) on how it can be improved (e.g. no '.' at > the end of param descriptions, don't repeat return info twice). I copied the text now. I take away "no dots at the end param descriptions" and "no redundant information on method documentation on return parameter". > Javadoc in AbstractProposalSorter is malformed. added missing ''<" twice. > The following from comment 36 is still missing: > - CompletionProposalPopup > - the Javadoc for setProposals should mention the sorting I'm a victim of the whitespace and autosave actions... ;-) I discarded the change I made before to address this comment. Some shortcuts are just built-in such as Cmd+Shift+F... Sorry. > - ContentAssistProcessor > - the sorter installation needs more work: we must guarantee that in the > next run the correct sorter is used (just assume it got changed by the > user). Most likely, we can simply set the sorter to 'null' What's the appropriate location for this? ContentAssistant#fireSessionEndEvent() ?
(In reply to comment #43) > > - ContentAssistProcessor > > - the sorter installation needs more work: we must guarantee that in the > > next run the correct sorter is used (just assume it got changed by the > > user). Most likely, we can simply set the sorter to 'null' > > What's the appropriate location for this? > ContentAssistant#fireSessionEndEvent() ? sessionEndEvent() seems to be too late. If a user cycles though the content assist lists she would have the sorter enabled for all lists (once activated) until the session ends. To the best of my knowledge, there is no suitable method in ContentAssistant that could be used to set sorter to 'null' based without adding additional fields for bookkeeping which completion engine was last active etc. Thus, I decided to let the each ContentAssistProcessor decide whether it configures the ContentAssistant with either a sorter or 'null' every time it gets called. I've no clue if this is the solution you had in mind. If not, too bad for me ;) Anyways, thanks for your patience. I'll now prepare the last patch before the deadline.
Created attachment 212588 [details] patch for jdt.ui to enable proposal resorting after filtering
Created attachment 212589 [details] patch for jface.text to enable proposal resorting after filtering
I'm not yet too happy on how the sorter is reset but otherwise the patches are OK. I've pushed them to master: http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=b243be1cdfecff6dc8850fd2bffd2d955808ac18 http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=298a230997ac9dac0777fd04bd109e741be69962 Will tweak them a bit now.
Polished version: http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=34c656445e49e127c5c4a4e8153b782a5731d1ce http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=b194caeeec9e921dd0da73661bc8cc46cc1d629c
Marcel, please verify that the polished version works for you.
(In reply to comment #49) > Marcel, please verify that the polished version works for you. Completion engine works as expected. During my tests I unfortunately observed a misbehavior and I'm not sure whether/how this could be fixed. When triggering completion on an existing prefix such as "set|b" and going back and forth leads to bad proposal sorting since proposal are not informed that prefix was shortened - and thus cannot update their relevance. Example: trigger completion on "java.awt.List aList.set|b" and move cursor one position to the right: "set|b" -> "setb|" : proposal.isPrefix gets called, proposals are filtered and sorted. Move cursor one to the left: "setb|" -> "set|b" : when going back, nothing gets called. Content assistant just filters based on previous observation and adds the proposals below the existing proposal. Is calling "isPrefix" on each proposal even when going back in the editor an option? Maybe an extension interface that adds a "setPrefix()" method would suffice too.
This bug here is correctly fixed. If your sorter depends on the prefix of the proposals being updated/called then that's another issue. Your sorter could simply trigger this and make sure that the correct prefix/relevance is re-computed on your proposals. If not, please file a separate bug report to discuss this. Note to self: the code that prevents re-validation is at the top of CompletionProposalPopup.computeFilteredProposals(int, DocumentEvent)
