feat: improved merge conflict solver #139
Conversation
|
I noticed that in this case the first line |
pkg/ui/conflict/conflict.go
Outdated
| case tea.KeyPgDown: | ||
| diffConflictCount := len(m.diff.GetUnifiedDiffConflictIndices()) | ||
| m.currentDiffIndex = min(diffConflictCount-1, m.currentDiffIndex+1) | ||
| m.viewport.YOffset = m.diff.GetUnifiedDiffConflictIndices()[m.currentDiffIndex] - 1 |
There was a problem hiding this comment.
GetUnifiedDiffConflictIndices returns zero-based line indices. So I'd like to just make sure that a possible -1 YOffset is ok and means something. I do not know the library.
pkg/ui/conflict/conflict.go
Outdated
| } else { | ||
| s.WriteString("keep") | ||
| s.WriteString("diff file written") |
There was a problem hiding this comment.
Just about the strings: should we use either present tense or past tense for all 3 options? I.e. "write diff file" here, if we would want to align to "use new" etc., or the other way around?
pkg/ui/conflict/conflict.go
Outdated
| @@ -66,16 +145,20 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { | |||
| m.submitted = true | |||
| return m, tea.Quit | |||
| case tea.KeyRight: | |||
| m.answer = true | |||
| m.resolution = min(m.resolution+1, 3) | |||
There was a problem hiding this comment.
I'm wondering if we should try to avoid hardcoding the numbers 3 here and also 1 on line 150 below. This might turn hard to manage if e.g. choices are added or removed. And also one way to sense that something might be off now is by noticing that the UseDiffFile constant is not used. We could replace this 3 with UseDiffFile and the 1 on line 150 with UseOld, but it still makes changes hard...
Should we maybe add something like var choices = [...]int{UseOld, UseNew, UseDiffFile} after the 3 constants at the top and then use this array to drive at least this navigation here, maybe some other similar place, too? Optimally we would then also refactor footerView to use this array to produce the resolutionSelector, for example. Ok, this might be too complicated all in all. Maybe we can make these changes if more options are ever added... not sure.
|
I think the first line of the diff might not be showing now - what Antti also noticed (at least). I added this brutal
The UI also feels as if there were more empty lines at the end than there actually are in the diff (can be arrowed down several lines more that where the file ends). This is not a show-stopper, but in some situations might be confusing. |
|
Also, should the
|
|
One more usability thing came to mind as I was testing (not a show stopper): currently Also it's not possible to PgDown to the first conflict in the file, because the field is already initialised to Maybe a solution would be to have Bit intricate and there might be a better way still... I'm just making this up now. It's maybe half bug half missing feature now. No strong opinions. |
majori
changed the title
Katajisto
force-pushed
the
feat/merge-conflict-solver
branch
from
d7cf914 to
9c15295
Compare
pkg/ui/conflict/conflict.go
Outdated
| oldStyle := lipgloss.NewStyle().Foreground(lipgloss.Color("#FF0000")) | ||
| newStyle := lipgloss.NewStyle().Foreground(lipgloss.Color("#00FF00")) |
There was a problem hiding this comment.
I wonder is the raw green and red best options here. For example there are "common" green and red used throughout the code
There was a problem hiding this comment.
I changed them to match these, I am not sure if it made it look better but consistency within the app is nice to have.
pkg/diff/diff_test.go
Outdated
| } | ||
|
|
||
| func readStringFromFile(name string) (string, error) { | ||
| fileBytes1, err := os.ReadFile(name) |
There was a problem hiding this comment.
FYI, you can use embed package to read files easily to a variable. It would work great in this case
There was a problem hiding this comment.
TIL, did not know about this package 👍 . I now rewrote this to use it with an FS. I'm not sure is that what you had in mind. This test is parametrised theoretically (although in the end there's only 1 test case now), so the completely static embedding to a string var would probably not work here, or I haven't figured out how. But this should be already a bit more compile time I guess, so probably better 👍 .
Let me know if you had some smarter way to use it in mind!
I've been sold ~100% in the meantime and I might reply with a delay & no longer be able to do larger changes.
|
Had a moment to read the code again and test - looks good! 👍 The only issue I found is that the yellow does not work so well with the light color scheme of the terminal. Maybe some shade of gray would work for both?
|
Katajisto
force-pushed
the
feat/merge-conflict-solver
branch
from
672700c to
fc057e2
Compare
No description provided.