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

Selecting long lines in a diff hunk in Emacs 25 highlights extra lines #2758

Closed
fice-t opened this issue Sep 4, 2016 · 29 comments
Closed

Selecting long lines in a diff hunk in Emacs 25 highlights extra lines #2758

fice-t opened this issue Sep 4, 2016 · 29 comments

Comments

@fice-t
Copy link
Contributor

@fice-t fice-t commented Sep 4, 2016

  1. First make some changes to a file so its diff contains lines longer than the width of the Emacs window.
  2. From emacs -Q load magit and enter the status buffer for the repo.
  3. View the diff and go the end of the line (past the window edge).
  4. C-SPC to select the line.

Notice how it's normal in Emacs 24.5 but in 25.1 the lines above and below the line are highlighted as well, pushing the selected line and the one below it down 1/2 lines respectively.

PS: I noticed how if you have consecutive lines that go past the window edge and you try to use the mouse to click and drag a region, the selection will jump right to the bottom of the lines instead of going line by line as would happen when using the keyboard.

Magit 20160902.1451, Git 2.9.3, Emacs 25.1.1, gnu/linux

@npostavs
Copy link
Member

@npostavs npostavs commented Sep 4, 2016

I can confirm.

PS: I noticed how if you have consecutive lines that go past the window edge and you try to use the mouse to click and drag a region, the selection will jump right to the bottom of the lines instead of going line by line as would happen when using the keyboard.

This part happens in 24.5 as well.


