search

junkblocker / patchreview-vim

Vim/Neovim plugin for doing single, multi-patch or diff code reviews
http://www.vim.org/scripts/script.php?script_id=1563
108 stars 8 forks source link

old/new windows are backwards #3

Closed kevin8t8 closed 11 years ago

kevin8t8 commented 11 years ago

I love this tool - it makes reviewing patches so much easier. I've had an issue that's been bothering me for a while, and I'm not sure whether it's my config or an issue with the tool.

When I execute :PatchReview some-file.patch, it opens a new tab for each modified file. In each tab, though, the "before" is on the right and the "after" is on the left. For me, this is backwards from what I expect. Is this expected, or configurable? Or is it interaction with something in my config?

Thanks!

junkblocker commented 11 years ago

Thanks :)

That behavior is as designed. The concept in my head is a) current content is on the right b) how it would be after applying the patch, or how it was before the change in case of a diff review, is on the left. I sort of got into that habit of thinking due to the built in :diffpatch command's behavior. So I never thought of making it configurable before. But I understand that mental concepts are very important and will look into making it configurable. Of course, I'll gladly accept a patch for this if you or somebody makes one..

junkblocker commented 11 years ago

Oops, referred to the wrong bug number.

kevin8t8 commented 11 years ago

I just created a pull request for one proposal for this. I tried to choose a reasonable name for the option, but I totally understand if you want to grab my patch and change it to your liking instead. Or, just let me know if there are any changes I can make. Thank you!

junkblocker commented 11 years ago

Looks good. I'll try it out with a few test cases and let you know in a couple of days. The variable name is ok but maybe I'll want to say something like patchreview_reverse_splits_direction instead of windows. Also noticed that the valid values setting is documented as just {1} whereas it should be {0|1} for the new option but I have the same mistake for the crlf setting.

kevin8t8 commented 11 years ago

Thank you for taking a look at the patch. I haven't done much vim coding, so I admit I did just copy and modify the documentation. :-)

Looking again at my own patch, I think I may have put the l:leftright command in too many places. It looks like some of them are doing horizontal splits, so I don't think I needed to modify those. The plugin is definitely complicated, so I wasn't completely sure if it was needed in several places.

junkblocker commented 11 years ago

Yes, horizontal split happens in case a patch can not be applied cleanly to display the fragment that could not be applied, or if there was a patch parsing error of some sort. If you are willing to wait, I can just implement this over the coming weekend. It's also high time I added a test suite to catch silly errors like in the 1.0.7/1.0.8 releases so the next release is going to need to wait for that anyway.

kevin8t8 commented 11 years ago

That sounds fine with me. I'm happy to wait!

junkblocker commented 11 years ago

@kevin8t8 - let g:patchreview_split_right = 1 . Thanks for the bug report and the pull request. The fix wasn't too far off from what you built :)