Ambigous cursor positions around invisible text in status buffer

[michael-heerdegen]

Hello,

this is an annoying bug I see all the time I think when there are multiple lines of unstaged stuff in the status buffer. I doubt it's related to my setup, but please tell me should you not see this behavior.

I use goal-column -> 0, that triggers the bug often because it occurs only in the first column in the status buffer.

Here is what I see: this is what my status buffer looks like:

Now I hit [down] (bound to next-line) two times to get to "iterators.el". This looks like this:

There is no highlighted file or hunk to operate on, this already shows that there is something not kosher. Ok, I hit s to stage all changes in "iterators.el". I see this:

Obviously, I operated on a line different from the line where the cursor had been displayed.

Such problems are typical when using overlays making buffer parts invisible. Maybe there is an invisible newline character between point and the visible line, so that point is not located at the line where it seems to be, or something similar.

Thanks in advance,

Michael.

[npostavs]

I use goal-column -> 0, that triggers the bug often because it occurs only in the first column in the status buffer.

Yes, after setting goal-column to 0 I see the same. Seems to be similar to #1771, I guess the goal column prevents visual movement from avoiding the problem.

[michael-heerdegen]

I think instead of forcing some user settings trying to avoid the problem, we should try to find the cause of the problem. Is it a bug in emacs, or do we do something wrong?

[michael-heerdegen]

I think part of the problem is which newline characters we make invisible.

Currently, the newline after the last line we want to stay is not made part of the overlay making text invisible. The newline after the last line of the text we want to hide is included in the overlay.
I think we should try the other way round.

[npostavs]

I think part of the problem is which newline characters we make invisible.

That would refer to the choice mentioned in 83b59be, right?

There is one difference; when hiding a section body, we do *not* hide
the newline at the end of the heading but *do* hide the newline at the
end of the body - `outline-flag-region' does the opposite. That means
that hiding the body does not remove the background color of parts of
the heading - and that we cannot display an ellipsis at the "end of the
heading" (that wouldn't be visible).

I had noticed that outline-flag-region's method doesn't suffer from the problem reported in #1771. But it causes some difficulties in highlighting, cf #1771 (comment)

[tarsius]

I think we should try the other way round.

I have been thinking the same, but not because I think it makes more sense. I think it makes less sense (and has drawbacks (e.g. the background color of headings of collapsed sections would not be in affect all the way to the right edge of the window anymore)), but this is how Org-mode does it (I assume for historic reasons) - so some of the edge cases have already been taken care of.

Anyway changing this would be a major undertaking with unknown consequences - there might be more issues after the switch. So I won't do that just because of some feeling that things might be better if we do.

However I do intend to rewrite the section code at some point, and then this will certainly be on my mind. That rewrite will entail much more than this though, so I won't do it anytime soon. This will likely involve switching to using EIEIO and the main goal will be to make it easier to create sections asynchronously and to cache washed sections.

[tarsius]

So let's talk about short- to mid-term solutions to this issue.

I think instead of forcing some user settings trying to avoid the problem, we should try to find the cause of the problem. Is it a bug in emacs, or do we do something wrong?

At least on an abstract level the cause is clear. As you said

Such problems are typical when using overlays making buffer parts invisible.

There are edge cases and Magit hits a few of those. Views on whether these problems exist because Magit abuses overlays or whether we just use the only method available to do the things we want to do, are divided.

It's a process and will require having lengthy conversations with the Emacs maintainers, and since I am trying to take a break right now and also don't have a prototype of the next-generation section code handy yet, I don't want to enter that now.

So in the short run I don't think we have alternatives to disable the features that trigger the issues. Like setting goal-column to 0. If simply not doing that fixes the issue, then I would prefer that approach. We can of course also investigate where exactly that setting causes the issue, and then maybe adjust that so that it no longer does, like we have done in some of the issues mentioned above. But that has the potential of being a lot of work, with not that much benefit.

[michael-heerdegen]

Jonas Bernoulli notifications@github.com writes:

So in the short run I don't think we have alternatives to disable the
features that trigger the issues. Like setting goal-column to 0. If
simply not doing that fixes the issue, then I would prefer that
approach. We can of course also investigate where exactly that setting
causes the issue, and then maybe adjust that so that it no longer
does, like we have done in some of the issues mentioned above. But
that has the potential of being a lot of work, with not that much
benefit.

I would like to discuss this on emacs-dev. When the problem happens,
point is located inside the invisible text. I think that should not
happen.

Dunno what the correct solution for Magit would be. If you give me some
more time, I try to experiment a bit more.

[tarsius]

I would like to discuss this on emacs-dev.

I once opened an issue about this, https://debbugs.gnu.org/cgi/bugreport.cgi?bug=19188, but did not follow through. Turns out I still have some draft replies sitting around though ;-)

(At one point I talk about C-p C-f when actually I mean C-u C-f.)

[michael-heerdegen]

Thanks. Stefan made another report after closing your 19188, https://debbugs.gnu.org/cgi/bugreport.cgi?bug=19200. That's the same issue as this one I think. It's even the same with evaluating M-: (point) two times moving point out of the invisible text.

[michael-heerdegen]

I investigated a bit more.

This issue is definitely caused by a bug in Emacs: line movement ends up at an invisible position, which should not happen.

The relevant code is all in simple.el, AFAIK, moving out of invisible regions is all done from Lisp. That code is all very complex, and this bug is a bit exotic and doesn't happen very often, so I somewhat doubt it would be fixed soon, but who knows.

Alternatively, we could try to make use of the new cursor-sensor.el package in Emacs 25. If I modify magit-section-hide to give the overlay a non-nil cursor-intangible property, and enable cursor-intangible-mode, point still lands inside the overlay (I guess it shouldn't), but if you hit s, this is corrected before the command is run, so at least staging happens as expected. Hmm, still a bit fragile...

We could also use post-command-hook to move out of the invisible region, but that would not be nice.

I have also a fix btw: in line-move-to-column, at the beginning, replace

  (if (zerop col)
      (beginning-of-line)
    (move-to-column col))

with just

(move-to-column col)

The problem is that I have no idea why this works and if it makes any sense...

[michael-heerdegen]

Hmm, Stefan meant it would best if we would use something like cursor-sensor. Would that be acceptable? The package itself will be only available for emacs 25.

Here is a short demonstration how it works: in some buffer, e.g.

(insert (propertize "a b" 'cursor-intangible t))

and then enable `cursor-intangible-mode' and move around.

