Bug 208062 - First context menu has mysterious Run As/Debug As/Profile As menu entries
Summary: First context menu has mysterious Run As/Debug As/Profile As menu entries
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: Debug (show other bugs)
Version: 3.3   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.4 RC3   Edit
Assignee: Curtis Windatt CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 209418 214932 (view as bug list)
Depends on: 234372
Blocks:
  Show dependency tree
 
Reported: 2007-10-30 13:59 EDT by Kevin McGuire CLA
Modified: 2008-05-29 12:18 EDT (History)
15 users (show)

See Also:
curtis.windatt.public: review+
Michael_Rennie: review+
darin.eclipse: review+


Attachments
3 context menus v01 (1.70 KB, patch)
2007-11-01 19:16 EDT, Paul Webster CLA
no flags Details | Diff
Fix (using <adapt> expression) (1.39 KB, patch)
2007-12-13 10:45 EST, Markus Keller CLA
no flags Details | Diff
patch (8.67 KB, patch)
2008-01-23 16:30 EST, Darin Wright CLA
no flags Details | Diff
patch (1020 bytes, patch)
2008-05-21 22:53 EDT, Darin Wright CLA
no flags Details | Diff
Platform available v01 (1.37 KB, patch)
2008-05-22 21:40 EDT, Paul Webster CLA
no flags Details | Diff
patch (3.58 KB, patch)
2008-05-26 16:51 EDT, Darin Wright CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin McGuire CLA 2007-10-30 13:59:05 EDT
In the Templates View popup menu, Run As/Debug As/Profile As menus appear.  I've been told this is unintentional.

