codemirror / dev

Development repository for the CodeMirror editor project
https://codemirror.net/
Other
5.96k stars 377 forks source link

Unified merge view - improve what text is in cm-deletedText #1376

Closed ondrejmirtes closed 4 months ago

ondrejmirtes commented 6 months ago

Describe the issue

Hello, I'm really grateful that CM6 includes unifiedMergeView. When I started a project that needs it, I had no idea whether I'll have to spend several days implementing my own (inferior) solution, but fortunately I found @codemirror/merge.

I have a suggestion: in the diff like this:

Screenshot 2024-04-30 at 9 46 12

I'd like if -next-line part of the deleted line was inside cm-deletedText so that it'd also be highlighted similarly to identical.alwaysFalse, booleanAnd.alwaysFalse part of the added line.

This is currently how the DOM looks like:

<div class="cm-deletedChunk" contenteditable="false"><del>              /** @phpstan-ignore-next-line */</del><span class=" cm-deletedText"></span></div>
<div class="cm-changedLine cm-line"><ins class="cm-insertedLine">               <span class="ͼy">/** @phpstan-ignore</span><span class="cm-changedText"><span class="ͼy"> identical.alwaysFalse, booleanAnd.alwaysFalse</span></span><span class="ͼy"> */</span></ins></div>

I'd prefer if it looked something like this:

<div class="cm-deletedChunk" contenteditable="false"><del>              <span class="ͼy">/** @phpstan-ignore</span><span class=" cm-deletedText"><span class="ͼy">-next-line</span></span><span class="ͼy"> */</span></del></span></div>
<div class="cm-changedLine cm-line"><ins class="cm-insertedLine">               <span class="ͼy">/** @phpstan-ignore</span><span class="cm-changedText"><span class="ͼy"> identical.alwaysFalse, booleanAnd.alwaysFalse</span></span><span class="ͼy"> */</span></ins></div>

Thank you for considering this.

Browser and platform

No response

Reproduction link

No response

marijnh commented 6 months ago

Deleted text is highlighted. I'm not sure how that screenshot was made, it seems to use different styling than the default (compare). Maybe some mistake was made in the custom styles?

ondrejmirtes commented 6 months ago

Yeah, I have custom styles but without them it still doesn't work. It can't because the current DOM is not structured for that.

This is how it looks with the default styles:

Screenshot 2024-04-30 at 11 04 49

marijnh commented 6 months ago

Oh, right, unified view, I was looking at the split merge view. That indeed doesn't seem to render changed spans separately.

ondrejmirtes commented 6 months ago

Here's a reproduction on /try/.

ondrejmirtes commented 6 months ago

One unrelated question: how can I expect getChunks(state) to look in the unified merge view? Do they always have side set to 'b'? It seems like that from my testing (I don't know what text should I produce in order to have some chunks on side 'a'). Thanks.

marijnh commented 6 months ago

side is a per-editor thing that tells you which side of the chunks represent the document in that editor. In a split merge view both editors have a different side. In a unified view the editor is always b, and a refers to the given original document.

DaRosenberg commented 4 months ago

I have the same issue. An example:

image

I expect only the words always and Custom (on line 3 and 7 respectively) to be crossed over, not the whole lines.

image

@marijnh Would you say there's a good chance of this getting fixed in the near future? Obviously it's not something we can demand of you, it would just be helpful to know, to decide if we should consider Monaco instead, because I think this issue would be a blocker for our scenario unfortunately.

marijnh commented 4 months ago

This was supposed to already work, but the highlighting code was a bit confused. Attached patch should help.

DaRosenberg commented 4 months ago

It does indeed, works in 6.6.4 🎉 - thank you!