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

Explore UX for three-way merge #146091

Closed
daviddossett opened this issue Mar 26, 2022 · 42 comments
Closed

Explore UX for three-way merge #146091

daviddossett opened this issue Mar 26, 2022 · 42 comments
Assignees
Labels
merge-conflict Merge conflict decorations and actions merge-editor ux User experience issues
Milestone

Comments

@daviddossett
Copy link
Contributor

daviddossett commented Mar 26, 2022

We're exploring what it could look like to enable a three-way merge workflow in VS Code. Here are some initial ideas to get the ball rolling 👍

Prototype

Here's a brief walkthrough where I set up a simple merge conflict and resolve it with a three-way merge editor:

three-way-merge-walkthrough-converted.2.mp4

Main views

Here are some quick screenshots from the prototype above:

Entry point

I've been exploring the idea of having multiple entry points from a merge conflict state. One is via the existing codelens actions ("Preview Merge")...

entry-point-codelens

..and another via the SCM where there could be an icon button (icon here is just a placeholder) on the file that has conflicts:

entry-point-scm

Preview merge

Here's what the merge editor preview could look like. Here it features a mixed layout with the branches on top and the resulting code below. The branch editors are read-only while the result editor is editable.

default-state

With selections

Here are two examples where selections have been made. One with a selections from different conflicts on each branch:

with-selections

...and another showing the same changes being picked on both branches:

accept-both-changes

Feedback and ideas welcome!

cc @misolori @albertelo @lszomoru @rebornix

@daviddossett daviddossett added ux User experience issues merge-conflict Merge conflict decorations and actions labels Mar 26, 2022
@daviddossett daviddossett added this to the April 2022 milestone Mar 26, 2022
@Snailedlt
Copy link

Snailedlt commented Mar 26, 2022

Would be nice to see what branch incoming and current branches are.
I always struggle to remember which one is incoming and whoch one is current.

Maybe something like showing the branchname in parenthesis next to current/incoming, or showing the branchname on hover would be a good option?

Edit:
Really looking forward to this btw! If the implementation is good I'll fully transition over to vscode for most of my projects!

@hawkticehurst
Copy link
Member

hawkticehurst commented Mar 26, 2022

Feedback:

Entry point:
Why not have both? I personally question if I would have noticed either of the entry points that you demonstrated (or at least I think it would've taken me a while to notice), so having multiple opportunities to discover the new feature feels better to me than aligning on one of them.

Preview merge:
I love the mixed layout view, but more generally think that offering multiple layouts is the way to go.

One potential nit is the "inconsistency" of panel order between the horizontal and vertical layouts––I was expecting the results tab to be in the middle of the horizontal layout based on you demoing the vertical layout first. With that said, I could kind of see how you were maybe trying to align the mixed layout and horizontal layouts by having the results on the bottom?

I don't think I have a strong opinion about what the horizontal/vertical layout order should be (since again I can see myself almost exclusively using the mixed layout) but probably worth discussing this a bit more/getting community feedback on. Or maybe even giving the option to let people adjust the vertical/horizontal layout order to suit their preferences?

With selections:

I actually really like the usage of checkboxes as a mechanism for selecting lines of code (iirc this is not done elsewhere in VS Code right?). I personally think it's a really clear UI/UX model.

To offer another suggestion that came to mind while watching this: a visually cleaner (but potentially less intuitive) model could be to remove the checkboxes and simply click on the highlighted lines of code you want to choose from the incoming and current panels (since they are read-only). I almost imagine it as a sort of toggle button-esque experience since you've already designed a checked (higher opacity colors/text) and unchecked (lower opacity colors/text) state for the lines of code themselves.

I still think I prefer the usage of checkboxes though.

Misc:

Setting to make this a default
I love what you've presented so much that I can easily see myself wanting this to be the default way I manage merge conflicts in VS Code (if it's implemented), so I just want to toss in a formal request that a setting to make three-way merge the default view when managing merge conflicts be added as well. Please and thank you 🙏😊