I've verified the behaviour in a new workspace, built from today's build (I20071030-0100), and source from HEAD (which since we're in milestone test should be equivalent to the source of 20071030). I then patched over using:
source: https://bugs.eclipse.org/bugs/attachment.cgi?id=80754 (2007-10-19)
icons: https://bugs.eclipse.org/bugs/attachment.cgi?id=79118 (2007-09-25)

I get those added menu items consistently. I can insert templates, create new, etc. and the menus remain. But once I select one of Run As/Debug As/Profile As and reveal their submenu, they all go away.  I don't even have to select any one of the actual menu items, just one of the top level three to open its submenu. 

I'm not sure why others aren't seeing this, perhaps it requires a more recent build to test against as I've done.
Comment 1 Kevin McGuire CLA 2007-10-30 14:05:04 EDT
Note: restarting the workbench brings them back. It all somewhat feels plugin activation related (but which and how I can't speculate).
Comment 2 dakshinamurthy.karra CLA 2007-10-31 09:12:06 EDT
(In reply to comment #1)
> Note: restarting the workbench brings them back. It all somewhat feels plugin
> activation related (but which and how I can't speculate).
> 

I have setup the system with I20071030. I am able to reproduce the problem. The same happens with the outline view also. Trying to figure out what is causing this bug.

-- KD

Comment 3 dakshinamurthy.karra CLA 2007-11-01 04:32:55 EDT
I made some progress towards the solution.

The problem is occurring because the debug plugin is contributing the actions, but not loaded to evaluate them (for popupmenu contributions forcePluginActivation is set to false). If we set that to true, this disappears, but IMO, that is not the right solution.

I tried to see what changed by doing a CVS compare but could not find it. Since, this is a generic problem happening with other views also - I suggest we raise a bug against org.eclipse.debug and close it here.

What do you think?

-- KD

Comment 4 Dani Megert CLA 2007-11-01 06:21:31 EDT
>reveal their submenu, they all go away.
What is 'all'? No context menu at all? No Debug sub menus? No Debug menu?

>I am able to reproduce the problem. The
>same happens with the outline view also.
I tested the Outline view using I20071101-0010 and everything works as expected. The 'Debug As' and 'Run As' menus are there as expected and they work. There is no 'Profile As' menu item - this probably comes from a non-SDK plug-in.

You might have seen bug 207744 unless you've also built SWT from HEAD.


Of course the Templates view must not have those context menu entries at all as this view doesn't show elements that you can run. I did not check the code now but it seems that the template view entires adapt to IResource and hence the entries are there. Look at other views (e.g. Problems, Javadoc): they don't show the actions that are contributed on resources.
Comment 5 dakshinamurthy.karra CLA 2007-11-01 06:30:21 EDT
(In reply to comment #4)
> >reveal their submenu, they all go away.
> What is 'all'? No context menu at all? No Debug sub menus? No Debug menu?
> 
> >I am able to reproduce the problem. The
> >same happens with the outline view also.
> I tested the Outline view using I20071101-0010 and everything works as
> expected. The 'Debug As' and 'Run As' menus are there as expected and they
> work. There is no 'Profile As' menu item - this probably comes from a non-SDK
> plug-in.
> 
I use a right-click on a variable from outline view and I can see all three items. Similar to TemplateView if you hover on one of the items and later right-click these items disappear.

> You might have seen bug 207744 unless you've also built SWT from HEAD.
> 
> 
I am trying it out with I20071030-0010.

> Of course the Templates view must not have those context menu entries at all as
> this view doesn't show elements that you can run. I did not check the code now
> but it seems that the template view entires adapt to IResource and hence the
> entries are there. Look at other views (e.g. Problems, Javadoc): they don't
> show the actions that are contributed on resources.
> 

The debug plugin enables all of them for java.lang.Object with test enablement. The selection contains TemplatePersistanceData which is not IResource. See also the other message I posted.

Comment 6 Dani Megert CLA 2007-11-01 07:02:21 EDT
OK, I see it now. This happens only the very first time when a context menu is shown and there's a non-null selection. That's why I didn't see it in my workspace.

Also, it happens in every view and editor who provides a non-null selection. This bug got introduced during 3.4 M3 - most likely by the fix for bug 136397.
Comment 7 Paul Webster CLA 2007-11-01 07:49:40 EDT
Yes ... not loading debug until the submenu is shown also has the effect of not loading debug until you bring up a debug contributed menu.

I could add back the check in ObjectPluginAction that returns true if it's a dropdown.  Then simply popping up a context menu will load all objectContributions (and Debug) and the enablement will be evaluated.

PW
Comment 8 dakshinamurthy.karra CLA 2007-11-01 07:57:22 EDT
(In reply to comment #7)
> I could add back the check in ObjectPluginAction that returns true if it's a
> dropdown.  Then simply popping up a context menu will load all
> objectContributions (and Debug) and the enablement will be evaluated.
> 
Looks like the option forcePluginActivation is given just for this purpose. Just setting it to true solves the problem for both TemplateView and OutlineView.

-- KD

Comment 9 Dani Megert CLA 2007-11-01 08:02:36 EDT
>Looks like the option forcePluginActivation is given just for this purpose.
>Just setting it to true solves the problem for both TemplateView and
>OutlineView.
We don't want plug-ins to be loaded just because the user opens a menu. That's bad as this could load many plug-ins.

Maybe Debug could register more fine grained (e.g. to IResource) and a new rule/attribte could be added where generic contributions like such to java.lang.Object that need the plug-in to be loaded are not shown until the plug-in is there. But of course this would also be surprise to the user.
Comment 10 Paul Webster CLA 2007-11-01 08:16:00 EDT
(In reply to comment #9)
> We don't want plug-ins to be loaded just because the user opens a menu. That's
> bad as this could load many plug-ins.

Yes, we were just discussing this and we should look at how debug can take advantage of the framework.

> Maybe Debug could register more fine grained (e.g. to IResource) and a new
> rule/attribte could be added where generic contributions like such to
> java.lang.Object that need the plug-in to be loaded are not shown until the
> plug-in is there. But of course this would also be surprise to the user.
> 

That's one option (registering against a couple of more-fine-grained types).

Another is to have their test element mark forcePluginActivation="true" which will load their tester for evaluation (and will only load debug instead of a blanket load for all objectContributions).

Property testers have the advantage that they can be separated from the UI plugin and so if it is important, loading a property tester doesn't have to activate a UI plugin (assuming they have somewhere to go).

PW
Comment 11 dakshinamurthy.karra CLA 2007-11-01 09:59:40 EDT
Is it necessary for each element to support IActionFilter (either through inheritance or as an adaptor) for the property tester to be used? In that case, may be checking whether the element can be adapted to IActionFilter in TestExpression#evaluate might suffice.

I am still trying to understand how this works. Ignore this if it sounds stupid ;-)

-- KD

Comment 12 Paul Webster CLA 2007-11-01 10:15:52 EDT
(In reply to comment #11)
> Is it necessary for each element to support IActionFilter (either through
> inheritance or as an adaptor) for the property tester to be used? In that case,
> may be checking whether the element can be adapted to IActionFilter in
> TestExpression#evaluate might suffice.

Actually, IActionFilter is the old expression syntax for objectState and is used in objectContribution/visibilty and objectContribution/action/enablement :-)

objectContribution/enablement uses core expressions and the test element/property testers ... there are no implicit conversions in core expressions (well, not really) AFAIK

PW
Comment 13 Paul Webster CLA 2007-11-01 11:12:21 EDT
Moving to Debug for consideration.

PW
Comment 14 Paul Webster CLA 2007-11-01 19:16:15 EDT
Created attachment 81903 [details]
3 context menus v01

This is an example that would allow these object contributions to load debug eagerly to evaluate their expressions (on context menu popup) without having to load all object contributions eagerly.

This particular problem doesn't effect objectContributions that use the old-style expression, but only the objectContribution/enablement element with a property tester expression.

PW
Comment 15 Markus Keller CLA 2007-11-07 06:20:13 EST
I also see these items in the context menu of the JRE System Library (in a Java project).
Comment 16 Martin Oberhuber CLA 2007-11-14 07:26:58 EST
I can reproduce this easily in 3.4M3:

Launch Eclipse
New > Project > General > Project "foo" , Finish
Right-click foo > Profile As...
Click somewhere else
Right-click foo

--> The "Profile As..." entry is gone.
Comment 17 dakshinamurthy.karra CLA 2007-11-14 10:49:29 EST
(In reply to comment #14)
> Created an attachment (id=81903) [details]
> 3 context menus v01
> 
> This is an example that would allow these object contributions to load debug
> eagerly to evaluate their expressions (on context menu popup) without having to
> load all object contributions eagerly.
> 
> This particular problem doesn't effect objectContributions that use the
> old-style expression, but only the objectContribution/enablement element with a
> property tester expression.
> 
> PW
> 

This does not look like right solution - even when an object has nothing to do with a plugin (in this case debug) - the plugin is loaded. Now assume that lot more plugins implement property testers and contribute context menus.

I think bringing back some form of ActionFilter into core looks like right solution.

-- KD

Comment 18 Markus Keller CLA 2007-11-14 11:25:42 EST
A middle way to use could be a more sophisticated enablement expression that uses non-contributed expressions to rule out many targets even when the debug plug-in has not been loaded.

E.g. the 'adapt' expression works without loading plug-ins. It would hide the contributions in the cases from comment 0 and comment 15, but it would still show the Profile menu on IResources initially (but this cannot be avoided without loading the class LaunchablePropertyTester and executing the test code).

<enablement>
    <adapt type="org.eclipse.debug.ui.actions.ILaunchable"/>
    <test property="org.eclipse.debug.ui.launchable" value="debug"/>
</enablement>
Comment 19 Paul Webster CLA 2007-11-15 08:14:08 EST
(In reply to comment #17)
> 
> This does not look like right solution - even when an object has nothing to do
> with a plugin (in this case debug) - the plugin is loaded. Now assume that lot
> more plugins implement property testers and contribute context menus.
> 
> I think bringing back some form of ActionFilter into core looks like right
> solution.

If that was the solution, then you would use the selection element instead of the enablement element and objectState instead of test.  But then you have the restrictions of the old expression language, like the objects you query must implement IActionFilter, etc.

This it the trade off of a lazy loading system ... programmatic enablement requires loading *someone's* plugin.  You can mitigate the problem by separating our your property tests as much as possible, but for accuracy you have to load something ... or live with the inaccuracy until some user action causes the loading.

PW
Comment 20 Paul Webster CLA 2007-11-15 08:25:39 EST
(In reply to comment #18)
> A middle way to use could be a more sophisticated enablement expression that
> uses non-contributed expressions to rule out many targets even when the debug
> plug-in has not been loaded.

I think this is a good general approach, use the declarative expressions as much as possible to help narrow down the cases that would require property testing/programmatic testing.

> 
> <enablement>
>     <adapt type="org.eclipse.debug.ui.actions.ILaunchable"/>
>     <test property="org.eclipse.debug.ui.launchable" value="debug"/>
> </enablement>

ILaunchable says you can contribute to the extension but not actually contribute an implementation ... AdaptExpression goes to the AdapterManager and tries to adapt the element ... and returns TRUE if successful and FALSE if not (and NOT_LOADED if the factory isn't loaded yet), which we usually accept as TRUE.  If not having the factory (but having the extension) always means the adapt element will return NOT_LOADED then it should work.

PW
Comment 21 dakshinamurthy.karra CLA 2007-11-15 12:48:42 EST
(In reply to comment #19)
> If that was the solution, then you would use the selection element instead of
> the enablement element and objectState instead of test.  But then you have the
> restrictions of the old expression language, like the objects you query must
> implement IActionFilter, etc.
> 
> This it the trade off of a lazy loading system ... programmatic enablement
> requires loading *someone's* plugin.  You can mitigate the problem by
> separating our your property tests as much as possible, but for accuracy you
> have to load something ... or live with the inaccuracy until some user action
> causes the loading.
> 
I understand that part. What I was thinking of is the first level filtering of properties - check whether an object implements IPropertyFilter and if so whether the given property is supported. The next step is still to call the PropertyTester from the defining plugin.

-- KD
Comment 22 Paul Webster CLA 2007-11-15 13:32:17 EST
(In reply to comment #21)
> I understand that part. What I was thinking of is the first level filtering of
> properties - check whether an object implements IPropertyFilter and if so
> whether the given property is supported.

That is already part of the property tester ... they are targetted at specific objects.  It's just in this case this property tester targets java.lang.Object

But as Markus mentioned maybe <adapt type="org.eclipse.debug.ui.actions.ILaunchable"/> would provide the declarative restriction that would narrow the opportunities to load the property tester.  If that worked then the property tester could probably be moved down to debug.core where it simply loading the tester wouldn't load all of the menu UI elements.


Theoretically, anyway.

PW
Comment 23 Markus Keller CLA 2007-11-15 14:31:59 EST
(In reply to comment #22)

Yup, the <adapt ..> thing works and I don't see why the launchable property tester couldn't be moved to debug.core (and debug.core is already loaded as soon as the Java Perspective is opened).

> (In reply to comment #21)
> > I understand that part. What I was thinking of is the first level filtering of
> > properties - check whether an object implements IPropertyFilter and if so
> > whether the given property is supported.

We don't need a new concept for this. If there's need for another "first level filtering" (apart from the <adapt> clause), then this should also be implemented as a property tester.
Comment 24 dakshinamurthy.karra CLA 2007-11-16 01:13:02 EST
(In reply to comment #23)
> We don't need a new concept for this. If there's need for another "first level
> filtering" (apart from the <adapt> clause), then this should also be
> implemented as a property tester.
> 
There is already enough declarative information for non displaying of contributions without loading the providing plugin. What I was suggesting is to make use of that information as a first layer filter. <adapt> clause does not help as that also works similar to PropertyTester in that it returns NOT_LOADED as a result.

-- KD
 
Comment 25 Markus Keller CLA 2007-11-16 04:54:01 EST
(In reply to comment #24)
That's the whole point of delayed plug-in loading. There are just two choices:
a) delay plug-in loading and accept that an inapplicable contribution could be shown until the property tester's plug-in has been loaded
b) force plug-in loading (comment 10) and pay the performance penalty

> There is already enough declarative information for non displaying of
> contributions without loading the providing plugin.

That's not an acceptable solution. The enablement of contributions always has to err on the side of showing too many when the required plug-ins have not been loeaded. Otherwise, the user cannot know that the contribution exists and how to make it appear.

> <adapt> clause does not help as that also works similar to PropertyTester in
> that it returns NOT_LOADED as a result.

Have you actually tried comment 18? I did, and it worked fine for the example from comment 15, because there's no AdapterFactory registered that could adapt the selected ClassPathContainer to ILaunchable (so the <adapt> expression always evaluates to false).
Comment 26 Markus Keller CLA 2007-12-13 10:45:58 EST
Created attachment 85188 [details]
Fix (using <adapt> expression)

Here's a patch for the trivial fix outlined in comment 18.

Please release this soon to fix this bug for most users until you decide on a final solution, which could also include the proposal from comment 22 & 23 (move property tester to debug.core and, if you want, add 'forcePluginActivation="true"' to the references in debug.ui's plugin.xml).
Comment 27 Steffen Pingel CLA 2007-12-14 17:17:09 EST
*** Bug 209418 has been marked as a duplicate of this bug. ***
Comment 28 Curtis Windatt CLA 2008-01-04 12:22:50 EST
Applied Markus' patch.

Moving the property tester to Debug Core does not look doable.  It tests using ILaunchable and LaunchConfigurationManager, both of which are part of the UI plugin.

With the addition of the <adapt> expression, is this bug still a significant problem?

While it might be a little odd to have all the menus available the first time you right click on a launchable selection, IMO it seems worth avoiding loading the plugin.  In many cases the only submenu that would disappear would be the profile menu.
Comment 29 Markus Keller CLA 2008-01-04 13:14:47 EST
(In reply to comment #28)
> Moving the property tester to Debug Core does not look doable.  It tests using
> ILaunchable and LaunchConfigurationManager, both of which are part of the UI
> plugin.

I think it looks harder than it is. LaunchablePropertyTester#test()'s
'if ("launchable".equals(property) { .. }' block basically only contains:
- a call to LaunchConfigurationManager#launchModeAvailable(String), whose implementation only uses debug.core stuff, and
- an adapter test for ILaunchable, which is not necessary any more since this is already tested by the enablement expression.

> With the addition of the <adapt> expression, is this bug still a significant
> problem?

Not major any more, but nice to have. Moving the property tester to Debug Core would make the Profile menu disappear much earlier (at least for the SDK scenario, where debug.core is already loaded as soon as the first Java project's classpath is resolved).
Comment 30 Curtis Windatt CLA 2008-01-04 13:41:53 EST
In addition, the property tester is an API class, so the tester would have to be duplicated in debug core.

It's doable, but I'm not entirely convinced it is a good idea.  That being said, patches are always welcome and are very convincing ;)
Comment 31 Darin Wright CLA 2008-01-17 10:18:46 EST
*** Bug 214932 has been marked as a duplicate of this bug. ***
Comment 32 Darin Wright CLA 2008-01-17 10:19:44 EST
This fix introduced bug 214932. Removed adapt expressions from HEAD.
Comment 33 Darin Wright CLA 2008-01-23 16:30:56 EST
Created attachment 87702 [details]
patch

This adds a property tester to the debug.core plug-in. It solves the problem in the SDK (as debug.core gets loaded). I'm not sure it would solve the problem in other scenarios where JDT is not present (I think it may be JDT that is loading debug.core via classpath container resolving).

Not a perfect solution. Not sure it's worth the API duplication in property testers (also adds a requirement on debug.core for core.expressions).
Comment 34 Darin Wright CLA 2008-02-01 16:46:10 EST
Applied patch. Added "launchable" property tester to platform
Comment 35 Darin Wright CLA 2008-02-01 16:46:26 EST
Please verify, Curtis.
Comment 36 Curtis Windatt CLA 2008-02-04 15:26:40 EST
Verified.
Comment 37 Martin Oberhuber CLA 2008-05-21 13:06:18 EDT
The problem is not fixed in the following case for instance:

* Launch Eclipse SDK
* Switch to Resource perspective, close all other perspectives
* Quit & Restart Eclipse SDK
* Window > Show View > General > Error Log
* Select an item in error log, right - click

--> Context menu shows "Run As / Debug As / Profile As" on the error log entries.

The example here is just one example, we're also seeing those menu items in a couple of other places in our products, which really confuses users. This is a less-than-trivial issue! If it cannot be fixed properly in one central place, please come up with an explanation how we can customize our views to get rid of these items once and forever.

Reopening, feel free to close if you want that issue discussed in a separate bug.
Comment 38 Darin Wright CLA 2008-05-21 22:22:00 EDT
Removing milestone (since it is now invalid). 

The fix we provided has improved things, but of course there are situations where debug.core is not yet loaded, so the "Run/Debug As" actions show up for the same reason (because the enablement expression evaluates to 'notLoaded' which is considered "true").

I hesitate to only display the actions when the debug plug-in is active, as this will mean that the actions will not appear in valid locations either, which might actually be the trigger for loading debug - i.e. initiating a debug session.

Force loading debug.core is also not an ideal situation.

I wonder if we could have a more complex expression - something like:

"if debug.core is not active, the look for a resource adapter, else test if launchable"
Comment 39 Darin Wright CLA 2008-05-21 22:53:15 EDT
Created attachment 101420 [details]
patch

For example, I tried this expression:

<enablement>
   <or>
     <and>
	<not>
          <pluginState id="org.eclipse.debug.core" value="activated"/>
	</not>
	<adapt type="org.eclipse.core.resources.IResource"/>
     </and>
     <test property="org.eclipse.debug.core.launchable" value="profile"/>             
   </or>               
</enablement>  

But I get an expception when it is evaluated. For some reason the "pluginState" test is not working. Any ideas Paul?
Comment 40 Paul Webster CLA 2008-05-22 06:54:04 EDT
Darin, the objectContribution/enablement element takes a core expression.  You would want something like:

<with variable="org.eclispe.core.runtime.Platform">
  <test property="org.eclipse.core.runtime.bundleState"
        args="org.eclipse.debug.core"
        value="ACTIVE"/>
</with>

It looks like to support this Platform UI would need the same change in org.eclipse.ui.internal.ObjectActionContributor.ObjectContribution.isApplicableTo(Object) that we put in ExpressionAuthority (i.e. adding the variable org.eclipse.core.runtime.Platform to the created EvaluationContext).

PW
Comment 41 Darin Wright CLA 2008-05-22 09:02:51 EDT
Yeah - I tried that first and found that the "platform" variable was not defined. So I used "pluginState" (which just appeared for me when I did code assist in the PDE editor, and seemed to be in the schema docs for enablement).
Comment 42 Paul Webster CLA 2008-05-22 21:40:54 EDT
Created attachment 101671 [details]
Platform available v01

Darin, could you try your tests with this patch for org.eclipse.ui.workbench and the property tester in your core expression:
<with variable="org.eclispe.core.runtime.Platform">
  <test property="org.eclipse.core.runtime.bundleState"
        args="org.eclipse.debug.core"
        value="ACTIVE"/>
</with>


Martin, if that doesn't work you can always start up org.eclipse.debug.core in your product (that will make the menu visibility 100% correct).

PW
Comment 43 Darin Wright CLA 2008-05-26 16:51:41 EDT
Created attachment 102046 [details]
patch

This works with the new variable added to the workbench. 

In the error/bookmark/task view, no run/debug as appears. However, for things that have a resource adapter, run/debug/profile as appear. Since the debug plug-in is not loaded, the profile option appears.

So, it's one more step in the right direction.
Comment 44 Darin Wright CLA 2008-05-28 10:25:10 EDT
Marking as RC3 candidate, pending workbench fix.
Comment 45 Michael Rennie CLA 2008-05-29 11:32:57 EDT
+1 now with 100% less shortcuts in the error log
Comment 46 Curtis Windatt CLA 2008-05-29 12:17:07 EDT
+1 Fixed in HEAD.
Comment 47 Curtis Windatt CLA 2008-05-29 12:18:18 EDT
Verified.