bug#21072: 24.5; inconsistent behaviour of `C-M-h (mark-defun)' in Emacs Lisp

[Marcin Borkowski <mbork@mbork.pl>]

On 2016-05-07, at 07:07, Drew Adams <drew.adams@oracle.com> wrote:

> I agree that the behavior is not particularly consistent, and it
> does not completely correspond to the doc.  What the best fix is,
> I don't know.
>
> It's been this way for a long time, so there might be people
> who expect or like it to do what it does, in which case the
> doc should probably be fixed somewhat.
>
> On the other hand, I'd bet that few, if any, would complain if
> better behavior were implemented.
>
> In any case, the behavior of being able to repeat to keep selecting
> more defuns further down should be kept.
>
> I'd suggest that Someone (TM) (Marcin?) implement a better
> behavior and propose it to emacs-devel. ;-)
>
> What might be better?
>
> 1. At least consistency wrt which defun gets selected, when
> betweeen defuns.  The doc suggests a general rule (the next
> defun), but that is not always respected.
>
> 2. Something consistent also wrt a comment before the defun
> that will be selected.
>
> 3. It could be good for a numeric prefix arg to select that
> many defuns.
>
> 4. It could be good for a negative prefix arg to select in
> the opposite direction.  This is the main improvement I'd
> like to see.  E.g. `M-- C-M-h' selects the previous defun;
> `M-2 C-M-h' selects the two previous defuns.
>
> Someone should play around and dream up something useful.
>
> Wrt #2, I'm not sure what the best approach might be.

Hi all,

it's been some time, and I implemented a better (?) behavior indeed.
(Yes, I know a few months passed.  But I don't have a lot of time to
work on Emacs bugs now.  This bug took me almost 8 hours over the past
few months.)

Basically, I took into consideration all the points Drew mentioned.  I'd
like to submit my work for review.  I have one technical problem,
though: I have it as a series of commits on a separate branch in my
repo, and I'm not Git-literate enough to make one patch from them.
Should I submit a series of patches?  Should I rebase my commits into
one big one?  (No big deal, this might be a good idea anyway.)

In the course of my work, I have also implemented a macro to help write
tests for the new behavior (there are 6 ERT tests, with 40 assertions
total).  Here it is (for review):

--8<---------------cut here---------------start------------->8---
(defvar elisp-test-point-marker-regex "=!\\([a-zA-Z0-9-]+\\)="
  "A regexp matching placeholders for point position for
`elisp-tests-with-temp-buffer'.")

;; Copied and heavily modified from `python-tests-with-temp-buffer'
(defmacro elisp-tests-with-temp-buffer (contents &rest body)
  "Create a `emacs-lisp-mode' enabled temp buffer with CONTENTS.
BODY is code to be executed within the temp buffer.  Point is
always located at the beginning of buffer.  Special markers of
the form =!NAME= in CONTENTS are removed, and a for each one
a variable called NAME is bound to the position of such
a marker."
  (declare (indent 1) (debug t))
  `(with-temp-buffer
     (emacs-lisp-mode)
     (insert ,contents)
     (goto-char (point-min))
     (while (re-search-forward elisp-test-point-marker-regex nil t)
       (delete-region (match-beginning 0)
		      (match-end 0)))
     (goto-char (point-min))
     ,(let (marker-list)
	(with-temp-buffer
	  (insert (cond ((symbolp contents)
                         (symbol-value contents))
                        (t contents)))
	  (goto-char (point-min))
	  (while (re-search-forward elisp-test-point-marker-regex nil t)
	    (push (list (intern (match-string-no-properties 1))
			(match-beginning 0))
		  marker-list)
	    (delete-region (match-beginning 0)
			   (match-end 0))))
	`(let ,marker-list
	   ,@body))))
--8<---------------cut here---------------end--------------->8---

And it can be used like this:

--8<---------------cut here---------------start------------->8---
(defvar mark-defun-test-buffer
  ";; Comment header
=!before-1=
\(defun func-1 (arg)
  =!inside-1=\"docstring\"
  body)
=!after-1==!before-2=
;; Comment before a defun
\(d=!inside-2=efun func-2 (arg)
  \"docstring\"
  body)
=!after-2==!before-3=
\(defun func-3 (arg)
  \"docstring\"=!inside-3=
  body)
=!after-3==!before-4=(defun func-4 (arg)
  \"docstring\"=!inside-4=
  body)
=!after-4=
;; end
"
  "Test buffer for `mark-defun'.")

(ert-deftest mark-defun-no-arg-region-inactive ()
  "Test `mark-defun' with no prefix argument and inactive
region."
  (elisp-tests-with-temp-buffer
      mark-defun-test-buffer
    ;; mark-defun inside a defun, with comments and an empty line
    ;; before
    (goto-char inside-1)
    (mark-defun)
    (should (= (point) before-1))
    (should (= (mark) after-1))))
--8<---------------cut here---------------end--------------->8---

WDYT?

-- 
Marcin Borkowski