Icon buttons are no longer icon buttons?
More of a concern for future webview UI toolkit work, but I just wanted to make sure that there was an intentional decision to break the conventions of the icon button to include text in the top right corner of each panel?

I personally really like how it looks (compared to using a secondary button for example) and think the addition of text is a really important and useful way of distinguishing between the actions of each panel. But again just want to make sure this an intentional shift in how the icon button style is used?

@hawkticehurst
Copy link
Member

Would be nice to see what branch incoming and current branches are.

@Snailedlt I think that's already been included in the design 😊

three-way-merge

@bpasero
Copy link
Member

bpasero commented Mar 26, 2022

@daviddossett really like it and seems to pretty much match Visual Studio in terms of UX which I felt was already a great UI when I had used it.

Can we also make sure to get VS behaviour of auto-picking every change that can be merged without conflicts, such as in this example where a change is selected for getting applied automatically since it does not result in conflicts:

image

I think another entry point could be a prominent action in the editor toolbar to open the merge editor. Nevertheless, I think all the entry points you suggest work for me. We probably also want a setting to let the user choose to always open the merge editor by default when opening a file with merge markers. I personally would not want to always having to click on the entry point to get into this mode, I would want it always.

Have we thought about how to represent a change in a line? Throwing in some ideas from others:

image

image

The reason I bring this up is #25887, where people ask for a way to take changes from left or right even when just comparing 2 files. I think if we solve this for the merge editor, the same solution could be used for diff editors too?

@Snailedlt

This comment was marked as resolved.

@flumacs
Copy link

flumacs commented Mar 28, 2022

I think another entry point could be a prominent action in the editor toolbar to open the merge editor. Nevertheless, I think all the entry points you suggest work for me. We probably also want a setting to let the user choose to always open the merge editor by default when opening a file with merge markers. I personally would not want to always having to click on the entry point to get into this mode, I would want it always.

similarly to what @bpasero said, would it be possible to open this way all the conflicting files - by default - with a simple command within the CLI ?

@bpasero
Copy link
Member

bpasero commented Mar 28, 2022

👍 , I think the requirements for the command line are captured in #5770 and we want to make sure that is supported.

@Zerotask
Copy link

Zerotask commented Mar 28, 2022

It would be great to see how many remaining conflicts exists (and how many got resolved).
By clicking on the "x Conflicts remaining" it would jump to the next conflict. This can be especially useful for large files.

Example:

a

@isidorn
Copy link
Contributor

isidorn commented Mar 28, 2022

Love the direction @daviddossett 👏
Some feedback I already shared, just to repeat here for transparency:

  • You are suggesting that we introduce a new editor title area. I think this works for this use case, not sure if we could get away without this?

fyi for editor @alexdima @hediet, for scm thoughts @lszomoru @joaomoreno

@miguelsolorio
Copy link
Contributor

Overall, really digging the design and direction! I do think we'll need to have a separate exploration around improving the codelenses for the conflicts as the multiple actions can be hard to comprehend quickly.

I just wanted to make sure that there was an intentional decision to break the conventions of the icon button to include text in the top right corner of each panel?

+1 to this thought, we also already use a "Merge" button action for settings sync so we should make both of these button styles consistent either way we go. I'm more inclined to go with button style to avoid introducing another style (though it is used in some areas like the notebook toolbar).

CleanShot 2022-03-28 at 11 20 48@2x

@Snailedlt
Copy link

+1 to this thought, we also already use a "Merge" button action for settings sync so we should make both of these button styles consistent either way we go. I'm more inclined to go with button style to avoid introducing another style (though it is used in some areas like the notebook toolbar).

CleanShot 2022-03-28 at 11 20 48@2x