AFAICT this mode moves the cursor out of an intangible area in the same direction that point had been moved while running the last command, i.e. forward upto after the end when we came from before that area etc.

[michael-heerdegen]

Eli has joined the discussion and suggested to create another report. I'll do that, maybe he can help.

[michael-heerdegen]

It's bug#23079.

[michael-heerdegen]

Mmh, Stefan has closed the bug, but with good arguments.

But apparently, if we would use the invisible text property instead of the invisible overlay property, the problem could be avoided by manipulating property stickinesses.

[michael-heerdegen]

Ah: "For overlays, you need to use the insertion-type of the beg/end marker.
See the FRONT-ADVANCE and REAR-ADVANCE args of make-overlay." I'll try out tomorrow if that works for us.

[michael-heerdegen]

But because the behavior is reverse for overlays, we are already doing it correctly.

But I've now found something interesting: magit-section-update-highlight seems to interfere with the normal cursor adjustment. When I (setq-local post-command-hook '(t)), line movement works correctly! Yeah!

[npostavs]

Hmm, that's interesting. Removing just magit-diff-highlight from magit-section-highlight-hook makes the staging work correctly (though the highlighting glitch still occurs).

[michael-heerdegen]

Noam Postavsky notifications@github.com writes:

Hmm, that's interesting. Removing just magit-diff-highlight from
magit-section-highlight-hook makes the staging work correctly (though
the highlighting glitch still occurs).

Thanks! Then this is hopefully one of the last missing pieces of the
puzzle. Because as far as I see, magit-diff-highlight calls
magit-diff-paint-hunk, a function that changes text properties in the
buffer (or are there even more?).

This counts as changing the buffer, and changing the buffer in
post-command-hook suppresses cursor adjustment (said Stefan in an answer
in my bug report.)

The highlighting glitch is expected AFAICT, because post-command-hook
runs before cursor adjustment.

[michael-heerdegen]

magit-diff-paint-hunk seems to be the culprit. I'm curious why it uses text props and not overlays like all the other highlighting stuff...?

[michael-heerdegen]

Ok, seems this is not easy to fix.

