Bug 251776 - [Content Type] Need a better way to arbitrate content types and/or editor associations
Summary: [Content Type] Need a better way to arbitrate content types and/or editor ass...
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.4   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: 3.8 M5   Edit
Assignee: Dani Megert CLA
QA Contact:
URL:
Whiteboard:
Keywords: investigate
: 240405 (view as bug list)
Depends on: 370500
Blocks: 249156 370864
  Show dependency tree
 
Reported: 2008-10-22 16:35 EDT by Chris Recoskie CLA
Modified: 2012-02-08 02:16 EST (History)
15 users (show)

See Also:


Attachments
Sample plug-in (7.49 KB, application/zip)
2010-11-05 12:35 EDT, Szymon Brandys CLA
no flags Details
The screenshot showing Sample plug-in working (122.97 KB, image/jpeg)
2010-11-05 13:02 EDT, Szymon Brandys CLA
no flags Details
Possible Fix (33.57 KB, patch)
2011-11-30 09:05 EST, Dani Megert CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Recoskie CLA 2008-10-22 16:35:47 EDT
We have tooling in PTP's RDT (Remote Development Tools) which provides remote versions of most of CDT's functionality.  Because we are reusing and extending as much of CDT as we can, we need to have resources in our remote projects represented by the content types which CDT defines.

Unfortunately, we have hit a bit of a snag.  Since our editor is associated with the same content types that the CDT editor is associated with, by default Eclipse always tries to use the remote editor instead of the regular CDT editor (presumably this is a case of "the last one contributed wins").  This works fine if your project is a remote project, but causes problems for ordinary CDT projects because the remote editor expects certain things to be configured on the project the resource comes from in order for it to know what machine certain services are coming from.  Havoc ensues.

We need a more advanced way of defining the content types and/or editor associations so that we can ensure that when opening a file in a remote project (which has its own remote nature in addition to the regular CDT nature), that the remote editor is used.  Similarly, when opening a file from a regular CDT project, we need to ensure that it's the CDT editor that will be used.

Here are some things we looked into that didn't pan out:

- Creating new remote content types and associating them with our RemoteNature.  Since our content types had to inherit from the CDT ones in order for the rest of CDT to work, the editor system decided that our editor need not even show up in the context menu any more.  This meant the CDT editor always won no matter what we did.

- Content Describers.  All the IContentDescriber.describe() method gets is an input stream to the file.  There's no way from the input stream to tell the difference in how the file should be opened because the file contents look the same between the local case and the remote case.  You can't get the filename out of the input stream to determine what project it belongs to.  We hoped that maybe we could do some hackery like check the type of the input stream to see if it was a FileInputStream (and hence local), but the content type system always gives you a LazyInputStream.

A good start to my mind would be to have an IContentDescriber2 interface, which has an additional describe() method which foregoes the input stream in favour of an IFile.  This way we can perform arbitrary processing based on any project specific information we want.  A method that takes an IFile is handier over one that just has a URI because there may be more than one workspace resource that points to the same URI, and having he actual IFile removes that ambiguity.

It is not enough however to strictly limit the system to resources in the workspace.  We often open external files, and we need to be able to sensibly determine content types for those too.  As such, a method that takes a URI is still required to deal with these files.

However, there is a wrinkle with the URI case, as in many cases the required context to know how the user wants to open the file is dependent upon how they opened it.  For example, if the user is browsing through a source file in a remote project, sees a #include <foo.h> statement, and commands the editor to open that file by hitting F3 or by using a context menu, they will expect that the file be opened in the same type of editor they were just using.  Similarly, if they had the source file open in the regular CDT editor, they'd expect that opening the header file from it should use the CDT editor.  How exactly we handle this issue of context I'm not sure, due to the myriad of ways that editors can be opened.

It may make sense to clone this bug and track part of this against the UI component.  The discussion of content types and editor mapping is somewhat entwined though, so I thought it best to spell out the problems all in one place for starters, so that everyone has a clear picture of the entire situation.
Comment 1 Eduard Bartsch CLA 2010-03-11 10:50:30 EST
I have proposed a patch to Eclipse Content Type management (bug 305481) that may help with this requirement.

