Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFE] Add support for "editor" content types for compare editor / contentViewers extension #1747

Closed
iloveeclipse opened this issue Mar 12, 2024 · 6 comments · Fixed by eclipse-platform/eclipse.platform#1277 or #1800
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@iloveeclipse
Copy link
Member

Moved from eclipse/tm4e#730.

Currently textmate (and generic editor) based editors seem not support syntax highlighting in compare editor.

Example: in 4.31 Eclipse, set "Generic Text Editor" as default for XML content type, select two .project files in Package Explorer and via right click run "Compare With... -> Each Other" command.

That would show both parts of the editor without syntax highlighting:

image

That is of course not working for any other content type with textmate syntax support, XML here was chosen as it is simple to reproduce.

It would be nice if the syntax highlighting for all supported content types of generic editor also would be supported in compare editor.

During the discussion of this request, Mickael mentioned that:

Enabling (some) generic editor features in the compare viewer has to be declared per content-type, ideally by the same bundle as the one that defines the content-type association with generic editor. Eg from Wild Web Developer https://github.com/eclipse-wildwebdeveloper/wildwebdeveloper/blob/13c104d642e17156c7308ce43a78ccb04edcb55e/org.eclipse.wildwebdeveloper/plugin.xml#L1273C4-L1286C16

<extension
         point="org.eclipse.compare.contentViewers">
      <contentTypeBinding
            contentTypeId="org.eclipse.wildwebdeveloper.parent"
            contentViewerId="org.eclipse.ui.genericeditor.compareViewer">
      </contentTypeBinding>
   </extension>
   <extension
         point="org.eclipse.compare.contentMergeViewers">
      <contentTypeBinding
            contentMergeViewerId="org.eclipse.ui.genericeditor.compareViewer"
            contentTypeId="org.eclipse.wildwebdeveloper.parent">
      </contentTypeBinding>
   </extension>

and also:

Note that the generic editor isn't referenced directly here, it's a separate id/object that is assigned to the compare viewer, and there is no declarative mapping between this object and the underlying text editor.
So it's not trivial to infer the proper compare viewer from the editor association.
But if you think it's doable, that would indeed be an interesting improvement for Platform.

The problem we have is that editor associations could be manually updated by user, but contentViewers/contentMergeViewers cannot, even if that would technically work (like in generic editor case), because they share same underlying implementation.

So my proposal would be to extend contentViewers/contentMergeViewers extension point and add an attribute to consider "linked" editor content type associations. The code could check if contentViewers/contentMergeViewers contribution refers to such "linked" editor id in extension, and check if editor supports other content types. In case there are more content types supported by same "linked" editor, these would be automatically considered as valid contentTypeBinding for the contentViewers/contentMergeViewers.

This shouldn't be that complicated. From compatibility point of view, since the extra extension attribute would be not set by default, none of existing code would be affected. For the generic text editor that would mean to mark its contentViewers/contentMergeViewers contributions "linked" to "org.eclipse.ui.genericeditor.GenericEditor" and that's it. Any "specialized" editor versions of generic editor (with dedicated editor id) shouldn't be affected either.

Once I have some time, I would try to provide a PR for that idea, but any comments / contributions welcome.

@iloveeclipse iloveeclipse added enhancement New feature or request help wanted Extra attention is needed labels Mar 12, 2024
iloveeclipse added a commit to iloveeclipse/eclipse.platform that referenced this issue Mar 29, 2024
Extend `contentMergeViewers` extension point: add an attribute to
consider "linked" editor content type associations. The code checks if
contentMergeViewers contribution refers to such "linked" editor id in
extension, and check if editor supports other content types. In case
there are more content types supported by same "linked" editor, these
would be automatically considered as valid contentTypes for the
contentMergeViewers.

Updated CompareViewerSwitchingPane to consider more precise title
calculations for ContentMergeViewer instances (where concrete type info
is available after instantiation of a particular viewer).

Fixes eclipse-platform/eclipse.platform.ui#1747
iloveeclipse added a commit to iloveeclipse/eclipse.platform.ui that referenced this issue Mar 29, 2024
This allows GenericEditorMergeViewer be used by compare editor for
content types associated with the generic editor.

Additionally implemented getTitle() to provide more specific compare
editor description, so instead of default "Text Compare" one would see
"XML Compare" etc. This avoids the problem of duplicated "Text Compare"
options shown for different compare viewers (default one for plain text
and the one for generic editor).

See eclipse-platform#1747

