Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 317707

Summary: Convert Resource Configurations context menus to handlers
Product: [Tools] CDT Reporter: David Dubrow <david.dubrow>
Component: cdt-coreAssignee: David Dubrow <david.dubrow>
Status: RESOLVED FIXED QA Contact: Doug Schaefer <cdtdoug>
Severity: normal    
Priority: P3 CC: angvoz.dev, ed.swartz, jamesblackburn+eclipse, john.cortell, warren.paul
Version: 7.0Flags: david.dubrow: iplog-
angvoz.dev: review+
Target Milestone: ---   
Hardware: PC   
OS: Windows XP   
Whiteboard:
Attachments:
Description Flags
proposed change
none
proposed change - post review
david.dubrow: iplog-
proposed change - post review (2)
david.dubrow: iplog-, david.dubrow: review?
proposed change david.dubrow: iplog-, david.dubrow: review?

Description David Dubrow CLA 2010-06-23 10:34:41 EDT 
The two sub-menus of the "Resource Configurations" context menu item and its sub-menu items "Exclude from Build..." and "Reset to Default..." don't seem to apply in our product. We don't use managed build and we manage our own indexer settings. Yet, this menu is visible and confusing to our users. The history of the commands seems to point to them being used to manage indexer behavior.

Since these menus are provided as popup menu elements in object contribution extensions, they can't be turned off via capabilities. I would like to convert them to the new command handler mechanism so they can be hidden.