As far as I understand, RDT implements an own EFS file store. With the proposed patch, it should be possible to adapt the RDT file store and return an own MIMETypeProvider that would then deliver the MIME types that can be mapped to remote editors. Hence, when a CDT project is opened on RDT, the remote editors will be default. When opened on a local file system, the usual mappings to CDT editors via file extention will be active.
Comment 2 Szymon Brandys CLA 2010-11-04 14:20:06 EDT
ContentDescriber has no notion of IFile. I think that we should be able to declare editor association using some wider context than just content type binding.

Instead of contentTypeId in the editor extension, we should be able to use a resolver that based on the UI context, content types, IFile etc. can say that this editor should be used.
Comment 3 John Arthorne CLA 2010-11-04 15:02:51 EDT
Content type is (by definition) purely a description of a file's *content*. Changing content type based on where the file is located, how it was opened, who is viewing it, etc, seems like a very unnatural use of content type to me. This kind of contextual information is not available at the time a client requests a file's content description

What this problem definition says to me is that editor association based on content type alone is insufficient. Perhaps the problem is being viewed from the perspective of content types because that is the available editor association mechanisms available today. What if an editor extension could declare what file system schemes it supports, much like it specifies the supported file extensions today?
Comment 4 David Dykstal CLA 2010-11-04 15:33:15 EDT
I agree that overloading ContentDescriber is probably not the correct way to implement this. It looks like we need to add another scheme/extension point based on context. Such a scheme certainly could take the content description into account so it would be useful to determine that first if any describers were registered.

Content describers have limitations as to plugin activation. I assume that any context-based editor determination would have these same limitations? If so, are there other restrictions that should be placed on these beasts?
Comment 5 Szymon Brandys CLA 2010-11-04 19:18:08 EDT
(In reply to comment #4)
> I agree that overloading ContentDescriber is probably not the correct way to
> implement this. It looks like we need to add another scheme/extension point
> based on context. Such a scheme certainly could take the content description
> into account so it would be useful to determine that first if any describers
> were registered.

Couldn't we just extend the editor ext. point as we already suggested?

> Content describers have limitations as to plugin activation. I assume that any
> context-based editor determination would have these same limitations? If so,
> are there other restrictions that should be placed on these beasts?

It's not clear to me what limitation you are talking about. Could you explain, please?
Comment 6 Paul Webster CLA 2010-11-05 08:29:40 EDT
I don't believe it can be added to editors or content descriptions by themselves, as both extension points are open-ended contribution systems.  They both have the same problem, which is when a set of equivalent contributions are available, they both returns a semi-random [1] one.

It seems to me that both extension points need a mediation object that can help rank and select/sort the return list.

However ... we've encountered this problem in a number of places (like controling the order of preference categories/preference nodes).  You can allow a product to provide mediation for a given extension point, but you can't easily let contribution rank themselves.  At least that's what we've found.

While it wouldn't be able to mediate, we would be able to provide a <validWhen/> element to the editor descriptors that could use the current state (a la IEvaluationService) to disqualify itself (based on known information).  That wouldn't allow you to select amongst different editor descriptors, but would allow an editor descriptor to state it's only valid for a certain content type and if the editor input is in a project with a certain EFS (using core expressions).

PW

[1] they do tend to return the same one consistently, most often the first one found based on bundle ID lexigraphical order, but even this is not always the case :-)
Comment 7 Szymon Brandys CLA 2010-11-05 10:12:51 EDT
(In reply to comment #6)
> While it wouldn't be able to mediate, we would be able to provide a
> <validWhen/> element to the editor descriptors that could use the current state
> (a la IEvaluationService) to disqualify itself (based on known information). 
> That wouldn't allow you to select amongst different editor descriptors, but
> would allow an editor descriptor to state it's only valid for a certain content
> type and if the editor input is in a project with a certain EFS (using core
> expressions).

Well, I like this approach, however... it would not help in the case when we have a c++ file in an RDT project. We would still have two valid editors (CTD and RDT one) in a random order. After a chat with Paul it seems we need a mediation mechanism anyway.

Moving to UI. We could consider adding a similar mediation mechanism for content describers, however it would depend on the UI solution.
Comment 8 Mike Wilson CLA 2010-11-05 10:29:59 EDT
Paul and John, I'm assuming you guys will be continuing to work with Dave et al. on this.
Comment 9 Szymon Brandys CLA 2010-11-05 12:04:05 EDT
(In reply to comment #7)
> Moving to UI. We could consider adding a similar mediation mechanism for
> content describers, however it would depend on the UI solution.

I've just recalled that there is a mediation mechanism for content types that takes natures into account. In the nature extension we can specify which content types should be assigned to projects of this nature. I think that content types assigned to a nature just get a higher rank, but I have to double check.
Comment 10 Szymon Brandys CLA 2010-11-05 12:35:44 EDT
Created attachment 182495 [details]
Sample plug-in

This sample plug-in shows haw we can use the mediation mechanism. David, what do you think?
Comment 11 Szymon Brandys CLA 2010-11-05 13:02:19 EDT
Created attachment 182499 [details]
The screenshot showing Sample plug-in working
Comment 12 Dani Megert CLA 2010-11-10 09:58:35 EST
>I don't believe it can be added to editors or content descriptions by
>themselves, as both extension points are open-ended contribution systems. 
I'm not convinced that this holds for the 'org.eclipse.ui.editors' extension point: It already allows to specify a matching strategy which is used to find the right editor among open editors. Couldn't we add a similar matching strategy (based on editor descriptors) that is used to find which editor to take?

Another approach could be that both editors are no longer wired to the content type (and maybe renamed) and that the "CDT Editor" is no longer specified as an editor but via IEditorLauncher. The launcher could then do the additional smartness/delegation. This would probably need an extension point on the CDT side.
Comment 13 Paul Webster CLA 2010-11-12 10:48:16 EST
(In reply to comment #12)
> I'm not convinced that this holds for the 'org.eclipse.ui.editors' extension
> point: It already allows to specify a matching strategy which is used to find
> the right editor among open editors. Couldn't we add a similar matching
> strategy (based on editor descriptors) that is used to find which editor to
> take?

That would work for this specific case:

1) we get a set of possible editor descriptors.
2) we ask any descriptors that have a strategy to pick one (in this case the RDT C/C++ editor description knows how to pick between them).

My concern is the second contribution set that comes along, and wants to offer some enhanced C/C++ editing based on some other criteria.  Now we would be in the situation where we would have to rank the strategies so we could determine which one is allowed to proceed.  This is what I refer to as the open-ended contribution ranking problem.

It seems there are a couple of suggestions:

1) Use a content type that can take a project nature into account, which would give the RDT editor a higher priority over the CDT editor.