I'm more for the option shown in the preview, where the "take all incoming" prompt is on the topbar. I feel it's a more natural place for it to be. It's not as intrusive ad the bright blue "pop up" buttons, which takes focus away from, and covers up some of the code.
The pop-up button style is more useful in cases where you need to react to something unusual, or something that happens then and there. Since it covers up the code, I like to think of it as a "notification.

Meanwhile the topbar button is more useful when it's a recurring choice, that happens every time (which is the case for merge conflicts, where you always want to get the option to accept all changes).

TLDR: Topbar button is better in this case since it's less intrusive, while the bright blue pop-up button is better for notification type prompts.

@hediet
Copy link
Member

hediet commented Mar 29, 2022

One problem I often have with merge conflicts is to understand the changes on the right and on the left individually before trying to combine them.
For simple renames this is easy to figure out, but for reorderings of members or complex refactorings it is not so obvious.

I think it could be helpful to show more details of the commits between merge-base and the respective branch (maybe even filtering out those commits that did not touch the file).

Maybe the most recent commit message + author could be shown next to the branch name. More commits could be indicated by three dots.
Clicking on such a commit could show the diff between that commit and the merge-base.

I wonder if we could innovate here a bit by trying to figure out what the text edit is (A rename of a word? A reordering of some lines?) to help the user to apply the effect of one side to the other.

@bpasero
Copy link
Member

bpasero commented Mar 29, 2022

Such a concept exists in Eclipse as "Structure Compare". I couldn't find a good screenshot, but the gist is:

image

(source)

@daviddossett
Copy link
Contributor Author

Thanks for all the feedback so far! Responding to a handful of comments before jumping into some variations:

@hawkticehurst

To offer another suggestion that came to mind while watching this: a visually cleaner (but potentially less intuitive) model could be to remove the checkboxes and simply click on the highlighted lines of code you want to choose from the incoming and current panels (since they are read-only).

A similar suggestion came up to use the code lens actions above each change to avoid a new pattern. Each change could have an action that flips the state. A bit clunky perhaps, but one less thing to learn.

Icon buttons are no longer icon buttons?

This is actually using the icon button component from our design toolkit. We have a variant for an icon w/ label. @misolori are these used anywhere in VS Code? I swear I'd seen them but I can't find any from a quick dig around. In any case I'll explore some other variations here—see comments below re: settings sync.

CleanShot 2022-03-30 at 16 03 20@2x

@bpasero

Can we also make sure to get VS behaviour of auto-picking every change that can be merged without conflicts, such as in this example where a change is selected for getting applied automatically since it does not result in conflicts:

I didn't actually realize they included changes that didn't result in conflicts. Good to know. It makes this more of a general diff merge workflow rather than dealing specifically with conflicts 👍

I think another entry point could be a prominent action in the editor toolbar to open the merge editor. Nevertheless, I think all the entry points you suggest work for me. We probably also want a setting to let the user choose to always open the merge editor by default when opening a file with merge markers. I personally would not want to always having to click on the entry point to get into this mode, I would want it always.

Good idea. @hawkticehurst had the same feedback above. I agree—open to suggestions for how this setting could look if anyone has strong opinions.

Have we thought about how to represent a change in a line?

Not yet but I'll take a look at these examples + others.

@misolori

I do think we'll need to have a separate exploration around improving the codelenses for the conflicts as the multiple actions can be hard to comprehend quickly.

I'll get a new issue going for this.

+1 to this thought, we also already use a "Merge" button action for settings sync so we should make both of these button styles consistent either way we go. I'm more inclined to go with button style to avoid introducing another style (though it is used in some areas like the notebook toolbar

This is on my radar to explore as another option this week 👍

@miguelsolorio
Copy link
Contributor

@daviddossett I believe this is only used in Notebooks for the toolbar:

@daviddossett
Copy link
Contributor Author

daviddossett commented Apr 1, 2022

👋 Following up with some variations based on some of the feedback/suggestions above. I'm sure there will be more to come later as there are other interesting ideas above that I haven't addressed yet.