(Could someone explain how these are supposed to work in case they may be useful in our case?)
Comment 1 James Blackburn CLA 2010-06-23 11:23:27 EDT 
"Exclude from build" alters the source path path-entries. This has knock on effects for managedbuild (the source isn't built) and the indexer - the source isn't indexed (I believe).  
See: Properties > C/C++ General > Paths & Symbols > Source Location.

"Reset to Default" allows removing resource-specific paths & symbols (as well as build settings...).  So if you add an include search path to a folder or file: properties > C/C++ General > Paths & Symbols > Includes > Add ...  
Reset to Default deletes the resource specific configuration. 

Both of these should be applicable to all integrations irrespective of MBS being used.  Ideally there would be a better name / description, but I'm not sure what that might be...
Comment 2 Andrew Gvozdev CLA 2010-06-23 11:39:28 EDT 
That is an interesting idea to control those via capabilities. See also some of a discussion in bug 207816.
Comment 3 Ed Swartz CLA 2010-06-23 15:51:45 EDT 
(In reply to comment #1)
> "Exclude from build" alters the source path path-entries. This has knock on
> effects for managedbuild (the source isn't built) and the indexer - the source
> isn't indexed (I believe).  
> See: Properties > C/C++ General > Paths & Symbols > Source Location.

Ok, I see.  But this specific panel is not available in our product's projects.  We have a custom builder and custom CConfigurationDataProvider, and the indexer settings and source paths are derived entirely from build files.  (I.e. we don't want the user to make changes like this through this UI, since it may be misleading).
 
> "Reset to Default" allows removing resource-specific paths & symbols (as well
> as build settings...).  So if you add an include search path to a folder or
> file: properties > C/C++ General > Paths & Symbols > Includes > Add ...  
> Reset to Default deletes the resource specific configuration. 

This one wouldn't be useful for us either -- we only support project-wide paths & symbols settings.

> Both of these should be applicable to all integrations irrespective of MBS
> being used.  Ideally there would be a better name / description, but I'm not
> sure what that might be...

I agree the code seems to be amenable to any sort of CDT project, but our concern isn't about the naming, but just about not exposing it to our users unless they want to see it (i.e. capabilities).

I recommend we just provide a patch to convert these actions into commands, giving "Resource configurations" submenu its own capability-controllable identifier.  The actual functionality/naming/enablement tweaks can be done elsewhere.
Comment 4 James Blackburn CLA 2010-06-23 16:39:59 EDT 
(In reply to comment #3)
> Ok, I see.  But this specific panel is not available in our product's projects.
>  We have a custom builder and custom CConfigurationDataProvider, and the
> indexer settings and source paths are derived entirely from build files.  (I.e.
> we don't want the user to make changes like this through this UI, since it may
> be misleading).

Wow, finally found someone outside managedbuild who uses this API :)!

> I recommend we just provide a patch to convert these actions into commands,
> giving "Resource configurations" submenu its own capability-controllable
> identifier.  The actual functionality/naming/enablement tweaks can be done
> elsewhere.

Sounds reasonable to me.
Comment 5 David Dubrow CLA 2010-06-24 16:27:26 EDT 
Created attachment 172684 [details]
proposed change

Most of the code is the same between the old actions and the new handlers. Since the actions were API, I had to up the bundle version.
Comment 6 Andrew Gvozdev CLA 2010-06-25 00:50:23 EDT 
(In reply to comment #5)
> Created an attachment (id=172684)
> proposed change
Here are some comments:

- You changed where Resource Configuration menu appears, is it for a reason? It was in the "build" group with indexer and Make Targets before.
- Resource Configuration menu is not supposed to appear on project level, only for files and folders.
- You limit menu appearance in editor to 5 hardcoded content types. So it does not show for assembler files for example. I assume it is conceivable to have other source types as well and it is inconsistent with PE.
- The enablement actually does not work from the editor anymore, the submenu items are always grayed.
- One provoked observation not really related to the patch: currently the menu shows in Project Explorer pretty much for every resource (except projects) and items enabled in some cases where it does not make sense.

A question (or 3 of them):
- Are you interested in "Build Configurations" item? Do you think it should be controlled by the same capability? Do you have configurations in your custom managed build?

A couple of comments related to the code:
- You need to add copyright headers to the new files, see http://www.eclipse.org/legal/copyrightandlicensenotice.php
- In manifest, you should increment to 5.3.0, not to 5.3.100 (from 5.2.100).
- I suggest to indicate in JavaDoc when the interface is deprecated (i.e. "deprecated as of CDT 8.0"). That will help later to decide when to retire it.
Comment 7 David Dubrow CLA 2010-06-25 12:21:13 EDT 
Created attachment 172774 [details]
proposed change - post review

Hi Andrew, thanks for the review. :) The issues you mention were caused inadvertently, but thanks for pointing them out. See my comments below.

(In reply to comment #6)
> Here are some comments:
> 
> - You changed where Resource Configuration menu appears, is it for a reason? > It was in the "build" group with indexer and Make Targets before.

I was able to put it into the build group when it was off the resource, but there is no such group in the menu when in the editor, so I left it after additions which I believe was just like before.

> - Resource Configuration menu is not supposed to appear on project level, only
> for files and folders.

Fixed.

> - You limit menu appearance in editor to 5 hardcoded content types. So it does
> not show for assembler files for example. I assume it is conceivable to have
> other source types as well and it is inconsistent with PE.

You are right. I've replaced the whole mechanism for determining whether it is an appropriate editor input by adapting it to a file in a project with c nature. This is closer to the original test.

> - The enablement actually does not work from the editor anymore, the submenu
> items are always grayed.

Fixed.

> - One provoked observation not really related to the patch: currently the menu
> shows in Project Explorer pretty much for every resource (except projects) and
> items enabled in some cases where it does not make sense.

I didn't address this, but did retain the behavior where it was shown for only files and folders in projects with c nature.

> 
> A question (or 3 of them):
> - Are you interested in "Build Configurations" item? Do you think it should be
> controlled by the same capability? Do you have configurations in your custom
> managed build?

You are right in that my primary concern is to be able to hide them using capabilities since they are not very applicable to our project model - see comment #3. The current implementation using object contributions and actions don't respond to capabilities at all. The capabilities to hide them were going to be added in Nokia-specific code. Converting them to the 3.3+ handler model would just allow capability hiding.

> A couple of comments related to the code:
> - You need to add copyright headers to the new files, see
> http://www.eclipse.org/legal/copyrightandlicensenotice.php

Done. I put the copyright header from the action files and added a contribution line to show the conversion to handlers. Most of the code is exactly the same as it was in the action class.

> - In manifest, you should increment to 5.3.0, not to 5.3.100 (from 5.2.100).

Fixed.

> - I suggest to indicate in JavaDoc when the interface is deprecated (i.e.
> "deprecated as of CDT 8.0"). That will help later to decide when to retire it.

Done.
Comment 8 David Dubrow CLA 2010-06-25 12:23:09 EDT 
Created attachment 172775 [details]
proposed change - post review (2)

Oops, removed an extraneous file.
Comment 9 Andrew Gvozdev CLA 2010-06-26 21:54:27 EDT 
(In reply to comment #8)
> Created an attachment (id=172775)
> proposed change - post review (2)
I don't see anything to pick on in the latest patch, thanks. Marking review flag with a +.
Comment 10 David Dubrow CLA 2010-06-28 08:59:11 EDT 
(In reply to comment #9)
> I don't see anything to pick on in the latest patch, thanks. Marking review
> flag with a +.

Thanks for the review!
Comment 11 CDT Genie CLA 2010-06-28 09:23:07 EDT 
*** cdt cvs genie on behalf of ddubrow ***
Bug 317707 convert resource configs context menu to handlers to allow hiding via capabilities

[*] ExcludeFromBuildAction.java 1.11 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/src/org/eclipse/cdt/ui/actions/ExcludeFromBuildAction.java?root=Tools_Project&r1=1.10&r2=1.11
[+] ExcludeFromBuildHandler.java  http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/src/org/eclipse/cdt/ui/actions/ExcludeFromBuildHandler.java?root=Tools_Project&revision=1.1&view=markup
[+] DeleteResConfigsHandler.java  http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/src/org/eclipse/cdt/ui/actions/DeleteResConfigsHandler.java?root=Tools_Project&revision=1.1&view=markup
[*] DeleteResConfigsAction.java 1.11 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/src/org/eclipse/cdt/ui/actions/DeleteResConfigsAction.java?root=Tools_Project&r1=1.10&r2=1.11

[*] plugin.properties 1.202 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/plugin.properties?root=Tools_Project&r1=1.201&r2=1.202
[*] plugin.xml 1.380 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/plugin.xml?root=Tools_Project&r1=1.379&r2=1.380

[*] MANIFEST.MF 1.55 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/META-INF/MANIFEST.MF?root=Tools_Project&r1=1.54&r2=1.55
Comment 12 John Cortell CLA 2010-06-29 14:08:17 EDT 
I committed a fix to DeleteResConfigsHandler for an NPE I'm seeing during shutdown. Simple null pointer check. Genie shoud post the commit info shortly...
Comment 14 David Dubrow CLA 2010-06-29 14:25:36 EDT 
(In reply to comment #12)
> I committed a fix to DeleteResConfigsHandler for an NPE I'm seeing during
> shutdown. Simple null pointer check. Genie shoud post the commit info
> shortly...

Thanks! I never saw the null selection when I tested.
Comment 15 David Dubrow CLA 2010-07-01 09:18:05 EDT 
Created attachment 173214 [details]
proposed change

Andrew, would you mind for the new handlers to not be API (added to an internal package)? This would allow me to backport this to 7.0.1 since it would allow me to maintain the same bundle version.

I could keep the deprecated tags out of the 7.0.1 version in case someone is depending on the action classes. The only difference is that the menus would be using the non-api handlers rather than the old actions.
Comment 16 Andrew Gvozdev CLA 2010-07-01 09:31:25 EDT 
(In reply to comment #15)
> Created an attachment (id=173214)
> proposed change
> Andrew, would you mind for the new handlers to not be API (added to an internal
> package)? This would allow me to backport this to 7.0.1 since it would allow me
> to maintain the same bundle version.
No, not at all. In fact, I regret that I did not suggest that in my review. I am not aware of any reason to keep them as API.
Comment 17 David Dubrow CLA 2010-07-01 09:37:41 EDT 
(In reply to comment #16)
> No, not at all. In fact, I regret that I did not suggest that in my review. I
> am not aware of any reason to keep them as API.

Thanks! I had also forgotten that we plan to ship our next release with 7.0.1! :)
Comment 18 CDT Genie CLA 2010-07-01 10:23:04 EDT 
*** cdt cvs genie on behalf of ddubrow ***
Bug 317707 make new handlers non-api and revert bundle version

[*] ExcludeFromBuildAction.java 1.12 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/src/org/eclipse/cdt/ui/actions/ExcludeFromBuildAction.java?root=Tools_Project&r1=1.11&r2=1.12
[-] ExcludeFromBuildHandler.java  http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/src/org/eclipse/cdt/ui/actions/ExcludeFromBuildHandler.java?root=Tools_Project&view=markup
[-] DeleteResConfigsHandler.java  http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/src/org/eclipse/cdt/ui/actions/DeleteResConfigsHandler.java?root=Tools_Project&view=markup
[*] DeleteResConfigsAction.java 1.12 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/src/org/eclipse/cdt/ui/actions/DeleteResConfigsAction.java?root=Tools_Project&r1=1.11&r2=1.12

[+] DeleteResConfigsHandler.java  http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/actions/DeleteResConfigsHandler.java?root=Tools_Project&revision=1.1&view=markup
[+] ExcludeFromBuildHandler.java  http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/actions/ExcludeFromBuildHandler.java?root=Tools_Project&revision=1.1&view=markup

[*] plugin.xml 1.381 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/plugin.xml?root=Tools_Project&r1=1.380&r2=1.381

[*] MANIFEST.MF 1.56 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/META-INF/MANIFEST.MF?root=Tools_Project&r1=1.55&r2=1.56