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

Merge editor starts showing merge markers after closing file #151033

Closed
mjbvz opened this issue Jun 1, 2022 · 13 comments
Closed

Merge editor starts showing merge markers after closing file #151033

mjbvz opened this issue Jun 1, 2022 · 13 comments

Comments

@mjbvz
Copy link
Collaborator

mjbvz commented Jun 1, 2022

Testing #150389

  1. Open merge editor
  2. In another editor group, open the file being merged3.

Screen Shot 2022-06-01 at 2 36 16 PM

  1. Now close the file being merged (main.ts in my case)

Bug
The bottom part of the merge editor scrolls all the way to the bottom of the editor. When I scroll it back up, I see merge markers:

Screen Shot 2022-06-01 at 2 36 25 PM

@mjbvz
Copy link
Collaborator Author

mjbvz commented Jun 1, 2022

I also don't understand what the filled circles that replace the checkmarks mean

@hediet
Copy link
Member

hediet commented Jun 2, 2022

I think the bug is caused by how we integrate the merge editor into the workbench.
Hopefully this gets resolved after @jrieken, @bpasero and me meet next week.

I also don't understand what the filled circles that replace the checkmarks mean

This means that the user has changed the result in this area, but has not accepted either input 1 or input 2.

@hediet hediet added this to the June 2022 milestone Jun 2, 2022
@hediet hediet added bug Issue identified by VS Code Team member as probable bug merge-editor labels Jun 2, 2022
@bpasero
Copy link
Member

bpasero commented Jun 2, 2022

I cannot really reproduce the issue:
Recording 2022-06-02 at 15 19 54

Anyway: the "Result" of merge editor should be identical to the file on disk, meaning that edits in the one are reflected in the other editor, same as we do for diff editors.

@hediet
Copy link
Member

hediet commented Jun 2, 2022

Anyway: the "Result" of merge editor should be identical to the file on disk

Only if the editor is not dirty, right? In that case all editors should use the same text model.

@bpasero
Copy link
Member

bpasero commented Jun 2, 2022

I agree that opening an editor should not make it dirty right from the beginning, so I suggest to:

  • always have the result be the actual file on disk
  • never change the file when you open the editor
  • find a clever way how to hide the merge markers so that resolving the merge conflict is still nice without having to remove the markers and thus turn the document dirty

I am not sure whether this is possible or not?

@hediet
Copy link
Member

hediet commented Jun 2, 2022

I like this! Let's try to collapse merge markers, so they take only a single line.

@bpasero
Copy link
Member

bpasero commented Jun 3, 2022

One thing I am not entirely sure: even when we remove merge markers, the file would still contain both conflicting changes one above the other. Maybe the merge editor initially should reflect this by showing both checkboxes checked in that case and then as a user, only when I click a checkbox, the editor becomes dirty and changes are removed?

Edit: interesting, so VS also opens the file dirty right from the beginning, maybe they have gone through the thought process and didnt find a better solution?

image

@jrieken
Copy link
Member

jrieken commented Jun 7, 2022

One thing I am not entirely sure: even when we remove merge markers, the file would still contain both conflicting changes one above the other.

Yeah, that's the issue. The merge markers and the inlined versions should be understood as a "poor man's" merge editor. IMO keeping that in a "UI-driven" merge editor adds more confusion than help. It's also entirely unclear who tells us what part of a file is "merge marker noise".

I do understand the confusion about the editor opening as dirty. We could not make any changes during open, just be a different display-mode for how the file is on disk (e.g the results pane wouldn't show the file as on disk). Once the user accepts changes make the file dirt, maybe have a user-setting to auto-apply non-conflicting changes (which means dirty on open again)

@bpasero
Copy link
Member

bpasero commented Jun 7, 2022

Btw I believe in VS case, the file is actually in some temp dir, so you may not be editing the actual file, but some copy.

@jrieken
Copy link
Member

jrieken commented Jun 7, 2022

Yeah, that's something we have thought about and maybe not discussed enough yet. The downside of a temp/virtual file is that language services likely won't handle them as good as files on disk

@bpasero
Copy link
Member

bpasero commented Jun 7, 2022

Yeah wasn't suggesting to go down that path, I think all advantages when the real file on disk is edited 👍

@jrieken
Copy link
Member

jrieken commented Jun 24, 2022

I believe this issue will go away when we implement #151024. The plan for that is to show a modal message when you are closing the merge editor while still having unresolved conflicts

@jrieken
Copy link
Member

jrieken commented Jun 27, 2022

Closing since we now have the explicit warning about unhandled conflicts on close (#151024)

@jrieken jrieken closed this as completed Jun 27, 2022
@jrieken jrieken removed the bug Issue identified by VS Code Team member as probable bug label Jun 27, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Aug 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants