dogears-within-function and dogears--within return incorrect Within value

[chansey97]

Reproduce steps:

1. Open bug-dogears.el with

   (defun test1 () 1)
   
   (defun test2 () 2)

2. Move cursor to the 1st position of the 2nd line, like |(defun test2 () 2)

3. M-x dogears-remember

4. M-x dogear-list

The expect behavior shows:

#  ✓ Relevance  Within                    Line                        Buffer                 Mode            Pos
0  ✓ definition   (defun test2 () 2)  (defun test2 () 2)   bug-dogears.el  emacs-lisp    21 ...

The actual behavior shows:

#  ✓ Relevance  Within                   Line                        Buffer                  Mode            Pos
0  ✓ definition   (defun test1 () 1)  (defun test2 () 2)   bug-dogears.el  emacs-lisp    21 ...

Obviously, there is a problem with the Within value.

The root cause is there are two bugs in calculating Within

1. The dogears--which-function always returns nil (the 1st bug).

   You can try M-x eval-expression with (dogears--which-function) in the step 3 to check dogears--which-function returns nil.

2. When dogears--which-function return nil, dogears--place falls back to dogears--within (which has the 2nd bug that causes the unexpected behavior).

   You can try M-x eval-expression with (dogears--within) in the step 3 to check it returns the unexpected Within value.

The suggested patches:

For the 1st bug, let (add-log-current-defun-function nil) in dogears--which-function

(defun dogears--which-function ()
  "Call `which-function' while preventing it from using `add-log-current-defun'."
  (cl-letf (((symbol-function 'add-log-current-defun) #'ignore)
            (add-log-current-defun-function nil)) ; <--- PATCH HERE
    (which-function)))

Frankly, I don't know why you have to ignore add-log-current-defun? Deleting ((symbol-function 'add-log-current-defun) #'ignore) also works. Nevertheless, I follow you convention.

More details see https://github.com/emacs-mirror/emacs/blob/667ca66481c74325f3c8e4391d185ee547fdbb36/lisp/progmodes/which-func.el#L338

Note that to fix the example above only, this patch is enough.

For the 2st bug, add (end-of-defun) to dogears--within.

(defun dogears--within ()
  "Return string representing what the current place is \"within\"."
  (cl-case major-mode
    (Info-mode
     ;; HACK: To make Info buffer records more useful, return nil so the
     ;; bookmark name is used.
     nil)
    (otherwise (ignore-errors
                 (save-excursion
                   (end-of-defun) ; <--- PATCH HERE
                   (beginning-of-defun)
                   (buffer-substring (point-at-bol) (point-at-eol)))))))

Due to the 1st patch, this fallback situation is now rarely encountered. Also, this patch isn't perfect (you know that if the cursor at the last position of defun). Anyway, I add it since most of the time, the cursor to be in the first position of a defun is very common, e.g. xref-find-definitions.

P.s. In order to make dogears better fit my own workflow, I have made many changes to dogears. I would submit separate issues and enhancement patches in the future. So don't care about the initial motivation at the moment.

These are just two isolated bugs.

Thanks.

[alphapapa]

Hello,

Thanks for reporting this. I don't recall why I wrote those functions that way, and git doesn't seem to reveal the answers, either. That was probably in an early stage of the library's development when I was trying to get it working quickly. The APIs I was trying to make use of were a bit awkward, and still are, so the code seems a bit hacky (e.g. the way beginning-of-defun both moves to the beginning of the defun at point, as well as moving back to the previous defun if called again, makes it awkward to use for non-interactive purposes). It might be appropriate to revisit that area of functionality now.

I don't plan to work on this soon, as I have a lot going on, but I'll plan to get to it eventually.