Make "Compare with Clipboard" work in active editor of FormEditor

[brychcy]

e.g. m2e's MavenPomEditor or PDE's ManifestEditor

[vogella]

@SougandhS can you review?

[github-actions]

Test Results

 1 947 files  +  158   1 947 suites  +158   1h 32m 51s ⏱️ - 32m 30s
 4 720 tests ±    0   4 696 ✅ ±    0   24 💤 ±0  0 ❌ ±0 
14 160 runs  +1 117  13 993 ✅ +1 116  167 💤 +1  0 ❌ ±0 

Results for commit 9234bd4. ± Comparison against base commit c4f892a.

♻️ This comment has been updated with latest results.

[SougandhS]

Tested the changes & LGTM 👍
Thank you for the improvement

[tomaswolf]

Why only FormEditor and not MultiPageEditorPart in general?

[SougandhS]

Why only FormEditor and not MultiPageEditorPart in general?

I noticed thatMultiPageEditorPart.getActiveEditor()is a protected method, and since all comparison-related actions (including this one) extend from BaseCompareAction, we can't directly access it from here

[SougandhS]

@brychcy while you already fixed this issue for Compare, would it be possible to add the same change in org.eclipse.compare.internal.ClipboardReplace too ?

same code can be pasted before if (editor instanceof ITextEditor txtEditor) {

[brychcy]

I can, but actually may I ask why "Replace With > Clipboard" exists at alll?

At least on macOS I can just do "paste" (CMD-V) and the current selection will be replaced with the clipboard contents. If I want to replace the whole file I can easily do "select all" (CMD-A) first.

Also the "Clipboard" entry is now in the "Replace With"-submenu in between "Local History..." and "Previous from Local History" and the feels really misplaced there (actually everything else in that submenu is also history related, so if you really wanted to keep it, maybe move it to the end?)

So in the current state I think this makes normal usage of "Replace With"-options harder to use.
I think it should either be move it somewhere else or removed again (better in my Opinion, as I don't see a use case)

[SougandhS]

I can, but actually may I ask why "Replace With > Clipboard" exists at alll?

#1862 (comment)

Also the "Clipboard" entry is now in the "Replace With"-submenu in between "Local History..." and "Previous from Local History" and the feels really misplaced there (actually everything else in that submenu is also history related, so if you really wanted to keep it, maybe move it to the end?)

Yeah it can be moved to the end 👍

[brychcy]

Also there are still some issues with "Replace With... > Clipboard".

E.g. it directly changes content of "derived" files.
Normally there should be a question "This file is derived. Do you really want to edit it?"

[brychcy]

I can, but actually may I ask why "Replace With > Clipboard" exists at alll?

#1862 (comment)

I agree with that comment in that being able to compare files with the Clipboard is useful, but I disagree that adding "Replace" is a good idea

[SougandhS]

E.g. it directly changes content of "derived" files. Normally there should be a question "This file is derived. Do you really want to edit it?"

Will check that. thanks for noticing 👍

[brychcy]

Similar, "Replace with" doesn't check for "Locked"

[brychcy]

Also "Replace With" applied on files should have "Undo" support

[brychcy]

Anyways, thanks a lot for providing "Compare With..." Clipboard!
(that feature was actually one of the few reasons I sometimes launch IntelliJ)

[brychcy]

Anyways I added the code as requested to ClipboardReplace, too, so we can close this.

[SougandhS]

Anyways I added the code as requested to ClipboardReplace, too, so we can close this.

Sorry for the late reply, I was at lunch.
You can revert changes for Replace, I will push a fix including this change later

[SougandhS]

Anyways, thanks a lot for providing "Compare With..." Clipboard! (that feature was actually one of the few reasons I sometimes launch IntelliJ)

Thank you @brychcy

[SougandhS]

Anyways I added the code as requested to ClipboardReplace, too, so we can close this.

Sorry for the late reply, I was at lunch. You can revert changes for Replace, I will push a fix including this change later

Taking this back, you keep this commit. Thank you for this again

[SougandhS]

Hi @vogella
Can you merge this PR ?

[iloveeclipse]

Thanks everyone. Change looks good.

(that feature was actually one of the few reasons I sometimes launch IntelliJ)

The feature is part of AnyEdit since ~15 years or so :-)

[vogella]

Thanks @brychcy , nice to see you contributing again.

Thanks @SougandhS for the review

[tomaswolf]

Why only FormEditor and not MultiPageEditorPart in general?

I noticed thatMultiPageEditorPart.getActiveEditor()is a protected method, and since all comparison-related actions (including this one) extend from BaseCompareAction, we can't directly access it from here

if (editor instanceof MultiPageEditorPart mpe) {
  Object page = mpe.getSelectedPage();
  if (page instanceof IEditorPart e) {
    editor = e;
  }
}

MultiPageEditorPart.getSelectedPage() is public, and its contract is to return the editor part if the current page is an editor part.

[brychcy]

Interesting.
But then I wonder if there is a way to make "Compare With Clipboard" available to all Text Editors.
E.g. it currently doesn't work at all in the "Effective POM" tab of the Maven POM Editor, or in ClassFileEditor (when looking at the attached source code of .class files)

[tomaswolf]

Interesting. But then I wonder if there is a way to make "Compare With Clipboard" available to all Text Editors. E.g. it currently doesn't work at all in the "Effective POM" tab of the Maven POM Editor, or in ClassFileEditor (when looking at the attached source code of .class files)

There is. Andrey's AnyEditTools can do that, too.

[SougandhS]

MultiPageEditorPart.getSelectedPage() is public, and its contract is to return the editor part if the current page is an editor part.

Thanks for this, I have incorporated this change in #2007