Alternate button styles and placement

Titlebar with small primary buttons

One option would be to use a small primary button as found in the extensions list view. While this idea of a new toolbar for each pane in the editor is still new, there would at least be some connection to other buttons in VS Code compared to the initial idea of the icon w/ label:

pane-button-small-primary

Fixed footer with primary buttons

Another idea explores @misolori's suggestion to be consistent with the settings sync merge could yield something like this:

pane-button-fixed-footer

Toolbar actions

One other idea would be to simply use the toolbar for all three actions. Less discoverable, but no net new UI here:

CleanShot 2022-04-01 at 12 16 09@2x

Using CodeLens actions instead of checkboxes

This examples reuses the UI and interaction people are used to in our existing merge conflicts UI for picking changes. It's slightly different in that the actions act as toggles in this case.

codelens.mp4

Removing the title bar for each pane

Following up on @isidorn's suggestion, we could try not using a title bar for each pane and instead rely on other cues to indicate which pane is the incoming, current, and result. I think these are somewhat difficult to understand, but it's worth thinking more about how we could avoid introducing a new split view w/ title bar concept.

Both options lack context re: branch name which makes it especially hard to know what's happening. Regardless, some jumping off points could look like:

Using the hints from the buttons

no-titlebar-checkboxes-buttons

Using more specific CodeLens text

no-titlebar-codelens

@hediet
Copy link
Member

hediet commented Apr 4, 2022

Personally, I'd prefer to not add too much noise to the source code. The colors are already quite noisy and I already find it difficult to not get lost in the changes. If there is a good design without inline code-lenses, I'd go for that (such as "Titlebar with small primary buttons", which imo optimizes for productivity).

@hediet
Copy link
Member

hediet commented Apr 4, 2022

What happens if the user manually edits the result view in a way that incoming changes can no longer be accepted/rejected automatically?
(E.g. when the entire result is deleted)

@Snailedlt
Copy link

Would it be possible to have the option to show the three windows side-by-side similar to what Jetbrains IDE's do?

That makes it easier to see what line the changes affect without looking up and down at the line numbers. It also gives us the opportunity to distinguish the two changes from eachother by saying "left" and "right" instead of the commit-hash, which is a bit easier to understand for beginners.
I also think the way "accept changes", and "decline changes" is handled with arrow and x symbols instead of with text is quite intuitive.

Picture for reference:
merge-conflict

@isidorn
Copy link
Contributor

isidorn commented Apr 4, 2022

@daviddossett great explorations, thanks a lot!
I think this convinced me that we can not get away without introducing one title area per merge editor.

As for the way to accept changes - I agree with @hediet we should not add noise to the source code. And based on that moto I prefer the checkboxes in the glyph area on the left side.

@joaomoreno
Copy link
Member

I think this convinced me that we can not get away without introducing one title area per merge editor.

I really dislike having a title area, cause itself is a container in a hiearchy of containers with title areas. We could have an alternative affordance which, like the huge buttons in bottom right, floats in that corner of the editor but is a toolbar which contains small action icons. Precedence: the debug toolbar is floating around.

@hediet
Copy link
Member

hediet commented Apr 4, 2022

Precedence: the debug toolbar is floating around.

I'm not a huge fan of the floating debug toolbar to be honest - it always covers something. I don't know if we should use that as precedence.

@byehack
Copy link

byehack commented Apr 4, 2022

I think this convinced me that we can not get away without introducing one title area per merge editor.

I really dislike having a title area, cause itself is a container in a hiearchy of containers with title areas. We could have an alternative affordance which, like the huge buttons in bottom right, floats in that corner of the editor but is a toolbar which contains small action icons. Precedence: the debug toolbar is floating around.

Just using title bar for Result editor.
[Accept | Take All] Incoming/Current can go in Result title bar. (I think it has enough space in Mixed Layout mode)

image

I also agree with code lense in Result editor. so we can jump to Incoming/Current editor by clicking on code lenses in result editor.

