Use posframe or popup to show the current hunk

[alvarogonzalezsotillo]

This PR is a follow up of #145

I refactored the code to allow several backends to to show the differences of the current hunk. Currently, posframe and popup are implemented. Also, there is a mode diff-hl-show-hunk-mode with new key bindings (C-v x *, C-v x { and C-v x }) and another mode diff-hl-show-hunk-mouse-mode to enable the mouse in the fringe and the margin.

popup seems to work fine from the begining. posframe has been harder, but the current version works with Ubuntu (KDE, Gnome 3) and Windows.

I would like to include diff-hl-show-hunk in the diff-hl-command-map as C-x v *, but It would require a dependency from diff-hl to diff-hl-show-hunk, instead of the other way arround (not very clean). Do you think it will be possible in any other way?

I made minor changes in README.md, but I can add more instructions (see https://github.com/alvarogonzalezsotillo/diff-hl-show-hunk#customization)

The images added to the readme file have not the same style as the previous ones. Maybe you can get a static image with your own settings and change them. If not, I can change my theme in emacs and take again all the images.

Finally, this package is part of GNU ELPA, so to accept a contribution like this I'll have to ask you to complete copyright assignment to FSF. Would you be willing to do that?

If you like this PR, I will need some directions about how to proceed with the FSF.

[dgutov]

Seems like posframe already does this in posframe--create-posframe.

[alvarogonzalezsotillo]

You are right, I will undo this change

[dgutov]

Hi! Sorry for the late reply.

I like this in principle, but we'll need to discuss some details. And also it would be nice to have a review from the target audience (perhaps @deadtrickster would like to take a look?).

I would like to include diff-hl-show-hunk in the diff-hl-command-map as C-x v *, but It would require a dependency from diff-hl to diff-hl-show-hunk, instead of the other way arround (not very clean).

Do you want them there always or only when one of the new minor modes is enabled?

The images added to the readme file have not the same style as the previous ones. Maybe you can get a static image with your own settings and change them. If not, I can change my theme in emacs and take again all the images.

Yeah, the current screenshots here don't reflect the theme I'm using now either. I can redo them. Or we could standardize on the default Emacs theme. No dark themes, please. Though we could add some extra (dark) screenshots in a directory inside, and link to them.

If you like this PR, I will need some directions about how to proceed with the FSF.

Please see these instructions: https://code.orgmode.org/bzg/org-mode/raw/master/request-assign-future.txt

(Keep the answer to the first question that's written there, "Emacs"). That will let you contribute to Emacs itself later, as well as other packages in GNU ELPA.

Other notes

Two new modes is a good, conservative choice, considering some other feature might want to use the fringe/margin as well, and there is no existing way for such features to cooperate.

Since popup and posframe are not require-d by any of the features here (except at runtime), we probably need a bunch of declare-function declarations to clean up the byte compilation warnings.

Whenever you have several empty lines in a row (in the code), you can replace them with just one.

The new files need the appropriate license and copyright blurbs, as well as the author header (if it stumps you, I can add those later).

What about the default width? Will it often be shorter than the diff inside? If we're emulating a feature from some other editor, could you link to some screenshot, so we can compare the behavior?

On the screenshots, I see the popups showing more than just one hunk. Is that intentional? I wonder what the users prefer in general.

[deadtrickster]

Awesome, I will try it soon!

[alvarogonzalezsotillo]

I would like to include diff-hl-show-hunk in the diff-hl-command-map as C-x v *, but It would require a dependency from diff-hl to diff-hl-show-hunk, instead of the other way arround (not very clean).

Do you want them there always or only when one of the new minor modes is enabled?

I would like to be always available. Anyway, the user can add its own keybindings, so it is not very important.

Yeah, the current screenshots here don't reflect the theme I'm using now either. I can redo them. Or we could standardize on the default Emacs theme. No dark themes, please. Though we could add some extra (dark) screenshots in a directory inside, and link to them.

I agree dark themes are opinionable :)
Even if I use the default theme, I have not the same default font, dpi and window decorations. Can you take the new screenshots? I think is the best solution, and I can repeat the animated gifs and put them in a "screencast" directory.

Two new modes is a good, conservative choice, considering some other feature might want to use the fringe/margin as well, and there is no existing way for such features to cooperate.

Since popup and posframe are not require-d by any of the features here (except at runtime), we probably need a bunch of declare-function declarations to clean up the byte compilation warnings.

I will investigate this declare-funciton thing (I am still pretty new in emacs-lisp). I supose there will be something link declare-variable also.

Whenever you have several empty lines in a row (in the code), you can replace them with just one.

Ok

The new files need the appropriate license and copyright blurbs, as well as the author header (if it stumps you, I can add those later).

Ok

What about the default width? Will it often be shorter than the diff inside?

I don't change the width of the posframe, it seems to adjust automatically to the contents, and sometimes the width is smaller that the contents.
There is a custom variable diff-hl-show-hunk-posframe-poshandler that is a function that changes the posframe position and size. I can try to use some function like string-width to calculate the max line width, and change the width of the posframe.

If we're emulating a feature from some other editor, could you link to some screenshot, so we can compare the behavior?

I think I saw something like this in eclipse, called quick diff

On the screenshots, I see the popups showing more than just one hunk. Is that intentional? I wonder what the users prefer in general.

The content of the popup is determined by diff-hl-show-hunk-narrow. It can be:

• nil: The complete diff
• t: The hunk that is contained between two @@......@@ lines. This depends on the diff algorithm used by the underlying VCS (I suppose, only tested in git), and I think is the part of the file that will be reverted if diff-hl-revert-hunk

[dgutov]

I would like to be always available. Anyway, the user can add its own keybindings, so it is not very important.

Perhaps it should be added (and removed on disable) by diff-hl-show-hunk-mode?

Even if I use the default theme, I have not the same default font, dpi and window decorations. Can you take the new screenshots? I think is the best solution, and I can repeat the animated gifs and put them in a "screencast" directory.

Yes, sure. We'll do that at the end, after everything else is taken care of.

I supose there will be something link declare-variable also.

The counterpart for variables is just defvar, but without INITVALUE. It can be at the top level, or only inside the function that needs it (usually at its beginning).

I don't change the width of the posframe, it seems to adjust automatically to the contents, and sometimes the width is smaller that the contents.

When the diff is narrower than the current window, I suppose it's okay. But if it's wider, it would be better to use the width of the current window, I think.

Either way, perhaps we can do without diff-hl-show-hunk-popup-default-width? Or do you consider this customization valuable enough?

The content of the popup is determined by diff-hl-show-hunk-narrow.

Ok.

and I think is the part of the file that will be reverted if diff-hl-revert-hunk

Not exactly. Or, at least, while it is functionally the same, diff-hl-revert-hunk-1 makes an effort to separate the actual change from the context (by splitting it twice, usually). I wonder whether the users might prefer this, considering otherwise the context should be apparent in the parent buffer. Or not.

I think I saw something like this in eclipse, called quick diff

So Eclipse was your inspiration?

[dgutov]

I think it would be more correct to put just you as the author of this and other files here.

[dgutov]

The URL header is unnecessary here, it's only read from the main .el file by the package.el infrastructure.

[dgutov]

eval-when-compile is unnecessary here, IIRC.

[RosanneZe]

I've been trying out the feature and I like it. One point of feedback I would have for the popup version (I still need to install posframe and try it out), is that it would be good if the popup could always popup at the start of the line, so the aligment of the diff in the popup is the same of that of the file.

[deadtrickster]

I tried it out today, couldn't make posframe integration work (likely I messed up with my cache because I (mis)use gccemacs). One thing I spotted immediately - when I click on fringe it asks to save file. In my version I specifically avoided saving, I think it is not convenient and error prone. How to turn this off?

[alvarogonzalezsotillo]

One thing I spotted immediately - when I click on fringe it asks to save file. In my version I specifically avoided saving, I think it is not convenient and error prone. How to turn this off?

I use diff-hl-diff-goto-hunk to find the differences, that calls vc-buffer-sync, that saves the buffer. I don't know if there is other way to get the differences.

If I get this PR merged, my plan is to store the diffs in the overlays (#112) as an option, with a customize flag like diff-hl-store-diffs-in-overlays, and use diff-hl-diff-goto-hunk or the overlays based on that customized variable.

[alvarogonzalezsotillo]

is that it would be good if the popup could always popup at the start of the line, so the aligment of the diff in the popup is the same of that of the file.

Now I use point-at-bol to show the popup

Either way, perhaps we can do without diff-hl-show-hunk-popup-default-width? Or do you consider this customization valuable enough?

I used a customized variable because I didn't know what value to use. But now, it seems clear that the correct width for the popup is the full width, so i removed diff-hl-show-hunk-popup-default-width.

Still working in the width of the posframe.

So Eclipse was your inspiration?

I used Eclipse long time ago, and I liked quick-diff, and #112 was the final inspiration.
But the motivation is my son, that uses VSCode, and I need to show him that emacs and I are still relevant :)

On the screenshots, I see the popups showing more than just one hunk. Is that intentional? I wonder what the users prefer in general.

I think the solution could be the contex of the diff algorithm. Two hunks can be overlaped in the output of vc-diff-internal if the context arround the changes overlap.

If I customize vc-git-diff-switches to --unified=0 the hunks remain isolated.

For SVN, maybe setting vc-svn-diff-switches to --diff-cmd=diff -x -U0 could work.

[RosanneZe]

Now I use point-at-bol to show the popup

I'm still using your repo version, so I don't know if you updated this, but for me, if I mouse click on the fringe to get the popup (configured at the right fringe), it pops up at the end of line.
If I do M-x diff-hl-show-hunk the popup starts at the point.
But if you're going to make the popup full width, this might not be an issue anymore.

[deadtrickster]

If I get this PR merged, my plan is to store the diffs in the overlays (#112) as an option, with a customize flag like diff-hl-store-diffs-in-overlays, and use diff-hl-diff-goto-hunk or the overlays based on that customized variable.

why not to make overlays default?

[dgutov]

why not to make overlays default?

Perhaps that would not be optimal for both possible values of diff-hl-show-hunk-narrow? When it's t, saving a hunk to an overlay should work, but not in the other case.

[dgutov]

And either way, the code could reuse the method employed by diff-hl-flydiff, though it might require a bit more effort.

[dgutov]

@alvarogonzalezsotillo

Now I use point-at-bol to show the popup

Very good.

I used a customized variable because I didn't know what value to use. But now, it seems clear that the correct width for the popup is the full width, so i removed diff-hl-show-hunk-popup-default-width.

Yeah, looks like full width is the best. VS Code uses the full width anyway: https://stackoverflow.com/a/47296478/615245

But the motivation is my son, that uses VSCode, and I need to show him that emacs and I are still relevant :)

One of the best motivations! Props to you.

I think the solution could be the contex of the diff algorithm.

Right. Check out the macro diff-hl-with-diff-switches, it already applies said switches in a way that works for several VCSes. That's how diff-hl-changes-buffer gets the isolated hunks.

[alvarogonzalezsotillo]

why not to make overlays default?

Maybe there are some memory problems in large diffs. But I think there will be no problem with modern hardware.

Perhaps that would not be optimal for both possible values of diff-hl-show-hunk-narrow? When it's t, saving a hunk to an overlay should work, but not in the other case.

I am afraid diff-hl-show-hunk-narrow is here for historical reasons: my initial code didn't extract the hunk, because I didn't know how to do it. At this moment, I think there is no reason to maintain this option, and narrow-to-hunk is always the good choice.

If there are no objections, I will remove it.

[deadtrickster]

Fully understanding no one owes nothing to nobody, I would still like to point out that forcing save might be a show stopper for others as well. At least in my workflow I reach out to diffs when I think I'm screwing up, other users could have those live-reload-on-save a-la node.js systems and forcing users to save might be undesirable.

[alvarogonzalezsotillo]

There is a bug I'm working on: when going to previous hunk from the first hunk, No further hunks found appears in minibuffer instead of There is no previous change.

Solved.

You can use the :overline property instead. And I'd also like to see no empty line above the header.

There is a problem in console (not GUI): console doesn't not support overline, so I simulated it with an empty line with underline. I added a display-graphic-p test, hope it will have no problem with the GUI and the console is used at the same time (via emacsclient)

And when it's non-nil, I think it would be natural to have diff-hl-show-hunk-inline-popup-hide-hunk set to nil.

Currently, I prefer -inline-popup-hide-hunk set to t and -smart-lines set to t :) For me is like a wayback machine. I think I have set the default values to your preferences, tell me if not.

