Skip to content
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

Move line number into hunk header #473

Merged
merged 8 commits into from
Dec 28, 2020
Merged

Move line number into hunk header #473

merged 8 commits into from
Dec 28, 2020

Conversation

dandavison
Copy link
Owner

@dandavison dandavison commented Dec 28, 2020

Move line number into hunk header cc @ulwlu @torarnv

This PR gets rid of the isolated line number and moves it into the hunk header. This is similar to @torarnv's #309 (comment).

By default the new hunk header will look like this (that's hunk-header-style = line-number syntax)

image

The file path can be included with hunk-header-style = file line-number syntax:

image

or just the file path and no line number hunk-header-style = file syntax

image

hyperlinks still functions as before, converting the line number into a hyperlink. We may want to consider hyperlinking the file path as well.

@dandavison dandavison changed the title Move line number Move line number into hunk header Dec 28, 2020
@dandavison dandavison force-pushed the handle-hunk-header-refactor branch 2 times, most recently from e94327e to f680b51 Compare December 28, 2020 02:43
Base automatically changed from handle-hunk-header-refactor to master December 28, 2020 03:11
@dandavison dandavison merged commit 959b211 into master Dec 28, 2020
@dandavison dandavison deleted the move-line-number branch December 28, 2020 03:17
@torarnv
Copy link
Contributor

torarnv commented Dec 28, 2020

Awesome, thank you!!! I can now enable this:

image

And with iTerm2's semantic history logic, hovering over the file:line allows me to click and go directly to the location in the file:

image

https://iterm2.com/documentation-preferences-profiles-advanced.html

Enabling hyperlinks in delta seems to confuse iTerm:

image

Either way, I would recommend hyperlinking both the file and line number as one item, as you can use hyperlinks-file-link-format to tweak the resulting hyperlink, e.g. "file://{path}#line={line}"

One additional feature that I'm not sure is possible, is to (optionally) offset the line number with the number of context lines. Eg., if I pass -U0, I'm linked directly to the first actually changed line:

image

I'd like the line number to refer to line 14, so I can jump directly to the affected code, but still show context lines in the diff.

@dandavison
Copy link
Owner Author

Hey @torarnv, thanks for the -U0 point, I'm going to look into that. Regarding

Enabling hyperlinks in delta seems to confuse iTerm:

Hm, this should work. I would ask if your less version supports hyperlinks, but you've already been involved in gwsw/less#54! Are you using tmux? That also needs a patch applying in order for it to support hyperlinks (e.g. dandavison/tmux@31ae4ab).

@torarnv
Copy link
Contributor

torarnv commented Dec 28, 2020

You're right, I hadn't upgraded my less 😊 Thanks!

Works fine with less v570+ (brew install less --HEAD), or by passing -r to earlier versions.

image

As suggested earlier I think it would make sense to make the file:line the hyperlink, not just the line, but as I'm using the iTerm2 semantic hyperlinking it's not a showstopper for me 😊

@dandavison
Copy link
Owner Author

Great. I agree, I'll make the whole thing the hyperlink. It might be slightly ugly with the way iterm2 currently renders them but I still agree it's the right thing to do.

@dandavison
Copy link
Owner Author

dandavison commented Dec 29, 2020

@torarnv I've hyperlinked the entire file:line-number construct (in #479, which is merged to master).

There's one issue with the hyperlink: when the file path has a color, the hyperlink rendering terminates at the end of the file and does not extend to the line number. I'm just going to dump these examples here. I haven't looked into this properly yet and don't know whether it's a bug in my ANSI codes, or a bug in iTerm2, or it's not supported by the spec, etc.


--file-style red: incorrect rendering

git show | delta --no-gitconfig --hunk-header-style 'line-number file syntax' --file-style red --hunk-header-decoration-style 'plain box' --hyperlinks

image

␛]8;;file:///Users/dan/src/delta/src/delta.rs␛\␛[38;5;1msrc/delta.rs␛[0m:350␛]8;;␛\:·

--file-style-plain --hunk-header-decoration-style 'red box' correct rendering

git show | delta --no-gitconfig --hunk-header-style 'line-number file syntax' --file-style plain --hunk-header-decoration-style 'red box' --hyperlinks

image

␛]8;;file:///Users/dan/src/delta/src/delta.rs␛\src/delta.rs:␛[38;5;1m350␛[0m␛]8;;␛\:·

--file-style-plain --hunk-header-decoration-style 'plain box' correct rendering

git show | delta --no-gitconfig --hunk-header-style 'line-number file syntax' --file-style plain --hunk-header-decoration-style 'plain box' --hyperlinks

image

␛]8;;file:///Users/dan/src/delta/src/delta.rs␛\src/delta.rs:350␛]8;;␛\:·

@torarnv
Copy link
Contributor

torarnv commented Dec 29, 2020

Thanks!!

There's one issue with the hyperlink: when the file path has a color, the hyperlink rendering terminates at the end of the file and does not extend to the line number. I'm just going to dump these examples here. I haven't looked into this properly yet and don't know whether it's a bug in my ANSI codes, or a bug in iTerm2, or it's not supported by the spec, etc.

Hmm, hard to say. I'd keep it as is for now, and file an issue with iTerm2 to see what they say.

@kevinhwang91
Copy link

@dandavison
I encounter an issue, here's config to reproduce:

[delta]
    line-numbers = true
    hunk-header-style = file line-number syntax

image

can't display header's line number when line-number = true.

@dandavison
Copy link
Owner Author

dandavison commented Dec 31, 2020

Thanks @kevinhwang91. That was stupid of me to leave that inconsistency in! Fixed in master at a499d46. Yes, the first line number will then be present in two places, but as @torarnv has mentioned, the file.rs:447: construct is helpful for tools to open files at the correct line in text editors. And I should know that! [1] [2]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants