wolray / symbol-overlay

Highlight symbols with keymap-enabled overlays
346 stars 42 forks source link

Fix incorrect behavior of symbol-overlay-switch-backward #108

Closed roife closed 8 months ago

roife commented 8 months ago

When executing symbol-overlay-switch-backward, it currently jumps to the first overlay in the buffer instead of to the one immediately preceding the cursor.

For dir > 0, the function symbol-overlay-get-list retrieves overlays positioned after the cursor in sequence. Conversely, when dir < 0, it fetches overlays located before the cursor in sequence. Therefore, with dir < 0, the appropriate action is to return the last overlay in the list which is just the overlay before the cursor.

purcell commented 8 months ago

An alternative (and possibly neater) solution might be this:

diff --git a/symbol-overlay.el b/symbol-overlay.el
index 453128a..e1be478 100644
--- a/symbol-overlay.el
+++ b/symbol-overlay.el
@@ -255,7 +255,7 @@ If SYMBOL is non-nil, get the overlays that belong to it.
 DIR is an integer.
 If EXCLUDE is non-nil, get all overlays excluding those belong to SYMBOL."
   (let ((overlays (cond ((= dir 0) (overlays-in (point-min) (point-max)))
-                        ((< dir 0) (overlays-in (point-min) (point)))
+                        ((< dir 0) (nreverse (overlays-in (point-min) (point))))
                         ((> dir 0) (overlays-in
                                     (if (looking-at-p "\\_>") (1- (point)) (point))
                                     (point-max))))))
roife commented 8 months ago

Yes, but I think last might be more efficient?

purcell commented 8 months ago

Kinda, but the existing order of locations returned by symbol-overlay-get-list is unnatural when dir is negative, to the extent you needed to debug this and explain why last was the correct choice. So I suspect it's better to change that function instead. Both methods are wildly inefficient, in that all the overlays in the preceding (or following) text need to be found before the closest one is jumped to: that's the real issue IMO. Really we should have symbol-overlay-get-next and symbol-overlay-get-prev functions which use next-overlay-change and previous-overlay-change to find the closest overlay in either direction.

roife commented 8 months ago

After consideration, I agree that directly modifying the function is a simpler approach, which improves the readability of the code. I have submitted a new commit.

purcell commented 8 months ago

Nice, thanks!