Both these issues are fixed using the np/overlay-rm-nl branch (which I've just rebased onto master). Though it has its own minor problems: move-end-of-line will skip to the next line, and the bottom line doesn't quite get rendered far down enough (because it's an underlining of text).

@tarsius tarsius added the bug label Sep 6, 2016
@fice-t
Copy link
Contributor Author

@fice-t fice-t commented Sep 15, 2016

PPS: I noticed that if you click once on a diff hunk in 25.1 then the line will automatically be highlighted, but in 24.5 it just positions the point like expected.

I wonder what happened in 25.1 that broke this.

@tarsius
Copy link
Member

@tarsius tarsius commented Sep 19, 2016

I am trying to wrap my head around the changes in np/overlay-rm-nl again. There's a lot of fixup commits in there and failed and reverted attempts. That's good it tells us about what we were thinking, but it would be better if we had branches for the various attempts and things were ordered logically instead of historically.

@tarsius
Copy link
Member

@tarsius tarsius commented Sep 19, 2016

I have pushed the result to a new branch overlay-rm-nl.

@tarsius
Copy link
Member

@tarsius tarsius commented Sep 20, 2016

I noticed that if you click once on a diff hunk in 25.1 then the line will automatically be highlighted, but in 24.5 it just positions the point like expected.

That has annoyed me for a while to but I did not realize it was a regression with 25.1.

I wonder what happened in 25.1 that broke this.

Some code that used to be run when the mouse is dragged is now already run on mouse-down-1, I suppose.

Anyway, I fixed it.

tarsius added a commit that referenced this issue Sep 20, 2016
@tarsius
Copy link
Member

@tarsius tarsius commented Sep 20, 2016

Hm, not quite, seems a bit more complicated.

@tarsius
Copy link
Member

@tarsius tarsius commented Sep 20, 2016

In v25.1 the region highlighting happens at a different time in the command loop than it used to. It now happens after this-command is already nil again.

@tarsius tarsius reopened this Sep 20, 2016
@tarsius
Copy link
Member

@tarsius tarsius commented Sep 20, 2016

Though it has its own minor problems: move-end-of-line will skip to the next line, and the bottom line doesn't quite get rendered far down enough (because it's an underlining of text).

I cannot reproduce these issues anymore with v25.1! If you can confirm that, then we should probably start using the new implementation at least for that Emacs version.

@tarsius
Copy link
Member

@tarsius tarsius commented Sep 20, 2016

We should squash the commits and the final commit message should explain each change. There should probably be a bullet point for each of the current commits. @npostavs could you please do that for your commits, I don't fully understand all the current commit messages.

@npostavs
Copy link
Member

@npostavs npostavs commented Sep 21, 2016

I cannot reproduce these issues anymore with v25.1!

Oh, I got mixed up as to which thing is broken where, see #2293 (comment) for the correct details. The move-end-of-line thing is only broken in v24 indeed. There is still the temporary-goal-column problem in both, to see, move to the end of a shorter line and then hit C-n to move to a longer one. Point moves to end of line instead of staying in the same column.

I do see the line rendering thing regardless, it might depend on the font. Here is a screenshot, notice the orange line crosses the |.

magit-screenshot

could you please do that for your commits, I don't fully understand all the current commit messages.

I'll try to squash things down into something more managable tomorrow.

@tarsius
Copy link
Member

@tarsius tarsius commented Sep 22, 2016

Or maybe we should just give up and use this instead:

diff --git a/lisp/magit-diff.el b/lisp/magit-diff.el
index 9088e2f..f242193 100644
--- a/lisp/magit-diff.el
+++ b/lisp/magit-diff.el
@@ -2111,15 +2111,7 @@ (defun magit-diff-update-hunk-region (section)
         (ov sbeg cbeg 'face 'magit-diff-lines-heading
             'display (concat (magit-diff-hunk-region-header section) "\n"))
         (ov cbeg rbeg 'face face 'priority 2)
-        (when (and (window-system) magit-diff-show-lines-boundary)
-          (ov rbeg (1+ rbeg) 'before-string
-              (propertize (concat (propertize "\s" 'display '(space :height (1)))
-                                  (propertize "\n" 'line-height t))
-                          'face 'magit-diff-lines-boundary))
-          (ov rend (1+ rend) 'after-string
-              (propertize (concat (propertize "\s" 'display '(space :height (1)))
-                                  (propertize "\n" 'line-height t))
-                          'face 'magit-diff-lines-boundary)))
+        (ov rbeg (1+ rend) 'face 'bold)
         (ov (1+ rend) send 'face face 'priority 2)))))

 ;;; Diff Extract

@npostavs
Copy link
Member

@npostavs npostavs commented Sep 22, 2016

Or maybe we should just give up and use this instead:

Yeah, Stefan did suggest something like that in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=21468#28

@fice-t
Copy link
Contributor Author

@fice-t fice-t commented Sep 23, 2016

In v25.1 the region highlighting happens at a different time in the command loop than it used to. It now happens after this-command is already nil again.

Was changing the header colour when the mouse is clicked down an intentional change? It's a bit distracting in my theme.

Or maybe we should just give up and use this instead:

I don't think that is as easy to view as the previous behaviour.

@tarsius
Copy link
Member

@tarsius tarsius commented Sep 24, 2016

@npostavs' squashed iteration can be found in #2791. I have also pushed a new iteration which lets users pick their preferred variant: https://github.com/magit/magit/compare/hunk-region-styles?expand=1.

Was changing the header colour when the mouse is clicked down an intentional change?

No, but I am not sure how easy it is to fix.

@tarsius
Copy link
Member

@tarsius tarsius commented Sep 24, 2016

No, but I am not sure how easy it is to fix.

My second commit seems to do the trick.

@tarsius
Copy link
Member

@tarsius tarsius commented Sep 30, 2016

I noticed that if you click once on a diff hunk in 25.1 then the line will automatically be highlighted, but in 24.5 it just positions the point like expected.

I have pushed a fix for that part.

@tarsius
Copy link
Member

@tarsius tarsius commented Sep 30, 2016

Okay. Piu. I think I am done. I just merged my latest iteration of the hunk-region ordeal ;-)

The new option magit-diff-highlight-hunk-region-functions explains it all, I hope.

@tarsius tarsius closed this as completed Sep 30, 2016
@fice-t
Copy link
Contributor Author

@fice-t fice-t commented Sep 30, 2016

@tarsius Feature-wise it looks fine, thanks for your and @npostavs's efforts.

However, it hangs Emacs (25+) when selecting some lines past the window edge, forcing me to kill the process.

Recipe:

In the status buffer select a diff that goes past the window edge and go to the end of the line.
C-SPC
Press C-n until the point attempts to go into a line that doesn't go past the edge. Emacs will then hang.

@tarsius
Copy link
Member

@tarsius tarsius commented Sep 30, 2016

Yikes. I can reproduce and have temporarily removed the using-underline variant.

@tarsius tarsius reopened this Sep 30, 2016
@fice-t
Copy link
Contributor Author

@fice-t fice-t commented Sep 30, 2016

Should the underline variant be temporarily removed from the default value of magit-diff-highlight-hunk-region-functions as well?

@tarsius
Copy link
Member

@tarsius tarsius commented Sep 30, 2016

Yes. Done.

@tarsius
Copy link
Member

@tarsius tarsius commented Oct 1, 2016

The older version in ef91987 had the same issue, so its not something in my recent changes.

"Stepping over the line" is what triggers the issue, when removing the bottom underline (or the top overline when moving backward), then the hang does not occur. Not very surprising, but confirmed.

@tarsius
Copy link
Member

@tarsius tarsius commented Oct 2, 2016

I was able to prevent the hang, which shows that for some reason (tm) window-hscroll keeps returning larger and larger values when moving from a position that requires scrolling to one that does not.

The debug code with comments can be found at https://github.com/magit/magit/compare/debug-align-to?expand=1, in particular ce311cf.

@npostavs
Copy link
Member

@npostavs npostavs commented Oct 2, 2016

In the status buffer select a diff that goes past the window edge and go to the end of the line.
C-SPC
Press C-n until the point attempts to go into a line that doesn't go past the edge. Emacs will then hang.

It seems that the amount it goes past the window edge matters too. I have a tiling window manager and with Emacs at full screen, the ;; go-to eol to scroll ;;;;... line does go past the window edge, but moving off it didn't trigger the hang until I made the Emacs frame smaller.

Hmm, and then the add-hook which says it prevents the hang, does not prevent it.

@tarsius
Copy link
Member

@tarsius tarsius commented Oct 6, 2016

So basically we both have no idea what is going on? Those little cute lines seemed like such a good and simple idea at the time... Maybe it's time to admit that, like Eli says, we are indeed abusing the display engine.

Should I just remove the leftover documentation about the "temporarily" removed function and we put this of for later again? At least we now have a variant which just makes the "region" bold and isn't affected by any of those glitches.

I think our time would be better invested lobbying for negative overlay priority and maybe even overlay effects. (By overlay effect I mean that e.g. :background (0.5 "gray") would be applied to whatever background color would have been in effect without the overlay "covering" it.)

@npostavs
Copy link
Member

@npostavs npostavs commented Oct 7, 2016

I've reported this as Emacs Bug#24633, but I expect it won't be fixed until Emacs 26 anyway. So unless there is some clever workaround, "temporarily" will be for quite a while.

@npostavs
Copy link
Member

@npostavs npostavs commented Oct 9, 2016

So it looks like the problem is that the decision to scroll the window doesn't take into account the cursor property, hence making the :align-to spec bigger causes scrolling which makes the spec bigger... Eli says that fixing this would be "messy":

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24633#24

It'd be messy, yes. We'd need to search for before- and after-strings
that begin/end right at point, then augment the call to the display
emulating function accordingly, deal with complications like strings
that include newlines, etc.


So yeah, probably we should just give up on this.

@npostavs
Copy link
Member

@npostavs npostavs commented Oct 9, 2016

Actually using something like `(+ (0 . right) ,(min (window-hscroll) (- (line-end-position) (line-beginning-position)))) avoids the problem at the cost of the line sometimes not quite reaching the edge of screen.

tarsius added a commit that referenced this issue Oct 9, 2016
This iteration does not suffer from the infinite hang of the
previous version, but the lines it draws are sometimes to short.
There is nothing we can do about it.  For more information see
#2758 ff.
@tarsius
Copy link
Member

@tarsius tarsius commented Oct 9, 2016

I've reintroduced the underline variant with the suggested change. This time around the overlay variant is used as the default regardless of Emacs version. The "wonfix" on this issue means "we won't provide a variant that draws boundary lines and has no glitches". That just seems more and more impossible. Choose the glitches that bother you less or use the face (bold) variant. Those are the only options, I am afraid.

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

No branches or pull requests

3 participants