See eclipse-platform/eclipse.platform#1277
@iloveeclipse
Copy link
Member Author

@iloveeclipse
Copy link
Member Author

@mickaelistria : would be nice if you could review the proposals.

iloveeclipse added a commit to iloveeclipse/eclipse.platform that referenced this issue Mar 31, 2024
Extend `contentMergeViewers` extension point: add an attribute to
consider "linked" editor content type associations. The code checks if
contentMergeViewers contribution refers to such "linked" editor id in
extension, and check if editor supports other content types. In case
there are more content types supported by same "linked" editor, these
would be automatically considered as valid contentTypes for the
contentMergeViewers.

Updated CompareViewerSwitchingPane to consider more precise title
calculations for ContentMergeViewer instances (where concrete type info
is available after instantiation of a particular viewer).

Fixes eclipse-platform/eclipse.platform.ui#1747
@iloveeclipse
Copy link
Member Author

@mickaelistria : would be nice if you could review the proposals.

Here is how it looks like on Windows, comparing .project or .classpath files with each other with installed Textmate and no extra configuration needed:

image

I would like to merge for M1, if there is any objections, please comment on PR's.

@iloveeclipse iloveeclipse removed the help wanted Extra attention is needed label Apr 2, 2024
@merks
Copy link
Contributor

merks commented Apr 2, 2024

If no one finds time to review, you should use your own best judgment and feel empowered to proceed.

iloveeclipse added a commit to iloveeclipse/eclipse.platform that referenced this issue Apr 3, 2024
Extend `contentMergeViewers` extension point: add an attribute to
consider "linked" editor content type associations. The code checks if
contentMergeViewers contribution refers to such "linked" editor id in
extension, and check if editor supports other content types. In case
there are more content types supported by same "linked" editor, these
would be automatically considered as valid contentTypes for the
contentMergeViewers.

Updated CompareViewerSwitchingPane to consider more precise title
calculations for ContentMergeViewer instances (where concrete type info
is available after instantiation of a particular viewer).

Fixes eclipse-platform/eclipse.platform.ui#1747
iloveeclipse added a commit to iloveeclipse/eclipse.platform that referenced this issue Apr 3, 2024
Extend `contentMergeViewers` extension point: add an attribute to
consider "linked" editor content type associations. The code checks if
contentMergeViewers contribution refers to such "linked" editor id in
extension, and check if editor supports other content types. In case
there are more content types supported by same "linked" editor, these
would be automatically considered as valid contentTypes for the
contentMergeViewers.

Updated CompareViewerSwitchingPane to consider more precise title
calculations for ContentMergeViewer instances (where concrete type info
is available after instantiation of a particular viewer).

Fixes eclipse-platform/eclipse.platform.ui#1747
iloveeclipse added a commit to eclipse-platform/eclipse.platform that referenced this issue Apr 3, 2024
Extend `contentMergeViewers` extension point: add an attribute to
consider "linked" editor content type associations. The code checks if
contentMergeViewers contribution refers to such "linked" editor id in
extension, and check if editor supports other content types. In case
there are more content types supported by same "linked" editor, these
would be automatically considered as valid contentTypes for the
contentMergeViewers.

Updated CompareViewerSwitchingPane to consider more precise title
calculations for ContentMergeViewer instances (where concrete type info
is available after instantiation of a particular viewer).

Fixes eclipse-platform/eclipse.platform.ui#1747
iloveeclipse added a commit that referenced this issue Apr 3, 2024
This allows GenericEditorMergeViewer be used by compare editor for
content types associated with the generic editor.

Additionally implemented getTitle() to provide more specific compare
editor description, so instead of default "Text Compare" one would see
"XML Compare" etc. This avoids the problem of duplicated "Text Compare"
options shown for different compare viewers (default one for plain text
and the one for generic editor).

See #1747

See eclipse-platform/eclipse.platform#1277
@iloveeclipse iloveeclipse self-assigned this Apr 3, 2024
@iloveeclipse iloveeclipse added this to the 4.32 M1 milestone Apr 3, 2024
iloveeclipse added a commit to iloveeclipse/eclipse.platform that referenced this issue Apr 4, 2024
TextMergeViewer was expected by SaveableCompareEditorInputTest but more
specific implementation (subclass of it) - GenericEditorMergeViewer was
seen by the test after changes made to allow more specific viewer to
"win" in the compare editor for text content.