You're right about plain empty popup being a problem, though. Maybe it should show something, maybe it should display some clarification text,

I have added the diff-hl-show-hunk--no-lines-removed-message constant, that is shown when -smart-lines set to t and the hunk has only additions.

Will you remove the scrollable-quick-peek backend?

I forgot it. Removed.

[dgutov]

Solved.

Thanks.

There is a problem in console (not GUI): console doesn't not support overline, so I simulated it with an empty line with underline. I added a display-graphic-p test, hope it will have no problem with the GUI and the console is used at the same time (via emacsclient)

I wonder if console users would prefer no extra line, even if the horizontal line can't be shown. Anyway, we can leave that to future bug reports.

But note that you missed the first bit from the diff I posted: the one that removed a newline from in front of the header as well (in inline-popup--compute-header).

Currently, I prefer -inline-popup-hide-hunk set to t and -smart-lines set to t :) For me is like a wayback machine. I think I have set the default values to your preferences, tell me if not.

All right, so it seems we should keep the both option variables. But is it working right for you?

If you prefer for them to be both t, perhaps this piece of code should do something else?

+         (smart-lines (and diff-hl-show-hunk-inline-popup-smart-lines (not diff-hl-show-hunk-inline-popup-hide-hunk)))

I have added the diff-hl-show-hunk--no-lines-removed-message constant, that is shown when -smart-lines set to t and the hunk has only additions.