@miguelsolorio
Copy link
Contributor

We also utilize the tab title in regular diffs as well, as an additional area to surface information:

CleanShot 2022-04-04 at 09 28 40@2x

@daviddossett
Copy link
Contributor Author

We also utilize the tab title in regular diffs as well, as an additional area to surface information

I had thought about something like that. One challenge would be not knowing that the bottom pane is the result until you have sort of seen the effect of taking/discarding changes. Open to other ideas here though.

CleanShot 2022-04-04 at 20 58 10@2x

@daviddossett
Copy link
Contributor Author

Would it be possible to have the option to show the three windows side-by-side similar to what Jetbrains IDE's do?

@Snailedlt that layout option is featured in the walkthrough video above if I'm not mistaken—added an example below.

Given that this model accounts for different layout modes, I'm not sure how the Jetbrains example would work on something like the mixed layout (incoming and current in two columns on the top row, result in one row below) that we've generally been discussing here. Let me know if you have any ideas on how that might work.

a

@abhijit-chikane
Copy link
Contributor

abhijit-chikane commented Apr 5, 2022

well in vertical layout I think we can just show the
image

this arrow

and if we implement the Jet brains like view I don't think we will ever think of using any other layout

@Snailedlt
Copy link

Snailedlt commented Apr 5, 2022

well in vertical layout I think we can just show the
image

this arrow

and if we implement the Jet brains like view I don't think we will ever think of using any other layout

Agreed.
For an even more generic icon, we can just use a checkmark (✅) for accepting the changes, and an ❌ for declining them.

@daviddossett So, same concept, just different icons

@Snailedlt
Copy link

Snailedlt commented Apr 5, 2022

Another thing:
When selecting a change in for example the current pane, I'd like for that change to go unmarked in the current pane. Again, this is like what Jetbrains does. It makes it easier to see which changes you've already handled, and gives a less cluttered view. Also if we could see when all conflicts have been handled that would be great! Maybe by reflecting the state of the "Accept Merge" button (change color and/or text), and also show a popup asking you if you're sure you want to sccept the merge if you've selected none of the changes for a conflict.

This is almost exactly what Jetbrains does, so use that as an interactive example.

Sidenote:
The "accept merge" button should be more visible imo, atleast when all the conflicts are solved, so as to indicate that it's the next step in solving the conflict. Right now it's not obvious in which order to press the buttons for absolute beginners.

@cadsuara
Copy link

cadsuara commented Apr 5, 2022

This improvement looks fantastic!! However, I think it could be even better. For instance adding support to something like the default layout in the vimdiff git mergetool option. That option shows 'LOCAL', 'BASE', 'REMOTE' and 'MERGED' files

vimdiff
			Opens vim with a 4 windows layout distributed in the following way:
 
			    ------------------------------------------
			    |             |           |              |
			    |   LOCAL     |   BASE    |   REMOTE     |
			    |             |           |              |
			    ------------------------------------------
			    |                                        |
			    |                MERGED                  |
			    |                                        |
			    ------------------------------------------
 
			"LOCAL", "BASE" and "REMOTE" are read-only buffers showing the contents of the
			conflicting file in a specific git commit ("commit you are merging into",
			"common ancestor commit" and "commit you are merging from" respectively)

			"MERGED" is a writable buffer where you have to resolve the conflicts (using the
			other read-only buffers as a reference). Once you are done, save and exit "vim"
			as usual (":wq") or, if you want to abort, exit using ":cq".

Check also this blog

An even better implementation that will require more development effort is to follow the same approach as in this change being submitted to the git project. The proposal is to have a variable describing the layout, using some default value that is 'comfortable' for everybody.

@AndersMad
Copy link

AndersMad commented Apr 5, 2022

as @cadsuara wrote - here with another example of an excellent compare tool (Beyond Compare) that has 3+1: https://www.scootersoftware.com/features.php - see https://www.scootersoftware.com/images/TextMerge.png

