Community
Participate
Working Groups
Relevance of Quick fixes and Quick assists is hardcoded as integer literals, which makes it almost impossible to compare relevance of Quick fixes/assists. We should replace integer literals with int constants so that a reference search on the constants can be done to make it easier to compare relevance.
Yes. Each quick fix should have its own constant. In a first step, we should just extract the existing literals into constants (in one interface, with constants ordered by value). Later (post 3.7), we should try to categorize the quick fixes by relevance (e.g. SOLVES_IMMEDIATELY, SOLVES_OFTEN, SOLVES_MAYBE) and use an approach similar to the constants in IProblem (base relevance + fine tuning). Unfortunately, IJavaCompletionProposal#getRelevance() talks about a range of [0, 100], but this is not enforced, and I know we don't always adhere to this. Since there's not enough room in that range and there are no hints about how quick fix processors should choose the relevance, I think we should increase that range, make a few base relevances API, and advise contributors about the slots they should use. This will inevitably break more orderings than the small tweaks we currently apply, but we better do this once than stay with the unsatisfactory current state forever. This should be done in an early milestone.
(In reply to comment #1) > Unfortunately, IJavaCompletionProposal#getRelevance() talks about a range of > [0, 100], but this is not enforced, and I know we don't always adhere to this. > > Since there's not enough room in that range and there are no hints about how > quick fix processors should choose the relevance, I think we should increase > that range, make a few base relevances API, and advise contributors about the > slots they should use. I think currently we use [-10, 10] for quick fixes/assists. So [0,100] should be enough..
> I think currently we use [-10, 10] for quick fixes/assists. So [0,100] should > be enough.. It would be enough right now, but we should reserve some slack so that we can add new fixes (or tweak old ones) at any position in the future, without having to touch all other relevances in the same category. We don't want to screw third-party contributors again if they tweaked their relevances to work well with our quick fixes.
It can be hard to keep the following 2 lists upto date. http://help.eclipse.org/indigo/index.jsp?topic=%2Forg.eclipse.jdt.doc.user%2Freference%2Fref-java-editor-quickassist.htm http://help.eclipse.org/indigo/index.jsp?topic=%2Forg.eclipse.jdt.doc.user%2Freference%2Fref-java-editor-quickfix.htm We can probably try to make this easier as well along with this bug.
I pushed the first draft in a topic branch http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/log/?h=dazad/bug-339223-replace-integer-literals-with-constants - A large percentage of integer literals are extracted, but not all. - The relevance values have not been tweaked to the 0-1000 scale (even though I updated the Javadoc of IJavaCompletionProposal#getRelevance()) Markus, let me know if something does not go in the right direction. (Or you can also just fix stuff in the branch)
There are a few places where we start with a base relevance and then tweak it based on certain conditions e.g. UnresolvedElementsSubProcessor.addSimilarVariableProposals(...). At this point I am not entirely sure on how to deal with these. I guess we can just extract the base relevance... http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=959eba75c2e83b543874d4394c7b5bf1c10b9ec5 - Almost all other int literals should be extracted in this commit.
Markus, a quick review at this point would be helpful. We already agreed on 0-1000 range (or should it be 0-999 ?). Next step would be to tweak the current relevances to this range and try to categorize quick fixes /assists by relevance.
Looks good, except that ReturnTypeSubProcessor#METHOD_RETURNS_VOID didn't make it into the interface. I fixed this, rebased on top of master, and squashed into a single commit. The I realized that I'm not allowed to hijack your topic branch and push a non-FF commit. So I pushed it into a new branch: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=11c47e2761389d7d816fd160186ef647fc35fae7 I guess the best process for now is to do ping-pongs where each of us absorbs the other's topic branch and then pushes non-FF changes to his own branch. A bunch of proposals still use hard-coded relevances. Please open a type hierarchy on IJavaCompletionProposal#getRelevance(), show members in hierarchy, and find the constants there (e.g. LinkedNamesAssistProposal). Next step is to come up with categories. The categories should not be based on features, but be based on likelihood to fix a given problem (for quick fixes) or on likelihood to be the "best applicable" (for quick assists). Comment 2 proposed "SOLVES_IMMEDIATELY, SOLVES_OFTEN, SOLVES_MAYBE", but we should look for even more speaking names. Relevances are always in relation to other relevances, so the highest relevance should be for quick fixes that are "the right thing to do" in 99% of the cases (e.g. because they just implement the only sensible solution or the one that is already given in the error message). IJavaCompletionProposal#getRelevance() also needs useful Javadoc that tells explicitly whether higher or lower numbers are more relevant. We should also relax the contract a bit and just say the proposals *should* be in that range -- without telling anything about what happens if they don't. Then we're free to use other relevances, but contributors are not.
(In reply to comment #8) > So I pushed it into a new branch: > http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=11c47e2761389d7d816fd160186ef647fc35fae7 Sorry I messed up the push. Fixed with a non-FF. Take http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?h=mkeller/bug-339223-replace-integer-literals-with-constants
(In reply to comment #8) > I guess the best process for now is to do ping-pongs where each of us absorbs > the other's topic branch and then pushes non-FF changes to his own branch. I would rather have keep adding commits to one topic branch and do the squashing in the end when we are ready to merge the changes to master. This way we have the history of commits as the feature was developed (and it is simple for each committer to understand what others did), and we can avoid the 2 branch ping pong. Also we can probably leave the work of squashing commits and cleaning up the history to the 'owner' of the topic branch. > A bunch of proposals still use hard-coded relevances. Please open a type > hierarchy on IJavaCompletionProposal#getRelevance(), show members in hierarchy, > and find the constants there (e.g. LinkedNamesAssistProposal). Yeah, there were a few more that I found later and fixed in my workspace :-) > Next step is to come up with categories. Currently, we use all values from from 0-10 (and a few outside this range). I started off by creating a category for each value currently being used - but coming up with 10 categories is kind of hard (and was not making sense). So far I had something like the following (in the order of decreasing relevance) QF_SOLVES_IMMEDIATELY QF_SOLVES_OFTEN QF_SOLVES_MAYBE QA_MOST_APPLICABLE QA_OFTEN_APPLICABLE QA_MAYBE_APPLICABLE Essentially I tried to give QFs higher relevance than QA. But then - some QAs are also QFs - current relevances do not map directly to this sort of categorization - for a few QAs we fiddle with relevance based on whether there are errors at the invocation location or not. => Essentially, we will break/change the ordering at a few places. I guess we are OK with this, and also these changes might actually be improvements in some cased. (Let me know if you have better ideas) I will try to release the stuff I have so far today to 'my topic branch' :-) > IJavaCompletionProposal#getRelevance() also needs useful Javadoc Agree.
We should not make it more complicated than it is right now. Creation of maintenance / main branches doesn't happen that often, and there's always someone who has shell access and can do it. It should never be the webmaster who creates a branch on a project.
(In reply to comment #11) > We should not make it more complicated than it is right now. Creation of > maintenance / main branches doesn't happen that often, and there's always > someone who has shell access and can do it. It should never be the webmaster > who creates a branch on a project. Sorry, wrong bug.
(In reply to comment #10) Sounds good. The current ordering is not set in stone. The very reason for this bug to exist is that we lost control over the relevances. Some reorderings are to be expected. The documentation of the relevances needs to find the right level between being precise and being open (i.e. acknowledging that this is a dynamic system). One goal is to define some base relevances that are quite far apart, and then we declare common spreads around these bases (e.g. proposals from jdt.ui are usually in the range [base, base+50] -- but this is not guaranteed). The adjustments for quick assists that also serve as quick fixes were an ad-hoc attempt to fix pressing problems. The clean solution is to remove e.g. the 'relevanceDrop' variable in QuickAssistProcessor#getExtractVariableProposal(..) and use separate constants for the separate usage scenarios.
I extracted some more constants, I think I got them all now. I also rebased the branch on master, squashed commits and pushed to remote branch - http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/log/?h=dazad/bug-339223-replace-integer-literals-with-constants. I did not touch JUnitQuickFixProcessor yet, because that is probably best done while creating categories and APIs for relevances. I will take another look to check if I missed anything, after that I will merge the changes with master. Markus, if you want to make a change just commit changes to this branch as we had discussed earlier. You can also probably delete the branch you had created to keep the clutter less. (Also, in general do we delete topic branches once we are done with them? Maybe a topic for discussion for our weekly call...)
(In reply to comment #14) Looks good, except for the _1, _2, _3 suffixes. > I did not touch JUnitQuickFixProcessor yet Yes, let's finish the jdt.ui plug-in first. > (Also, in general do we delete topic branches once we are done with them?) Yes, I deleted mine.
(In reply to comment #15) > Looks good, except for the _1, _2, _3 suffixes. :-) Finding good names is tricky in a few cases. I made another pass and changes the names for several of them. A few still remain, but I think we can live with them... for now. There are still a few locations where we tweak the relevance where creating constants for each situation is tricky/not possible. E.g. the tweak in FixCorrectionProposal.getRelevance applies to a number of fixes at once. The tweaks in UnresolvedElementsSubProcessor for example are there to adjust the relevance of same kinds of proposals with respect to each other, as opposed to adjusting relevance in different situations. FixCorrectionProposal.getRelevance SuppressWarningsSubProcessor.addSuppressWarningsProposalIfPossible TypeMismatchSubProcessor.addChangeSenderTypeProposals UnresolvedElementsSubProcessor.getVariableProposals UnresolvedElementsSubProcessor.addSimilarVariableProposals UnresolvedElementsSubProcessor.addSimilarTypeProposals UnresolvedElementsSubProcessor.createTypeRefChangeProposal UnresolvedElementsSubProcessor.createTypeRefChangeFullProposal We can probably live with them for now, but maybe the tweaks will have to be changed and/or some code refactored once we create categories and expand the relevances to 0-1000 range. (I will make another pass over these to make sure I did not miss anything) Apart from the methods mentioned above (and few methods in their Call Hierarchy), there should not be any other place where relevance used is not a constant defined in IProposalRelevance.
(In reply to comment #16) > SuppressWarningsSubProcessor.addSuppressWarningsProposalIfPossible > TypeMismatchSubProcessor.addChangeSenderTypeProposals Tweaks of the form 'relevance + 1' or 'relevance - 1' i.e. adjust the relevance of quick fix with respect to another similar once in the same situation. I think we should allow tweaks of this form. - UnresolvedElementsSubProcessor.getTypeProposals - UnresolvedElementsSubProcessor.getVariableProposals These two are special situations where extracting all constants is not really feasible. (These 2 methods call other methods which actually tweak the relevances) I think it is OK to leave these as is. Branch updated with latest changes - http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/log/?h=dazad/bug-339223-replace-integer-literals-with-constants
Changes released in master - http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=5cd545dc61dfb0176b858db1cfb5cfa27d8fdbdc
http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=33beae078ce6b5c83564ef61865bb9019fefb901 Dani pointed out that I did not push IProposalRelevance.java. Not sure why this happened. Usually the commit dialog also shows newly created files, but it looks like this time it did not. I had to first 'add' the new file before committing it. Will take a look as to why this happened...
Deepak, any plans to finish this for M2?
(In reply to comment #20) > Deepak, any plans to finish this for M2? Probably not. I *will* get to it, once I get settled in the new city.
(In reply to comment #21) > (In reply to comment #20) > > Deepak, any plans to finish this for M2? > > Probably not. I *will* get to it, once I get settled in the new city. Ping!
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie.