Perhaps it should say "no lines removed".

Something else I've been thinking: since diff-hl-show-hunk is going to be a built-in feature, there's no need for it to require a minor mode to be accessible. It can just be in diff-hl-mode by default. The mouse-mode can still be a minor mode, or it can be determined by a user option. WDYT?

[alvarogonzalezsotillo]

But note that you missed the first bit from the diff I posted: the one that removed a newline from in front of the header as well (in inline-popup--compute-header).

You are right, removed that extra new line.

All right, so it seems we should keep the both option variables. But is it working right for you? If you prefer for them to be both t, perhaps this piece of code should do something else?

I admit I have changed my mind a couple of times:

	hide-hunk	not hide-hunk
smart-lines	(1) Sort of wayback machine	(2) Your preferences (default)
not smart-lines	(3) My preferences	(4) New lines are shown twice, noisy

I have tried to avoid (4), and convert it to (3), but the logic was wrong. I have changed an and for an or to avoid (4), and convert it to (2)

[alvarogonzalezsotillo]

It can just be in diff-hl-mode by default. The mouse-mode can still be a minor mode, or it can be determined by a user option. WDYT?

I think mouse-mode and global-minor-mode should be a separate minor mode, to avoid conflicts with other packages, but the rest of key bindings can be in diff-hl-mode

