Bug 307026

Comment 1 Dani Megert 2010-03-25 04:38:33 EDT

Let me take that one.

Comment 2 Dani Megert 2010-03-25 04:44:55 EDT

org.eclipse.team.internal.ui.synchronize.actions.OpenFileInSystemEditorAction also extends OpenFileAction.

Comment 3 Dani Megert 2010-08-20 06:08:31 EDT

Boris, OK for you to do this?

Comment 4 Paul Webster 2010-08-23 08:25:09 EDT

Can we stop promoting actions?  The compatibility layer needs to support them, but because they're bridged they're slower in 4.x and not supported at all in e4.

If we're going to add new API, it should be in the supported command/handler system.

PW

Comment 5 Dani Megert 2010-08-23 08:29:11 EDT

>If we're going to add new API, it should be in the supported command/handler
>system.
Would you have time to do this? Where would they live and how would they look like?

Comment 6 Boris Bokowski 2010-08-23 14:18:34 EDT

I would be in favour of publishing API that made it possible for clients to get at the required functionality. However, we should try to make this available "library-style" (through a refactoring of the current code if necessary). This would make the new API independent of the current class hierarchy and would address Paul's concerns about making more of the action classes API.

Comment 7 Dani Megert 2010-08-23 14:51:51 EDT

> However, we should try to make this available "library-style" 
Can you be a bit more specific? Is there already such a library entry point? Wouldn't it be better to do it as Paul suggested?

Also, the class is already API. We would only allow to extend it. Is that really an issue regarding e4?

Comment 8 Paul Webster 2010-08-24 13:41:05 EDT

(In reply to comment #5)
> Would you have time to do this? Where would they live and how would they look
> like?

I'm having a quick look at this tomorrow.
PW

Comment 9 Dani Megert 2010-08-25 01:56:15 EDT

>I'm having a quick look at this tomorrow.
>PW
Great, thanks! Could you also explain to me why allowing to sub-class an existing API class which has to be supported in 4.x causes problems for 4.x?

Comment 10 Dani Megert 2010-09-01 02:49:40 EDT

> (In reply to comment #8)
> I'm having a quick look at this tomorrow.
> PW
;-)

Comment 11 Paul Webster 2010-09-07 07:45:44 EDT

(In reply to comment #10)
> ;-)

Sorry, Dani, I went out with a Flu all last week.  But it's still on my list of things to write up :-)

Soon,
Paul

Comment 12 Dani Megert 2010-09-29 04:48:10 EDT

> Soon,
> Paul
Ping ;-)

Comment 13 Paul Webster 2010-09-29 13:38:40 EDT

A command version of this would probably look like:

An openResource command with 2 optional parameters, editorId and file

The behaviour would be similar to org.eclipse.ui.actions.OpenFileAction.  If there was an editorId specified, it would try and open the IFile with that editor, otherwise fall back on something like IDE.openEditor(*).  If there is an IFile specified try and open it, otherwise request the appropriate selection from the IEvaluationService context.

With the handled execute(*) implementing the behaviour, it could be overridden by another handler.  Then it's up to use if we want to make the handler API to make that easier.

For placing in menus we can provide something like the call below as a shortcut.

IContributionItem openItem = IDEContributionItemFactory.OPEN_FILE.create(window, String editorId, IFile file); although anyone can build a CommandContributionItem themselves.

OpenWithMenu is already a contribution item.  I would change it to a CompoundContributionItem and generate the CommandContributionItems necessary to use the openResource command.  Depending on how useful it could also be modified so it could be used in the menuContribution/dynamic element.

PW

Comment 14 Paul Webster 2010-09-29 13:46:50 EDT

Hi Dani,

Once again, sorry for the lateness of my reply.

(In reply to comment #9)
> Great, thanks! Could you also explain to me why allowing to sub-class an
> existing API class which has to be supported in 4.x causes problems for 4.x?

In 4.0 action extension points are deprecated, and in 4.1 this will apply to the workbench use of actions.  Resourcing has slowed down our conversion of actions to commands although not stopped it.  The actions will continue to work, of course.  They'll still excel in the "localized case" since they're a useful API to encapsulate creating a menu item/tool item and providing something to execute for an SWT.Selection event.

But they're even more removed from the e4 framework than they are from the 3.x commands and menu contributions.  Evolving our 4.1/4.x into something that can take advantage of the new e4 functionality will hopefull accelerate over 4.1, but actions are more a wrapper on creating SWT widgets and less part of the framework.

Given that, even though we must continue to provide our 3.x action API in the compatibility layer, opening them up (expanding their scope as API) seems to be heading "the other way" down the road.

I'm not dead set against it, and of course we must do what we must do, but I'd prefer to avoid it.

PW

Comment 15 Dani Megert 2010-09-30 09:22:37 EDT

> For a concrete use case see bug 153912. There we need to be able to extend
> - OpenWithMenu.openEditor(IEditorDescriptor, boolean)
> - OpenFileAction.run()
Sorry, that was the wrong bug #. It was need to fix bug 40632, bug 267171 and bug 315722.

It looks like the preference is to not allow an existing API class which is all this bug is about. Therefore closing as WONTFIX.

Comment 16 Dani Megert 2010-09-30 09:23:18 EDT

It looks like the preference is to not allow an existing API class
>allow to extend an existing API class

Comment 17 Dani Megert 2010-10-01 02:34:17 EDT

Paul, I've read your comments and still don't understand why one would switch over to commands and/or handlers instead of using the action to create the menu items. In comment 14 you say yourself:

  They'll still excel in the "localized case" since they're a
  useful API to encapsulate creating a menu item/tool item

and that's exactly where the subclasses of OpenWithMenu and OpenFileAction are used. I don't see what it would buy us to do some inconvenient dances to go via commands and/or handlers here. In addition I believe that if we did so, it would result in code duplication.

Comment 18 Paul Webster 2010-10-22 08:46:38 EDT

(In reply to comment #17)
> Paul, I've read your comments and still don't understand why one would switch
> over to commands and/or handlers instead of using the action to create the menu
> items. In comment 14 you say yourself:
> 
>   They'll still excel in the "localized case" since they're a
>   useful API to encapsulate creating a menu item/tool item

Yes, actions are localized.  They're nice window dressing on top of an SWT.Selection event.  And I would still recommend them if a behaviour (or action, pardon the overuse :-) doesn't need to be represented in a number of places, doesn't need to be accessed through keybindings or the framework, etc.

> and that's exactly where the subclasses of OpenWithMenu and OpenFileAction are
> used. I don't see what it would buy us to do some inconvenient dances to go via
> commands and/or handlers here. In addition I believe that if we did so, it
> would result in code duplication.

In this specific case I would say that OpenFile isn't a localized behaviour, but is applicable to more than just a view menu.  It's not unreasonable that Open File can be used in a number of places.  That is something that I would recommend be a command/handler.

PW