Open diff view from blame at the correct line#1375
Conversation
src/diff.c
Outdated
| if (file) { | ||
| state->file = get_path(file); | ||
| state->lineno = lineno_from_blame; | ||
| state->pos.offset = 5; // TODO: Should be half screen |
There was a problem hiding this comment.
I haven't yet figured out how pos works or what it means exactly. This happens to mostly work, but I wonder whether I'd have to refactor state->pos for a proper fix.
Only the difference between state->pos and state->lineno is ever used, and that happens only in one spot, so this would be easy to refactor.
It's not yet clear to me how to make the offset be half-screen, since view->height is not always initialized at this point.
src/blame.c
Outdated
| } else { | ||
| open_diff_view(view, flags); | ||
| if (filename_from_blame) { | ||
| free(filename_from_blame); |
There was a problem hiding this comment.
Somehow, I don't remember the pattern of "free the memory all but one time, and by the time winter rolls around, the gorillas simply freeze to death" in Kerninghan and Ritchie. 😢
Perhaps I should write to them and they'll put it into the next edition.
Update to a joke: Apparently, Dennis Ritchie died in 2011. 😞 Kernighan is OK so far, but probably no next edition.
There was a problem hiding this comment.
OK, I think I know how to fix this: simply get the blamed line and the corresponding commit from the global blame_view in diff.c.
I may or may not get to it before I hear something back about whether the project is interested in this functionality.
src/blame.c
Outdated
| if (filename_from_blame) { | ||
| free(filename_from_blame); | ||
| } | ||
| filename_from_blame = strdup(state->filename); |
There was a problem hiding this comment.
This used to not work correctly if the file was renamed since the blamed line. It's now fixed, but might still want a test.
Update: I can make the existing test use line 49 of deltablue/src/main/scala/org/scalajs/benchmark/deltablue/DeltaBlue.scala instead of the Build.scala (IIRC) it's currently using.
When pressing <Enter> (AKA `:enter`) on a line in the blame view, previously the diff view would open with the cursor either on the first line or on whatever line was last selected in that diff view (if it was previously opened). This commit makes the diff view open at the line corresponding to the line that was selected in the blame view. One can still press <d> (AKA `:view-diff`) to open the diff view at either the first line or the previously selected line. The implementation is currently proof-of-concept quality: it works, is tested, but does not fit well with the rest of the code. I'd need help to make the memory management more robust and to fit better with tig's design. Also, I currently re-run the `git diff` command every time the diff view is opened from blame, since I couldn't figure out an easier way to have the view load without using the line it was previously opened at.
include/tig/blame.h
Outdated
|
|
||
| extern struct view blame_view; | ||
| extern char *filename_from_blame; | ||
| extern unsigned long lineno_from_blame; |
There was a problem hiding this comment.
I think creating these two globals isn't actually necessary, though it's convenient for a POC. This information is also stored inside blame_view, though it's a bit fussy to extract it (see also a similar comment I made below).
* use view->env for inter-views communication * move state initialisation after view->lines is known * I don't see a real added value for open-diff-test over existing default-test for the blame view
|
Thanks, the experience is really good. As you expected, your implementation is not fully in line with Tig way of doing things, I pushed a commit with my review directly to your branch. |
ilyagr
left a comment
There was a problem hiding this comment.
Glad you like it! Thanks for all the fixes!
I think the test you removed was mostly testing for forgetting to set the OPEN_RELOAD flag (which I of course did in the first versions of the patch).
Make the diff view open at the line corresponding to the line that was selected in the blame view when pressing `<Enter>` (`:enter`). The behaviour when pressing `d` (`:view-diff`) remains unchanged, the diff view opens at either the first line or the previously selected line. Co-authored-by: Thomas Koutcher <thomas.koutcher@online.fr>
|
Hmm, this is not improvement for me, personally. I usually just look at the commit message, rather than the diff, and the "old way" was perfect for my use case. 😊 Is there some way to make it work as it did before? I've tried But that doesn't work (because the loading is asynchronous?), and I get the message |
|
|
Not exactly what I wanted, but definitely good enough. Thank you! 😀 |
This could be considered an RFC or even just a feature request. The feature goes with #1372 and #1370, but AFAICT it was never implemented before.
The implementation is a bit rough at this point; I had trouble following Tig's structure (I'm not an expert C programmer), would need help to make the code fit in better and maybe be more efficient.
However, it seems to work fine. Even if this is not merged, people can use it as a patch.
When pressing (AKA
:enter) on a line in the blame view,previously the diff view would open with the cursor either on the first
line or on whatever line was last selected in that diff view (if it was
previously opened). This commit makes the diff view open at the line
corresponding to the line that was selected in the blame view.
One can still press (AKA
:view-diff) to open the diff view ateither the first line or the previously selected line.
The implementation is currently proof-of-concept quality: it works,
is tested, but does not fit well with the rest of the code. I'd need
help to make the memory management more robust and to fit better with
tig's design.
Also, I currently re-run the
git diffcommand every time the diff view is opened from blame, since I couldn't figure out an easier way to have the view load without using the line it was previously opened at.