Maybe consider a temporary workaround, like the following:

(define-minor-mode magit-move-out-of-invisible-mode
  ""
  nil nil nil
  (if magit-move-out-of-invisible-mode
      (add-hook 'post-command-hook #'magit-move-out-of-invisible nil t)
    (remove-hook 'post-command-hook #'magit-move-out-of-invisible t)))

(defvar-local magit-move-out-of-invisible--last-point nil)

(defun magit-move-out-of-invisible ()
  (when (and magit-move-out-of-invisible--last-point
             (invisible-p (point)))
    (if (< (point) magit-move-out-of-invisible--last-point)
        (while (invisible-p (point))
          (forward-line -1))
      (while (invisible-p (point))
        (forward-line +1))))
  (setq magit-move-out-of-invisible--last-point
        (if (markerp magit-move-out-of-invisible--last-point)
            (move-marker magit-move-out-of-invisible--last-point (point))
          (copy-marker (point)))))

This repositions point based on a very simple heuristic that can sometimes guess wrong. At least, WYSIWYG, and highlighting works correctly.

[tarsius]

Ok, seems this is not easy to fix.

I could have told you that right from the start ;-) We have already invested a lot of time into this.

Maybe consider a temporary workaround, like the following:

Since given the current implementation there only appear to be problems given certain unusual (for Magit buffers) settings, I don't intend to add any hacks to Magit.

---

I think it is justified that we use text properties to highlight hunks but overlays for other sections because for hunks we would need way more than the usual one or two overlays.

But I am somewhat surprised that this even matters when moving to a collapsed file section. I think the hunk highlighting code should not run in that case and avoiding to do so would also give a performance improvement. I'll look into that.

[tarsius]

Please check whether moving a closing paren from the second to last line of magit-diff-paint-hunk to the last line fixes the issue.

[npostavs]

Please check whether moving a closing paren from the second to last line of magit-diff-paint-hunk to the last line fixes the issue.

Nope. I can't see why that should help though.

modified   lisp/magit-diff.el
@@ -1972,8 +1972,8 @@ (cl-defun magit-diff-paint-hunk
                (if highlight 'magit-diff-removed-highlight 'magit-diff-removed))
               (t
                (if highlight 'magit-diff-context-highlight 'magit-diff-context))))
-            (forward-line))))))
-  (magit-diff-update-hunk-refinement section))
+            (forward-line)))))
+    (magit-diff-update-hunk-refinement section)))

 (defun magit-diff-paint-whitespace (merging)
   (when (and magit-diff-paint-whitespace

But I am somewhat surprised that this even matters when moving to a collapsed file section. I think the hunk highlighting code should not run in that case

It only runs when the goal column is set to 0. It seems that magit-current-section gives the wrong result because of the misplaced point.

[michael-heerdegen]

Jonas Bernoulli notifications@github.com writes:

Since given the current implementation there only appear to be
problems given certain unusual (for Magit buffers) settings, I don't
intend to add any hacks to Magit.

Here we have different opinions. I don't consider setting goal-column
-> 0 globally unusual; OTOH, I find turning on visual-line-mode
(i.e. ignoring a user setting) a hack. My approach at least doesn't do
that, and so far works ok independently from user settings.

I think it is justified that we use text properties to highlight hunks
but overlays for other sections because for hunks we would need way
more than the usual one or two overlays.

Which would decrease performance, yes.

But I am somewhat surprised that this even matters when moving to a
collapsed file section. I think the hunk highlighting code should not
run in that case and avoiding to do so would also give a performance
improvement.

Agreed.

[michael-heerdegen]

Jonas Bernoulli notifications@github.com writes:

Please check whether moving a closing paren from the second to last
line of magit-diff-paint-hunk to the last line fixes the issue.

I tried with two parens, since moving one parent doesn't change the
semantics of the function. Unfortunately with no avail.

[tarsius]

@michael-heerdegen In the past we have used similar hacks to what you are proposing here. They caused more issues than they solved. That's why I don't want to add your proposed mode to Magit. In the past we had many issues due to weird cursor positions, and when we fixed a new variation, then that usually caused breakage in other cases. Since "you" (i.e. everyone who sets goal-column globally) are the only one who is affected by this, I do not want to make a change which risks breaking this again for many more users.

Please install the kludge you proposed locally instead (well I suspect you already did that).