added support for multiple diffs (up to 8) #27
Conversation
|
At a first glance, it looks really good! I still have to test it out carefully and maybe think how the documentation will change, but we'll see. If you can keep using this branch and make modifications as you find issues, this would be a good exercise for it. In the meantime, I'll see what I can do about adding some automated tests in master. |
|
Thank you for your feedback. I checked LinediffMerge & LinediffPick now, and made them work in the last commit. In general, I'm using my branch without issues so far, but I have to admit that I'm not using it too extensively at the moment. A few automated testcases would definitely be nice! |
|
I've added some automated tests. If you could merge master in this branch, Travis should let us know if your branch continues to work for the existing use cases (provided I've written enough tests to cover them, of course, but even if not, at least it'll cover some of the basics). |
- use LinediffAdd for each Block, LinediffLast for the last one OR use LinediffShow AFTER the last block - max number of diff blocks = 8 is hardcoded, because it's the max number of diff buffers in the newest vim, version 8 - older versions support only 4 diff buffers, no check is done for that they just throw an error if you try to diff more than 4 blocks - LinediffMerge *should* work, not tested yet at all!! - in general not very well tested yet, use with caution!
hcgraf
force-pushed
the
multiple-diffs
branch
from
2b65834 to
30592a3
Compare
|
OK it fails :) |
|
Hm, that's very strange. Locally, I checked out this branch, and the build seems to run, not sure what's happening on the build server. Might be an issue with the Vim version. I'll try to debug it tomorrow, or over the weekend. I compile Vim from source, so I can check out that particular version and see what's happening. |
|
This is now a nice new feature, LinediffMerge produces 3 diff-splits (only) when the common ancestor marker Editing and LinediffPick will of course work in all of the three splits. The tests are still failing, though :( |
|
I think I figured it out, sort of. This is a problem I used to have a long time ago, when I was trying to implement auto-LinediffReset upon closing any one of the special buffers. Turns out, Vim isn't happy about trying to close an already-closed buffer. That's why I had this code: " The closing logic is a bit roundabout, since changing a buffer in a
" BufUnload autocommand doesn't seem to work.
"
" The process is: if a window is entered after the other differ was destroyed,
" destroy this one as well and close the window.
" .. .snip ...
call s:differ_one.CreateDiffBuffer(g:linediff_first_buffer_command)
autocmd BufUnload <buffer> silent call s:differ_one.Reset()
autocmd WinEnter <buffer> if s:differ_two.IsBlank() | silent call s:differ_one.CloseAndReset(0) | endif
call s:differ_two.CreateDiffBuffer(g:linediff_second_buffer_command)
autocmd BufUnload <buffer> silent call s:differ_two.Reset()
autocmd WinEnter <buffer> if s:differ_one.IsBlank() | silent call s:differ_two.CloseAndReset(0) | endifSo, if one buffer is unloaded, its data structure is reset, but the other ones get reset when the cursor ends up in them, so it happens after the autocommand is done. To reproduce, it should be enough to This doesn't seem to be a problem in a recent enough Vim version, but Travis uses some sensible Debian, so I'd rather not break the plugin for it -- it's likely that a lot of people use that version. One solution I can think of is to extract some sort of diff controller, or something -- a piece of global state that can be consulted. Something like: when a buffer is closed, that controller gets a signal to propagate the effect (a flag is set, basically). Every diff buffer checks on I can't really say how this would work without trying it out. I've been poking around trying to figure out a workaround for a while, but nothing simple seems to work. Reimplementing the old hack with some controller object should hopefully fix the issue, but there might be more complications. Since I can compile 7.4.52 locally, I'll see if I can implement it and push it to your branch. |
|
Ok, thanks for your effort! After reading a bit documentation, I saw that |
|
Yeah, I really think it's a bug in Vim, and I think it's been fixed already (this doesn't happen in my current Vim version, or in yours). I think you've done a good job of setting up the logic, but, in the end, it's just a matter of squeezing in a hack to work around a system bug :). I'll try to do it in the following days, but no guarantees when I'll manage, I'm afraid. |
If we :tabnew, and then :tabonly, all buffers get unloaded without a WinEnter in any of them. That's why this case exists, if `StartDestroying` has been called on a blank set of differs, we can just stop it. This should really be carefully documented somewhere.
Was used for debugging purposes.
|
Okay, so, I've pushed a few commits to your branch which should fix the tests. I've basically reimplemented the old For now, what I still need to do is:
If you can test this on your own Vim version and let me know if it doesn't segfault or something, that would be great. I'll switch to Vim 8 soon as well, but I'd like to write the tests first, so I know the new ones continue to work. |
|
cool, looks like you thought about a lot of corner cases! Btw, I was using/testing my own version quite a bit over the last weeks, and it worked fine but I got some error messages when closing modified diff-tabs. |
|
I didn't have too many real-world testcases these two days, but some. No problems so far! I will definitely continue to use this branch, so I'll report any possible problems here. Thank you very much for your effort! |
There was a problem hiding this comment.
I think that the PR is ready to merge now. I've added more test cases for the new features, and documentation for them. I've also tried to explain the closing logic in the "internals" section of the docs. I may have squeezed in some code that isn't directly connected to the PR while I was at it, but it was easier than trying to do a separate thing.
@hcgraf Could you go over the PR and see if you approve of its contents right now? Is there anything in the code you find unclear or confusing (I'm guessing the closing logic would be pretty weird, for instance, but I've tried to explain it in the "internals" section)? Is the naming of commands and settings to your liking?
I'm okay with waiting for a while to merge it, mind you, so feel free to keep using the branch for experimentation and check if there are edge cases that we've missed. Of course, we could also merge it sooner rather than later and fix issues as they pop up, I'm fine either way.
autoload/linediff.vim
Outdated
| @@ -1,23 +1,42 @@ | |||
| let s:differ_one = linediff#differ#New('linediff_one', 1) | |||
| let s:differ_two = linediff#differ#New('linediff_two', 2) | |||
| let s:differ = [] | |||
There was a problem hiding this comment.
I would prefer we call this variable s:differs or s:differ_list. The singular name "differ" doesn't indicate it's a collection of items.
autoload/linediff.vim
Outdated
| endfunction | ||
|
|
||
| function! linediff#LinediffAdd(from, to, options) | ||
| for dfr in s:differ |
There was a problem hiding this comment.
I'd prefer we use the full word "differ", I'm not a big fan of skipping vowels :). With the list variable called differs or differ_list, there would be no ambiguity.
autoload/linediff/differ.vim
Outdated
| @@ -12,8 +12,10 @@ function! linediff#differ#New(sign_name, sign_number) | |||
| \ 'sign_number': a:sign_number, | |||
| \ 'sign_text': a:sign_number.'-', | |||
| \ 'is_blank': 1, | |||
| \ 'other_differ': {}, | |||
| \ 'other_differs': {}, | |||
There was a problem hiding this comment.
I think this should be a list.
There was a problem hiding this comment.
Very well. I did all this basically without vimscript knowledge, only general programming knowledge. So it was mostly a mix'n'match approach ;)
|
Wow, thanks for the continued effort! I'm currently using it at work on a regular basis, but usually in a similar flow - so it's not likely to cover strange corner cases. Anyway, no issues so far! (Btw this is GVim in Windows and regular Vim in Linux - so at least some cases covered). I think I understand the logic of the closing-logic, the documentation seems as clear as it can get for this one. Although I recently had one use-case where I had wished for another behavior:
As for merging: I think it's highly unlikely that this PR will destroy anything for the regular user (used to diffing two areas and continuing to use it like this), so it might be helpful to get some more field-tests for the N-way diffing… |
|
Yes, I can agree that closing one buffer to leave two makes sense. Maybe it's time to reconsider the logic there. Originally, the idea was that there's no point in leaving just one diff buffer. Now that it's possible to add a bunch of them, it doesn't hold up. If you happen to have some time, I would love to get a PR for this logic, not sure how hard it would be. Something like it only resets once there's only one differ left. If not, an issue with a feature request to remind me would be fine, and I'd get to working on it in my own time. Either way, time to merge this and see what's next :). Thank you very much for your work! |
OK, that was quick :)
I just couldn't resist to hack with this a bit, so here is my patch for my improvement proposal #26
But it was done in a few hours, with only very basic vimscript knowledge, and without in-depth testing.
So maybe this PR is not really a PR but rather a TR (Try-Request).
I tried to be minimally-invasive, to change as little as possible.
Notably, the original UI was kept, so :Linediff will automatically start the diffing after 2 blocks have been selected.
Here is the rest of my commit message, it explains how to use the modification, and some other details:
OR use LinediffShow AFTER the last block
of diff buffers in the newest vim, version 8
they just throw an error if you try to diff more than 4 blocks
I would be very happy to receive feedback from anybody!