Bugzilla – Full Text Bug Listing
| Summary: | [templates] new 'finally' template does not appear when there's no catch block | ||
|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Dani Megert <daniel_megert> |
| Component: | Core | Assignee: | Jay Arthanareeswaran <jarthana> |
| Status: | VERIFIED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | daniel_megert, jarthana, noopur_gupta, robert.roth.off |
| Version: | 4.5 | ||
| Target Milestone: | 4.6 M7 | ||
| Hardware: | All | ||
| OS: | All | ||
| See Also: |
https://git.eclipse.org/r/71128 https://git.eclipse.org/r/71141 https://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=3f07cee03bf21eb29e60cb096a0f462d3c1d5779 https://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=a4a8119e97de6e0227fed420006f636c311530e2 |
||
| Whiteboard: | |||
| Bug Depends on: | 184222 | ||
| Bug Blocks: | |||
|
Description
Dani Megert
Robert, will you have time to fix this today or tomorrow? (In reply to Dani Megert from comment #1) > Robert, will you have time to fix this today or tomorrow? Yes, I will take a look today (tonight), and comment with the results, be it a gerrit request or an "I'm stuck with ..." type answer. (In reply to Dani Megert from comment #0) > I20150503-2000. > > The new 'finally' template introduced via bug 184222 does not work when > there's no catch block: > > void foo() { > try { > } f<Ctrl+Space> > } By the way, the same stands for the catch block too for me, the catch block template is not shown in the proposals, autocompletion inserts the catch keyword only: void foo() { try { } c<Ctrl+Space> } Based on this, I feel that this is not strictly related to the new templates, but rather to the part selecting the templates to show in the given context, but will take a more in-depth look. It probably depends on the compile error at the place of invocation. I do not get other templates too at this location and some other locations with compile error. Please check if it is related to bug 432539. Dani, I took a look, but I'm not that familiar with JDT, so I couldn't really find the root cause, therefore I won't be able to fix it today. I wanted to check if it is indeed related to bug 432539, but I couldn't find another Java construct which would require two blocks (try...catch/finally), to see if a similar thing happens in case of a compile error (no templates in autocompletion). If it is indeed related to that bug, I guess this can't be fixed in time for 4.5, as the other bug is targeted for 4.6. Noopur, please take a look. When there is no catch block, CompletionContext#getTokenLocation() returns TOKEN_KIND_UNKNOWN at TemplateCompletionProposalComputer.computeCompletionEngine(JavaContentAssistInvocationContext context): line 71. When there is a catch block, it returns TL_STATEMENT_START, which is correct. Hence, in absence of catch block, the context is set as "java" instead of "java-statements" (the context of the template). As a result, we don't get the template proposal. We should not change the context of the template to "java" as it will result in template appearing at non-relevant locations too. Moving to JDT/Core for comments on CompletionContext#getTokenLocation(). Quick look shows that we have this code in CompletionEngine:
if (astNodeParent == null && astNode instanceof CompletionOnSingleNameReference && !((CompletionOnSingleNameReference)astNode).isPrecededByModifiers) {
context.setTokenLocation(CompletionContext.TL_STATEMENT_START);
}
In this particular case (where we don't have the catch block, the completion node is of type CompleteOnKeyword and we don't set token location to TL_STATEMENT_START.
By altering the check to allow CompleteOnKeyword, I can see the finally template being offered. But things still don't work inside a lambda body, which could be lot more tricky.
At this point, I don't know if there are side effects. I am not an expert in this area, so will have to rely on tests.
(In reply to Jay Arthanareeswaran from comment #9) > At this point, I don't know if there are side effects. I am not an expert in > this area, so will have to rely on tests. This sounds like a candidate to move to 4.6 then. (In reply to Dani Megert from comment #10) > This sounds like a candidate to move to 4.6 then. Yes, if it can wait. We can consider in 4.5.1 too if you like. New Gerrit change created: https://git.eclipse.org/r/71128 (In reply to Eclipse Genie from comment #12) > New Gerrit change created: https://git.eclipse.org/r/71128 I consider this to be a safe fix that works inside lambda as well. The problem is I can't write a meaningful testcase for this in core. Noopur, will you be able to review/test the fix and also write a test case on the UI front? Thanks! New Gerrit change created: https://git.eclipse.org/r/71141 (In reply to Jay Arthanareeswaran from comment #13) > (In reply to Eclipse Genie from comment #12) > > New Gerrit change created: https://git.eclipse.org/r/71128 > > I consider this to be a safe fix that works inside lambda as well. The > problem is I can't write a meaningful testcase for this in core. > > Noopur, will you be able to review/test the fix and also write a test case > on the UI front? Thanks! I tested the fix and it works fine. We don't have tests in UI for templates other than SurroundWithTemplates. I managed to create a test case for this issue: (In reply to Eclipse Genie from comment #14) > New Gerrit change created: https://git.eclipse.org/r/71141 It can be committed after the jdt.core change. Gerrit change https://git.eclipse.org/r/71128 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=3f07cee03bf21eb29e60cb096a0f462d3c1d5779 Thanks Noopur. For the records, the fix also works for the issue Robert reported in comment #3. Gerrit change https://git.eclipse.org/r/71141 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=a4a8119e97de6e0227fed420006f636c311530e2 Verified for Neon M7 using I20160425-1300 build |