Bug 294416 - [CommonNavigator] Make CopyAction and PasteAction API
Summary: [CommonNavigator] Make CopyAction and PasteAction API
Status: CLOSED WONTFIX
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.5.1   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: 3.6 M6   Edit
Assignee: Francis Upton IV CLA
QA Contact: Francis Upton IV CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-05 22:08 EST by Terran Gilman CLA
Modified: 2010-03-12 11:55 EST (History)
1 user (show)

See Also:


Attachments
Proposed solution for 3.6M6 (38.13 KB, patch)
2010-03-06 23:26 EST, Francis Upton IV CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Terran Gilman CLA 2009-11-05 22:08:06 EST
I am currently converting the existing custom file navigator view of the Voice Tools Project over to be a content contribution to the Project Explorer.  When wireing up the various context menu and action bar items I noticed that I could use the CopyAction and PasteAction classes without modification to enable these operations.  However, they are currently marked package private.  I have been using the JDT and CDT as examples in this task and noticed they have basically just lifted your source and placed them into their package space "as is".

I will be doing this as well to move the conversion process forward, but would love to simply instantiate the originals as I already have a dependency on that bundle.  Is there any reason these classes are package private as they stand?  

Thanks,
Trip Gilman
Voice Tools Project Lead
Comment 1 Paul Webster CLA 2009-11-06 08:08:39 EST
It would not involve making those classes public (they're actions) but converting them to some kind of appropriate handler.

PW
Comment 2 Francis Upton IV CLA 2010-03-05 06:01:16 EST
Paul, I would like to convert these actions to a handler, but I would like to preserve as much of the code as possible. Currently, the CopyAction for example is based on the SelectionListenerAction which gets called whenever the selection is changed to determine if the action should be enabled based on the current selection.

I would like to use exactly the code in CopyAction.updateSelection() to determine the enablement of then handler. I have looked through the command documentation, wiki, and Prakash's blog and it's not obvious to me where to hook this code. But I bet there is an easy way do this. Can you help me?

Also, what I would like to try and do is essentially convert all of the org.eclipse.ui.navigator.resources actions to commands/handlers and remove the grouping that's wired into the classes like EditActionGroup so that clients can have complete flexibility in choosing which commands they want to implement and the grouping of them. I'm hoping this all will not take much work and can be done in the next couple of days I have before M6.

I don't feel the need to make these handlers API since they generally will either be used whole or not (there is little value in subclassing them). Of course their command ids will be known, but I think those are already established.

Comments?
Comment 3 Francis Upton IV CLA 2010-03-05 06:08:54 EST
See also bug 298676
Comment 4 Francis Upton IV CLA 2010-03-06 23:26:40 EST
Created attachment 161235 [details]
Proposed solution for 3.6M6

Paul,

After playing around with adding proper support for commands, it turns out to be harder and more risky than (even) I want to do this late in the 3.6 game. It's possible to make the copy and paste actions handlers of course, but the interaction with the common action providers (which have their own override mechanism) is not clear. And CNF clients like JDT do override these actions.  This work is captured in bug 304925, which is probably something for 3.7, or possibly M7, as I don't think it hits API.

I recommend for 3.6 that we go ahead and make the actions public since so much of the CNF support now is based in actions. The current o.e.ui.navigator.resources CopyAction and PasteAction are copied from the resource navigator in the IDE plugin (which is deprecated in 3.5).  I recommend we copy those original actions into the IDE extensions in o.e.ui.actions along with all of the rest of the similar actions where they would fit well.

That's what this patch does; the one thing I have not finished about the patch (and don't want to spend the time unless you think it's a good idea to get it into M6) is to switch from the ResourceNavigatorMessages to the IDE messages.

I know this makes a couple of actions API, but overall I think it's the cleanest solution given where the CNF is in 3.6.
Comment 5 Francis Upton IV CLA 2010-03-07 12:12:15 EST
We are not going to do this change, here is the rationale from Paul (which I agree with). In the next release (3.7) we will have better command support for the CNF which will solve this problem.

(04:50:53 AM) paulweb515_: francis4: I wouldn't release the changes.  I don't think making copy action and paste action API are anything more than a convenience.
(04:51:53 AM) paulweb515_: francis4: I think you're right about deferring the proper support for commands until you can analyze the interactions between your current actionProviders
(04:52:53 AM) paulweb515_: francis4: but we wouldn't make actions API unless we can avoid it, and from this bug it looks like the reason is "it would be better if we didn't have to copy the code"
(04:54:03 AM) paulweb515_: francis4: it's not a bad reason :-)  but I don't think that justifies putting unplanned API (i.e. it wasn't really the plan to make these actions API) this late in the game
(04:54:38 AM) paulweb515_: francis4: maybe there's more to the reasons, but from what's available in the bug I'd say to defer this to 3.7 as well
(04:56:16 AM) paulweb515_: francis4: just a comment on the patch.  If you *were* making these API the constructors would take an IShellProvider, no longer a Shell.  Also, you'd probably have to leave the internal classes there (maybe re-implement them in terms of the public action) since we're trying to allow RCP apps and products that (wrongly) used internals to upgrade from 3.4 to 3.6 in a less painful manner
Comment 6 Francis Upton IV CLA 2010-03-12 11:55:13 EST
Closing