Re: popup menu support for smerge-mode

[Stefan Monnier]

>> Why not just reuse the existing menu ?

> Because smerge-mode-popup-menu is much smaller than smerge-mode-menu.
> I assume that user may do following two things frequently:
> 1. keep the version under the cursur. The user can(and want to) point out 
>    the version which one wants to keep by mouse. "Keep Other" and "Keep Yours"
>    are not needed here.
> 2. keep all.

I'd then suggest to reorder the main menu so that `keep current'
and `keep all' are placed at the top.

> Of course, other menu items might be useful. But as far as trying, above two
> items are `core functions' and are enough many situations. Other functions
> in smerge-mode-menu should be invoked from M-x, C-c ^ or menu bar, I think.

But reusing the same menu has the advantage that if the user wants
to use some other function, she can do so without resorting to
the keyboard.

> I agree. I've changed highlight the area when and where mouse-3 is pressed.

Good idea.
 
>> If base-start = base-end you end up with an empty overlay that the
>> user cannot click on.  
> I see. Now empty overlay is never put on.

The problem was not so much that you end up with an empty overlay.
The problem instead is that there is no place for the user to click on.
I don't know how "average" I am, but I very often need to select an empty
alternative, so with your code, I'd have to resort to the keyboard for
that case.

>> It's simpler to just place a single overlay over
>> the whole conflict (including markers).

> I have not done as you wrote.
> I expect "Keep Current" works on the region which is highlighted.

The highlight does not have to apply to the same region as the
`keymap' overlay.

> +  "Turn smerge-mode off and return t if no conflict is found.
> +Just return nil if a conflict is found."

  "If no conflict left, turn off Smerge mode.
Return non-nil if the mode was indeed turned off."

> +  (unless (smerge-auto-leave)
> +    (smerge-reset-all-overlays)))

On large files, this can take a while and force redisplay of the whole
buffer.  I'd much rather just "remove this one overlay" instead of "remove
all and place them all back, except for this one".

Also maybe it's worth it to create a smerge-replace-match while does
the replace-match&auto-leave&remove-overlay.
 
> +  (save-excursion
> +    (set-buffer (window-buffer (posn-window (event-end event))))

Aka (with-current-buffer (window-buffer (posn-window (event-end event)))

> +      (overlay-put overlay 'keymap (let ((km (make-sparse-keymap)))

Maybe it's better to create the keymap once and store it in
smerge-popup-menu-map.

> +                                      (save-excursion
> +                                        (set-buffer (window-buffer 
> (posn-window (event-end event))))

Again, I suggest `with-current-buffer'.

> +                                        (save-excursion
> +                                          (let (o)
> +                                            (goto-char (posn-point 
> (event-end event)))
> +                                            (setq o (car (overlays-at 
> (point))))
> +                                            (overlay-put o 'face 'region)
> +                                            (sit-for 0) ; redisplay
> +                                            (popup-menu 
> smerge-mode-popup-menu)
> +                                            (overlay-put o 'face nil))))))

Since you've moved point to event-end when you call `popup-menu',
I think that you can directly use `smerge-keep-current' in the menu
and throw away `smerge-keep-current-by-mouse'.

Note: in case the user hits C-g or some error occurs somewhere,
it's better to do

  (unwind-protect
      (progn
        (overlay-put o 'face 'region)
        (sit-for 0) ; redisplay
        (popup-menu smerge-mode-popup-menu))
    (overlay-put o 'face nil))))))

> +  (if smerge-mode
> +      ;; entering smerge-mode
> +      (set (make-variable-buffer-local 'smerge-overlays) nil)

If smerge-mode was already ON, this ends up throwing away the list of
overlays already present, so you'll never remove them any more.
Better just do (make-variable-buffer-local 'smerge-overlays), or even
just make-variable-buffer-local at top level.

> -      (while (smerge-find-conflict)
> +      (while (smerge-find-conflict nil)

Looks like a left over from the old patch.


        Stefan