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

Lock scrolling is not working when the cursor is not on the scrolled buffer #29

Open
otadmor opened this issue Jan 26, 2021 · 2 comments

Comments

@otadmor
Copy link

otadmor commented Jan 26, 2021

How to reproduce:

  1. start emacs within terminal
  2. do M-x xterm-mouse-mode
  3. M-x eval-expression (setq vdiff-lock-scrolling t)
  4. do M-x vdiff-files with two files (the amount of lines / difference between them dont seem to matter).
  5. place the cursor on the left buffer
  6. hover the mouse above the right buffer
  7. scroll the right buffer with the mouse wheel, keeping the cursor on the left buffer and the mouse-cursor on the right buffer

What happens?

  1. only the right buffer scrolls

What should happen?

  1. both of the buffers should scroll

Notes:

  1. Running from X (and not the terminal) have the same behavior.
  2. Also happens when the cursor is on the right buffer and the mouse scrolls on the left one.
@justbur
Copy link
Owner

justbur commented Jun 14, 2021

It looks like this is a limitation of emacs, and I'm not sure how to fix it. vdiff uses the window-scroll-functions hook to determine when to update the buffers, and this hook supposedly fires when window-start is updated in a buffer, but it seems that scrolling with a mouse does not have this effect and I'm not sure why.

To be honest, synchronizing the scrolling was the most difficult aspect of getting vdiff to work, and I never found a perfect way to do it. I would very much take any help that anyone would be willing to provide.

billsacks added a commit to billsacks/.emacs.d that referenced this issue Mar 23, 2022
(1) It doesn't work to scroll a window by small amounts if there are
virtual removed lines at the top of the window. So if we try and fail to
scroll a given vdiff window, switch to the other window and try the
scrolling there.

(2) vdiff's synchronized scrolling doesn't work when you try to do mouse
scrolling of one window when the other window is active. So force mouse
scrolling to only work on the active window in vdiff mode. (See also
justbur/emacs-vdiff#29.) This is especially
important to make mouse scrolling usable given that we're doing (1),
since which window is active can now change frequently while scrolling.
@billsacks
Copy link

As a workaround for this issue, I have put the following in my emacs customizations:

;; In vdiff-mode, the synchronized scrolling doesn't work if you try to scroll the
;; non-currently-active window (see also
;; https://github.com/justbur/emacs-vdiff/issues/29). So force mouse-based scrolling to
;; always scroll the active window in vdiff-mode. (I thought we would be able to do this
;; by setting mouse-wheel-follow-mouse, but that doesn't seem to have any effect - maybe a
;; bug?)
(defun my-vdiff-mouse-wheel--get-scroll-window (orig-fun &rest args)
  (if (bound-and-true-p vdiff-mode)
      (selected-window)
    (apply orig-fun args)))
(advice-add 'mouse-wheel--get-scroll-window :around #'my-vdiff-mouse-wheel--get-scroll-window)

Basically, this bypasses the function for choosing which window to scroll when we're in vdiff-mode, instead always choosing the active window. This feels like reasonable behavior when we're in vdiff-mode. (As noted in the comment, I first tried to do this by setting mouse-wheel-follow-mouse, but setting that to nil doesn't seem to work, at least on my Mac with emacs28. So I was forced to override the entire relevant function via advice.)

Slightly off-topic but slightly related: I also added the following to make scrolling work better:

(defun my-vdiff-scroll-up-command (arg)
  "Version of scroll-up-command for use in vdiff-mode

This generally works the same as scroll-up-command, but if we
can't scroll the current window, then we switch to the other
window before scrolling up.

This is needed in vdiff mode when the top of a buffer contains
virtual removed lines: trying to scroll that buffer by small
amounts doesn't work.

Note that this can lead to the other window becoming active."
  (let ((orig-window-start (window-start)))
    (scroll-up-command arg)
    (when (eq (window-start) orig-window-start)
      ;; the scroll was unsuccessful; switch to the other window and try scrolling there

      ;; this assumes that (other-window 1) gives the other window associated with
      ;; this vdiff session
      (other-window 1)
      (scroll-up-command arg))))
(defun my-maybe-vdiff-scroll-up-command (arg)
  "Generally works the same as scroll-up-command, but in vdiff mode
this might switch to the other buffer before scrolling up."
  (if (bound-and-true-p vdiff-mode)
      (my-vdiff-scroll-up-command arg)
    (scroll-up-command arg)))

(setq mwheel-scroll-up-function 'my-maybe-vdiff-scroll-up-command)

The motivation for this was: I found that, if the cursor is in a window where there are a few removed lines at the top of the window, scrolling gets stuck if you try to scroll by small amounts. I'm not sure if there's a way to fix this elegantly within vdiff itself (my elisp isn't that great), but this works around the issue in a slightly hacky way: If we try to scroll but find that nothing happened, then we take that as an indication that scrolling has gotten stuck like this. In that case, we switch to the other window (assuming that (other-window 1) always gives the other window in this vdiff session, which might not be a great assumption but works for my workflow) and try the scroll command again from there. This potentially leads to the cursor bouncing back and forth between the buffers as you scroll, but I feel like this is an acceptable trade-off for being able to scroll continuously without scrolling getting stuck. This second set of changes made the modification to mouse-wheel--get-scroll-window even more important, though: for mouse-based scrolling to work smoothly, both of these need to be done together.

I should note that I have only been using vdiff for a couple of days, so I don't yet have extensive experience with these changes, but so far they are making scrolling through diffs feel pretty smooth.

Thank you very much for your work on this vdiff package! Compared to ediff, I really like (1) the better alignment of diff regions, which stay aligned as you scroll; and (2) the highlighting of all fine diffs, rather than just those of the current region. I find that these two things make it easier to quickly review diffs.

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

No branches or pull requests

3 participants