-
Notifications
You must be signed in to change notification settings - Fork 29.4k
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
Comments
Would be nice to see what branch incoming and current branches are. Maybe something like showing the branchname in parenthesis next to current/incoming, or showing the branchname on hover would be a good option? Edit: |
Feedback: Entry point: Preview merge: 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 Icon buttons are no longer icon buttons? 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? |
@Snailedlt I think that's already been included in the design 😊 |
@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: 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: 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? |
This comment was marked as resolved.
This comment was marked as resolved.
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 ? |
👍 , I think the requirements for the command line are captured in #5770 and we want to make sure that is supported. |
Love the direction @daviddossett 👏
fyi for editor @alexdima @hediet, for scm thoughts @lszomoru @joaomoreno |
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.
+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). |
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. 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. 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. |
Such a concept exists in Eclipse as "Structure Compare". I couldn't find a good screenshot, but the gist is: (source) |
Thanks for all the feedback so far! Responding to a handful of comments before jumping into some variations:
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.
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.
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 👍
Good idea. @hawkticehurst had the same feedback above. I agree—open to suggestions for how this setting could look if anyone has strong opinions.
Not yet but I'll take a look at these examples + others. @misolori
I'll get a new issue going for this.
This is on my radar to explore as another option this week 👍 |
@daviddossett I believe this is only used in Notebooks for the toolbar: |
👋 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 placementTitlebar with small primary buttonsOne 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: Fixed footer with primary buttonsAnother idea explores @misolori's suggestion to be consistent with the settings sync merge could yield something like this: Toolbar actionsOne other idea would be to simply use the toolbar for all three actions. Less discoverable, but no net new UI here: Using CodeLens actions instead of checkboxesThis 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.mp4Removing the title bar for each paneFollowing 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 buttonsUsing more specific CodeLens text |
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). |
What happens if the user manually edits the result view in a way that incoming changes can no longer be accepted/rejected automatically? |
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. |
@daviddossett great explorations, thanks a lot! 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. |
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. |
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. |
Just using title bar for I also agree with code lense in |
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. |
@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. |
Agreed. @daviddossett So, same concept, just different icons |
Another thing: This is almost exactly what Jetbrains does, so use that as an interactive example. Sidenote: |
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
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. |
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 |
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: 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: |
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. |
Checkboxes assume the lines you merge are independent. Which may be true for some formats, but nearly always false for code. |
Do you happen to have a real world example? A screen recording would be amazing! |
Consider a class file where you and your colleague added a new method each and modified one of the existing methods. |
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. |
Floating windows aren't here yet #10121 |
Dare to add a screenshot? |
Locking conversation for now. Thank you very much for your feedback. We will keep this issue updated once we have more details. |
Closing now that this is available as an experimental feature via Tracking specific issues with the merge-editor label going forward 👍 |
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")...
..and another via the SCM where there could be an icon button (icon here is just a placeholder) on the file that has conflicts:
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.
With selections
Here are two examples where selections have been made. One with a selections from different conflicts on each branch:
...and another showing the same changes being picked on both branches:
Feedback and ideas welcome!
cc @misolori @albertelo @lszomoru @rebornix
The text was updated successfully, but these errors were encountered: