[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: popup menu support for smerge-mode
From: |
Stefan Monnier |
Subject: |
Re: popup menu support for smerge-mode |
Date: |
Sun, 24 Aug 2003 10:42:38 -0400 |
> +(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