See eclipse-platform/eclipse.platform.ui#1747
See eclipse-platform#1277
akurtakov pushed a commit to eclipse-platform/eclipse.platform that referenced this issue Apr 4, 2024
TextMergeViewer was expected by SaveableCompareEditorInputTest but more
specific implementation (subclass of it) - GenericEditorMergeViewer was
seen by the test after changes made to allow more specific viewer to
"win" in the compare editor for text content.

See eclipse-platform/eclipse.platform.ui#1747
See #1277
@iloveeclipse iloveeclipse modified the milestones: 4.32 M1, 4.32 M2 Apr 5, 2024
@iloveeclipse
Copy link
Member Author

I see that new feature only works in some cases, I've missed read-only diffs => need additional changes.

@iloveeclipse iloveeclipse reopened this Apr 5, 2024
iloveeclipse added a commit to iloveeclipse/eclipse.platform that referenced this issue Apr 8, 2024
Remember detected content type id in CompareConfiguration with the
CONTENT_TYPE key. This value can be picked up as a hint by
TextMergeViewer implementations (like GenericEditorMergeViewer) that
need to know which content type was detected by compare editor.

Most TextMergeViewer implementations don't need that hint because they
aren't generic and usually always know which content type they will have
- not so for GenericEditorMergeViewer which is generic.

The hint allows GenericEditorMergeViewer to know for which content type
was it actually created and so use that in cases where the content type
can't be always derived from the input (like data from git revisions
that don't have regular files or buffers associated to the viewer).

See eclipse-platform/eclipse.platform.ui#1747
iloveeclipse added a commit to iloveeclipse/eclipse.platform.ui that referenced this issue Apr 8, 2024
Detected content type id could be retrieved now from the
CompareConfiguration with the CONTENT_TYPE key (see
eclipse-platform/eclipse.platform#1294).

Use this value (if available) as fallbackContentTypes in
ExtensionBasedTextViewerConfiguration.

The allows GenericEditorMergeViewer to know for which content type
was it actually created and so use that in cases where the content type
can't be derived from the input (like data from git revisions
that don't have regular files or buffers associated to the viewer).

Fixes eclipse-platform#1747
iloveeclipse added a commit to eclipse-platform/eclipse.platform that referenced this issue Apr 8, 2024
Remember detected content type id in CompareConfiguration with the
CONTENT_TYPE key. This value can be picked up as a hint by
TextMergeViewer implementations (like GenericEditorMergeViewer) that
need to know which content type was detected by compare editor.

Most TextMergeViewer implementations don't need that hint because they
aren't generic and usually always know which content type they will have
- not so for GenericEditorMergeViewer which is generic.

The hint allows GenericEditorMergeViewer to know for which content type
was it actually created and so use that in cases where the content type
can't be always derived from the input (like data from git revisions
that don't have regular files or buffers associated to the viewer).

See eclipse-platform/eclipse.platform.ui#1747
iloveeclipse added a commit that referenced this issue Apr 9, 2024
Detected content type id could be retrieved now from the
CompareConfiguration with the CONTENT_TYPE key (see
eclipse-platform/eclipse.platform#1294).

Use this value (if available) as fallbackContentTypes in
ExtensionBasedTextViewerConfiguration.

The allows GenericEditorMergeViewer to know for which content type
was it actually created and so use that in cases where the content type
can't be derived from the input (like data from git revisions
that don't have regular files or buffers associated to the viewer).

Fixes #1747
@iloveeclipse
Copy link
Member Author

Ok, with latest changes merged I see this picture on latest SDK I20240409-1800 + latest Textmate installed (and no extra options used):
image

That works now for all file types supported by TextMate, both for files in the workspace or diffs coming from git (History view).

iloveeclipse added a commit to eclipse-platform/www.eclipse.org-eclipse that referenced this issue Apr 10, 2024
iloveeclipse added a commit to iloveeclipse/eclipse.platform that referenced this issue Jul 2, 2024
As noted on PR eclipse-platform#1277, the code looks weird and I can't explain what I
smoke while writing it. Fixed the code to do what actually was meant.
This is mostly paranoia code, as usually input.getName() is not null.

See eclipse-platform#1277
See eclipse-platform/eclipse.platform.ui#1747
iloveeclipse added a commit to eclipse-platform/eclipse.platform that referenced this issue Jul 2, 2024
As noted on PR #1277, the code looks weird and I can't explain what I
smoke while writing it. Fixed the code to do what actually was meant.
This is mostly paranoia code, as usually input.getName() is not null.

See #1277
See eclipse-platform/eclipse.platform.ui#1747
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment