Community
Participate
Working Groups
Created attachment 253129 [details] Screenshot Eclipse 4.5 M7 - I20150430-1445 - Create a Java 8 project, open java.util.Map and got to the method #get. - Place caret before the return type "V" and right-click to get the context menu. - "Annotate" command is enabled and selecting that does not produce any result. It should be disabled like the "Copy" command shown in screenshot when it is not applicable.
New Gerrit change created: https://git.eclipse.org/r/47094
Thanks for the reference to the "Copy" command, which helped me identify the code location where the fix should go :)
Mh, two consecutive gerrit/hudson runs (using the exact same commit) produced two disjoint sets of regressions. (#243): 9 failures in LeakTestSuite (#244): 3 failures in CodeCompletionTest I doubt that either set of failures could have been caused by my change.
This is getting weird: during 4 build runs, hudson strictly alternated between failing in LeakTestSuite and failing in CodeCompletionTest. The failure in CodeCompletionTest I cannot reproduce at all. Running LeakTestSuite locally I did observe one failure: testSearchResultEditorClose(). This I got twice actually. But never the 9 failures seen on hudson. Noopur, do you have a suggestion how to take this simple fix forward?
Fifth build against the same change succeeded ... go figure ...
Sorry for the random Gerrit failures. This is bug 423416, which we haven't been able to track down. Regarding this bug: The "Annotate" command should not show up in the context menu of editors in which it's not available (i.e. it should not just be disabled). There are two reasons why "Copy" is different and is sometimes shown disabled: - it's a very special command that everybody expects to be there all the time - its availability depends on whether there's a selection or not
(In reply to Markus Keller from comment #6) > Regarding this bug: The "Annotate" command should not show up in the context > menu of editors in which it's not available (i.e. it should not just be > disabled). Sure, patch set #3 implements the following 2-stage strategy: Create and show action only when relevant per this editor: - do not create action during createActions() - create action in verifyInput() if - we have source, and - null annotations are enabled account for editor re-use Disable action during editorContextMenuAboutToShow() if selection is not a type. This is determined based on the selected IJavaElement (we don't want to compute AST during this method). Unfortunately, wildcards and arrays don't have an IJavaElement, so we must keep the action enabled, when no IJavaElement is selected (could perhaps improve in 4.6 by further heuristics).
Markus, do you agree to the updated implementation (see comment 7) plus inclusion in RC1?
+1 to fix in RC1. -1 for patch set 3. The enabling/disabling of the action in editorContextMenuAboutToShow(..) can't work: - open the menu with a selection for which the action gets disabled - select a method return type => action still disabled and Ctrl+1 doesn't work; need to open menu to re-enable Furthermore, editor actions can't call codeSelect(..). Multiple such calls would be too expensive. If necessary, the action would have to become a SelectionDispatchAction and make use of the JavaTextSelection etc. But that's too big a change for RC1. Let's just hide the action from the menu when it's disabled, see patch set 4 (based on your patch set 2). OK to release if you agree.
Thanks for the review. (In reply to Markus Keller from comment #9) > +1 to fix in RC1. -1 for patch set 3. > > The enabling/disabling of the action in editorContextMenuAboutToShow(..) > can't work: > - open the menu with a selection for which the action gets disabled > - select a method return type > => action still disabled and Ctrl+1 doesn't work; need to open menu to > re-enable I see, I missed the interaction between menu invocation and Ctrl-1. > Furthermore, editor actions can't call codeSelect(..). Multiple such calls > would be too expensive. If necessary, the action would have to become a > SelectionDispatchAction and make use of the JavaTextSelection etc. But > that's too big a change for RC1. I saw JavaTextSelection but wasn't 100% sure about its purpose. Are you saying this mechanism is used for sharing the result of codeSelect among different actions? Sounds cool. > Let's just hide the action from the menu when it's disabled, see patch set 4 > (based on your patch set 2). OK to release if you agree. Sure, thanks for preparing the change. Clean and simple.
Gerrit change https://git.eclipse.org/r/47094 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=ae85ec9b8a760363ac5178079d0701b72e5b2007
.
Presence of Ctrl+1 to annotate and right-click > Annotate are not always in sync now. For example, create a Java 8 project and enable annotation-based null analysis. Open Map#get => Ctrl+1 not present, but Annotate is present in context menu. Now, add the location for external annotations in project's JRE - both ways to annotate are present. Also, noticed that if Map#get is already open and then annotation-based null analysis is enabled and location for external annotations is specified on project, then both ways to Annotate don't work. We have to close and reopen the Map editor to make them work. Another issue related to bug 466299 is that if I have 2 projects in workspace and I enable annotation-based null analysis on only one project and provide location for external annotations, I don't get the Annotate command in any way at Map#get. And observed that for a project, if two different locations are provided at the JRE and at rt.jar, then the one at JRE is used for .eea file when a type in Map#get is annotated. Only the first issue is related to this bug.
(In reply to Noopur Gupta from comment #13) > Presence of Ctrl+1 to annotate and right-click > Annotate are not always in > sync now. For example, create a Java 8 project and enable annotation-based > null analysis. Open Map#get => Ctrl+1 not present, but Annotate is present > in context menu. Now, add the location for external annotations in project's > JRE - both ways to annotate are present. What do you mean by "Ctrl+1 not present"? In this situation (null annotations enabled, but no external annotation location) the coarse grain checking signals "enable", which results in the menu action being shown. When attempting to perform the action (menu or key) we detect that we cannot create the annotation file (due to missing location) and the action silently aborts. Does this correspond to your observations? I'll respond to the other issues in bug 466299.
(In reply to Noopur Gupta from comment #13) > Also, noticed that if Map#get is already open and then annotation-based null > analysis is enabled and location for external annotations is specified on > project, then both ways to Annotate don't work. We have to close and reopen > the Map editor to make them work. Right, enablement is currently determined when setting the editor input. Should we re-check enablement during editorContextMenuAboutToShow()? This is a cheap operation. As for the key binding I'm not sure what would be the best hooks for re-checking? Perhaps by overriding isEnabled() right in the action? Of course a dynamic impl. of isEnabled() would address both issues. Would it then be safe to call setEnabled() from within isEnabled() (would that be needed)? I'm slightly afraid of triggering too much business going on inside firePropertyChange()... I don't see this issue as a must-fix for 4.5, though. Do you? > And observed that for a project, if two different locations are provided at > the JRE and at rt.jar, then the one at JRE is used for .eea file when a type > in Map#get is annotated. Topic for documentation: specify priority of these entries.
(In reply to Stephan Herrmann from comment #14) > Does this correspond to your observations? Yes, looks correct. (In reply to Stephan Herrmann from comment #15) > I don't see this issue as a must-fix for 4.5, though. Do you? Not an important one to be fixed right now.
I've filed bug 467123 to follow up on loose ends during the 4.6 cycle.
Verified as fixed with Eclipse 4.5 RC1 I20150514-1000.