2) Collude with CDT.  Have the current CDT editor provide an IEditorLauncher that delegates to the now internal real CDT Editor.  Plug into the editor launcher so that it can pick between RDT and CDT editors if available.

3) Allow an editor descriptor to provide a selection strategy, that can be applied to the list of valid editor descriptors for a given input.  On strategy collisions we would use whatever the first strategy returned.

4) Allow the product to provide a comparator to sort a list of valid editor descriptors.  That would make the products responsible for mediating this kind of problem but has proven difficult to maintain in the past.

#1 and #2 can be done with the current 3.x.

PW
Comment 14 Chris Recoskie CLA 2011-01-25 12:47:45 EST
(In reply to comment #10)
> Created attachment 182495 [details]
> Sample plug-in
> This sample plug-in shows haw we can use the mediation mechanism. David, what
> do you think?

We took a look at these plugins.  It seems to rely upon there being only one content type definition that sets its priority to high.  Once you have more than one set to high, it's once again a race to see which one wins.  The CDT content types are already set to high...
Comment 15 Paul Webster CLA 2011-06-27 11:00:51 EDT
(In reply to comment #14)
> 
> We took a look at these plugins.  It seems to rely upon there being only one
> content type definition that sets its priority to high.  Once you have more
> than one set to high, it's once again a race to see which one wins.  The CDT
> content types are already set to high...

Do the CDT content types need to be high?  What were they competing with in the eclipse install?

PW
Comment 16 Paul Webster CLA 2011-06-27 11:17:12 EDT
(In reply to comment #15)
> Do the CDT content types need to be high?  What were they competing with in the
> eclipse install?

But it probably couldn't be changed anywhere, because it might lead to a unanticipated change in behaviour out in the wilds.

PW
Comment 17 Dani Megert CLA 2011-10-25 06:10:12 EDT
(In reply to comment #14)
> (In reply to comment #10)
> > Created attachment 182495 [details] [details]
> > Sample plug-in
> > This sample plug-in shows haw we can use the mediation mechanism. David, what
> > do you think?
> 
> We took a look at these plugins.  It seems to rely upon there being only one
> content type definition that sets its priority to high.  Once you have more
> than one set to high, it's once again a race to see which one wins.

No, that's not true: even if the nature declares affinity with a content type with priority "low", that content type wins over those with priority "high". It would only become a problem if the same nature would want to declare affinity with similar content types of the same priority.
Comment 18 Dani Megert CLA 2011-11-15 11:15:54 EST
The plan is to introduce a new extension point in 'org.eclipse.ui.ide' which will allow to contribute an EditorAssociationOverride. The contributed class will have to implement:

- overrideDefaultEditor(IEditorInput, IContentType, IEditorDescriptor): IEditorDescriptor
- overrideEditors(IEditorInput, IContentType, IEditorDescriptor[]): IEditorDescriptor[]

This will allow full control over the editors and the default editor. One could even return editor descriptors which are not part of the given input descriptors. We will allow more than one extension (though this is discouraged). In such a case we will call each of them in a sequence to get the final result.

This will be honored by the code in ui.ide, e.g. code opening editors, OpenWithAction and 'Open Resource'.
Comment 19 Dani Megert CLA 2011-11-30 09:05:11 EST
Created attachment 207721 [details]
Possible Fix

Interested parties please take a look at this patch and report back whether this fits your needs.

Note that it does not yet cover Platform Team which will need some minor changes to also honor the new extension point.
Comment 20 Rick Sawyer CLA 2012-01-12 13:00:43 EST
I have applied the patch (to Eclipse 3.6), and it works quite well.  I also used multiple extensions, which worked well too -- although in our case only one extension is valid for a particular IFile/Content type pair.

The only comments I have are:
1. The images used in the project explorer do not use this override code, so the icon does not indicate the actual editor that will get used
2. The user preferences for 'File Association' are not respected, so even if the user says which editor they want for a file extension, we will still override it.

For #1, it is not that big of a deal, as opening a file in a non-default editor with 'Open With' has the same behaviour, but it is a bit misleading.  
For #2, we can provide our own preference, similar to 'File Association' to override the override or turn it off.
Comment 21 Dani Megert CLA 2012-01-13 02:34:07 EST
(In reply to comment #20)
> 1. The images used in the project explorer do not use this override code, so
> the icon does not indicate the actual editor that will get used

The file icon will be correct if the 'File Icons Based On Content Analysis' decorator is enabled (default). This decoration happens in the background and is implemented in the IDE plug-in (org.eclipse.ui.internal.ide.ContentTypeDecorator), hence a good place to respect the override.
Comment 22 Dani Megert CLA 2012-01-17 11:34:04 EST
Fixed in org.eclipse.ui.ide:
    master: a1c2eb8bb623519caffd20f4a0d6378f76a08fc3
    R3_development: 9ab6c6b5d0837b2e28be0e4f001ae1aafe3320c2

And a little fix in team.ui (master): 465ec762a55e70fb743d4334d0150ed487c68cda
Comment 23 Dani Megert CLA 2012-01-17 11:42:28 EST
*** Bug 240405 has been marked as a duplicate of this bug. ***
Comment 24 Rick Sawyer CLA 2012-01-19 13:02:58 EST
(In reply to comment #21)
> (In reply to comment #20)
> > 1. The images used in the project explorer do not use this override code, so
> > the icon does not indicate the actual editor that will get used
> 
> The file icon will be correct if the 'File Icons Based On Content Analysis'
> decorator is enabled (default). This decoration happens in the background and
> is implemented in the IDE plug-in
> (org.eclipse.ui.internal.ide.ContentTypeDecorator), hence a good place to
> respect the override.

Does this mean that you are going to add code to org.eclipse.ui.internal.ide.ContentTypeDecorator to call the overriders to set the file icons?  Will this even work?  Because I see that the EditorRegistry is actually the one that looks up the IEditorDescriptor in this case.
Comment 25 Dani Megert CLA 2012-01-19 16:18:25 EST
> Does this mean that you are going to add code to
> org.eclipse.ui.internal.ide.ContentTypeDecorator 
yes. try - then ask ;-)
Comment 26 Dani Megert CLA 2012-01-25 17:33:48 EST
Verified in 4.2-I20120125-0100 and 3.8-I20120124-2000.