Re: popup menu support for smerge-mode

["Stefan Monnier" <monnier+gnu/emacs@cs.yale.edu>]

> +(easy-menu-define smerge-mode-popup-menu smerge-mode-map
> +  "Popup menu for `smerge-mode'."
> +  '(nil
> +    ["Keep Current" smerge-keep-current-by-mouse :help "Use current (at point) version"]
> +    ["Keep All" smerge-keep-all :help "Keep all three versions"]
> +    ))

Why not just reuse the existing menu ?

>  (defun smerge-auto-leave ()
> -  (when (and smerge-auto-leave
> -	     (save-excursion (goto-char (point-min))
> -			     (not (re-search-forward smerge-begin-re nil t))))
> +  (smerge-remove-overlays)
> +  (when (and 
> +	 ;; 1. Are conflict existed?
> +	 ;; As the side effect, overlays are put.
> +	 (not (save-excursion 
> +		(goto-char (point-min))
> +		(let (matched)
> +		  (while (smerge-find-conflict nil t)
> +		    (setq matched t))
> +		  matched)))
> +	 ;; 2. Check customize option.
> +	 smerge-auto-leave)
>      (smerge-mode -1)))

This is clearly wrong.  smerge-auto-leave is about leaving smerge-mode
when all conflicts are resolved.  It shouldn't fiddle with overlays at all.
The overlays should be removed by the call to (smerge-mode -1).

> +(defun smerge-put-overlay (start end)
> +  "Put overlay of smerge-mode between START and END.
> +The overlay has its own keymap to show popup menu."
> +  (setq overlay (make-overlay start end))

You're modifying a global `overlay' variable here.  Please let-bind it.

> +  (overlay-put overlay 'mouse-face 'highlight)

Having tried it, I'm not sure whether this is a good idea or not:
when the conflict is large, you end up with the whole window flashing
in `highlight' face in a very annoying way.

> +  (overlay-put overlay 'keymap (let ((km (make-sparse-keymap)))
> +				 (define-key km [down-mouse-3]
> +				   (lambda () (interactive)
> +				     (popup-menu smerge-mode-popup-menu)))
> +				 km))
> +  (overlay-put overlay 'help-echo "down-mouse-3: Use current (at point) version")

The help-echo is wrong: mouse-3 does not select the version under the
cursor (it pops up a menu instead).

> +(defun smerge-remove-overlays ()
> +  "Delete all overlays made by  `smerge-put-overlay'."
> +  (mapcar (lambda (o) (delete-overlay o)) smerge-overlays)
> +  (setq smerge-overlays nil))

When the output of `mapcar' is not used you should use `mapc'.
(lambda (x) (f x)) should always be replaced by 'f.

> +	  (when put-overlay
> +	    (if (and base-start base-end)
> +		(smerge-put-overlay base-start base-end))
> +	    (if (and mine-start mine-end)
> +		(smerge-put-overlay mine-start mine-end))
> +	    (if (and other-start other-end)
> +		(smerge-put-overlay other-start other-end)))

If base-start = base-end you end up with an empty overlay that the
user cannot click on.  It's simpler to just place a single overlay over
the whole conflict (including markers).

Also, I'm not sure it's a good idea to have a `match' function add an
overlay like that.  Matching is not supposed to have such side effects.
It's probably better to let the few callers that need it call
smerge-put-overlay themselves.


	Stefan