@jairbubbles
Copy link
Contributor

jairbubbles commented Apr 14, 2022

Hi @daviddossett, I really like the direction, good job!

I would personally vote for the checkboxes (like in Visual Studio) it's just so easier to understand! In many tools you have custom buttons which are not standard, for instance in Araxis Merge:
image
For the "Take All" / "Take Incoming", a checkbox which reflects the global state (all selected / some selected / none selected) could be a nice addition.

Having the branch names to "current" / "incoming" is also super cool but I would like to emphasize that given the context the logic would be inverted. For instance when you do a merge it's quite understandable as "current" is indeed the checked out branch but when you do a rebase it's inverted. "current" will be the branch on which you're rebasing and your checked out branch will be on the right panel. It's very disturbing for people who are not very fluent with git. I believe most users would always their current branch to be on the left. In some tools, they use "Left" / "Right" so that i's universally right.

As for displaying "base" (in a 3 + 1 layout) I don't think it should be the default but there should be a way to display it. I would like to point out how it was resolved in Sublime Merge by allowing to toggle the result view with the base view:

image

@bwateratmsft
Copy link
Contributor

bwateratmsft commented Apr 14, 2022

I spoke to @isidorn about this offline, but I'd really like for this feature to work for any diff, not just merge conflicts. I think if that was done, it would basically solve #25887 as well. Plus, it would seriously make my day.

@Rchatru
Copy link

Rchatru commented Apr 17, 2022

I spoke to @isidorn about this offline, but I'd really like for this feature to work for any diff, not just merge conflicts. I think if that was done, it would basically solve #25887 as well. Plus, it would seriously make my day.

I was also glad to see this issue in the roadmap, I think it can be a good way to unblock the famous and claimed #25887, you can't imagine how many people are waiting for this feature.

@AnrDaemon
Copy link

Checkboxes assume the lines you merge are independent. Which may be true for some formats, but nearly always false for code.
The Meld's way (for example) of expressing operations is way more clear.
Also, read-only source editors are questionable. While most of the time I find them sufficient, for some problematic conflicts I'd like to edit the source to avoid seeing insignificant conflicts and get a clearer view on more important areas.

@hediet
Copy link
Member

hediet commented Apr 19, 2022

While most of the time I find them sufficient, for some problematic conflicts I'd like to edit the source to avoid seeing insignificant conflicts and get a clearer view on more important areas.

Do you happen to have a real world example? A screen recording would be amazing!

@AnrDaemon
Copy link

Consider a class file where you and your colleague added a new method each and modified one of the existing methods.
The new methods are clear additions, and the modified method is a conflicting change.
Merging all changes into common version would leave change marks in 3 places on each source panel. While actually only one change is significant.

@JuanmaMenendez
Copy link

We need much space as possible, will be better if we open the conflict solving view in a Modal that covers the full screen (or most of it 80%) and set the "column layout" mode by default as this is the mode clearest to understand by far.

The merge conflict window of WebStorm is perfect, we should use it as a close reference.

@IllusionMH
Copy link
Contributor

Floating windows aren't here yet #10121

@AnrDaemon
Copy link

merge conflict window of WebStorm

Dare to add a screenshot?

@microsoft microsoft locked as off-topic and limited conversation to collaborators May 5, 2022
@isidorn
Copy link
Contributor

isidorn commented May 5, 2022

Locking conversation for now. Thank you very much for your feedback. We will keep this issue updated once we have more details.

@daviddossett daviddossett modified the milestones: May 2022, June 2022 Jun 1, 2022
@daviddossett
Copy link
Contributor Author

Closing now that this is available as an experimental feature via "git.mergeEditor": true.

Tracking specific issues with the merge-editor label going forward 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
merge-conflict Merge conflict decorations and actions merge-editor ux User experience issues
Projects
Status: 💻 In Development
Development

No branches or pull requests