[alvarogonzalezsotillo]

Something else I've been thinking: since diff-hl-show-hunk is going to be a built-in feature, there's no need for it to require a minor mode to be accessible. It can just be in diff-hl-mode by default.

I'm about to implement this comment. So, diff-hl.el should require diff-hl-show-hunk.el?

[dgutov]

So, diff-hl.el should require diff-hl-show-hunk.el?

Not necessary: you just add ;;;###autoload above the command's definition and add its name to diff-hl-mode's keymap. Then whenever somebody uses it, the package will be autoloaded.

[dgutov]

I have tried to avoid (4), and convert it to (3), but the logic was wrong. I have changed an and for an or to avoid (4), and convert it to (2)

Thanks for the table, it confirms that we do need both variables. But I think we'll just remove the condition for smart-lines: while it makes (4) impossible, let's just make it the user's choice. Doing a runtime check like this would be useful if we also predicated it on something that changes between runs/hunks/files or so on, but this looks like a decision the user can make on their own.

Finally, it's high time to start merging this. I'll try to make time this weekend to merge the "meat" of this PR (the modes, commands, and the keymap changes), but simplify the README change for now.

Redoing the README will come after (and BTW, I'll need the name of the tool you used for the gifs ;).

[alvarogonzalezsotillo]

But I think we'll just remove the condition for smart-lines: while it makes (4) impossible, let's just make it the user's choice.

Done

I'll need the name of the tool you used for the gifs ;)

If you mean the displayed keystrokes, I use screenkey. The version in Ubuntu repositories is not very up to date, so I use the repository version (just checkout and run ./screenkey --opacity 0.0) . I think it only works on X11 :(

If you mean the screencast, I use OBS and Kdenlive and then convert the video to gif with

  # https://superuser.com/questions/556029/how-do-i-convert-a-video-to-gif-using-ffmpeg-with-reasonable-quality
  INPUT="$1" 
  OUTPUT="$2"
  WIDTH="$3"
  ffmpeg  -i "$INPUT" -vf "fps=10,scale=$WIDTH:-1:flags=lanczos,split[s0][s1];[s0]palettegen[p];[s1][p]paletteuse" -loop 0 "$OUTPUT"

[dgutov]

Excellent, thank you.

[dgutov]

@purcell Hi Steve! Would you say it's okay for diff-hl to "incubate" the inline-popup package inside it with its current file name, or should it be renamed (together with all symbols in it) to match the name diff-hl?

[purcell]

Would you say it's okay for diff-hl to "incubate" the inline-popup package inside it with its current file name, or should it be renamed (together with all symbols in it) to match the name diff-hl?

It should match the name prefix, or we could have the two packages coexist in the same repo, ie. 2 recipes. That's a bit fiddly though. I'd suggest incubating with the diff-hl- prefix, then later split it out and drop the prefix.

[dgutov]

I'd suggest incubating with the diff-hl- prefix, then later split it out and drop the prefix.

Thanks for the reply, guess I'll go that route indeed.

[alvarogonzalezsotillo]

I didn't notice the new diff-hl-amend-mode of the last month.

The header of the inline popup should be updated with the revision the diff is computed against, something like:

   -"Diff with HEAD"
   +(if (and (boundp 'diff-hl-reference-revision) diff-hl-reference-revision)
   +    (concat "Diff with " diff-hl-reference-revision)
   +  "Diff with HEAD")

Since you are merging this PR, do you prefer to make this change yourself? Or is it better if I merge master in this PR and push this change?

[dgutov]

@alvarogonzalezsotillo Please go ahead.

[alvarogonzalezsotillo]

@dgutov Just in case, you can find in https://github.com/alvarogonzalezsotillo/diff-hl/tree/unincubating-inline-popup a branch that has merged master, this PR and inline-popup-to-diff-hl-inline-popup refactor.

If you prefer I can merge unincubating-inline-popup in this PR.

[dgutov]

Thanks! I've done the merge now, and simplified some bits and pieces.

Please take a look, see whether everything still works to your expectations, and let's continue fixing and improving the feature in new issues and PRs.

I hope to get around to refreshing the images (and adding a gif or two) sometime soon.