Re: popup menu support for smerge-mode

[Masatake YAMATO]

> >> 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.
> 
I still insist on the simple popup menu.

The big menu (on menu bar) and the small popup menu have different scope.

The big menu provides rather buffer global operations. 
(e.g. next conflict, previous conflict => buffer global operations)
So "next conflict" and "previous conflict" should be top of the bgi menu.

In other hand, the small popup menu provides operations targeting on ONE
conflict. The scope of the small popup menu is narrower than that of the
big menu.


> > 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 this. So I added "More..." menu item which shows the big menu item
to the small popup menu.

> >> 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.

I've failed to notice this important issue.

In order to over come this situation, I've jsut added "Delete this" to the 
popup menu. `smerge-delete-current' and `smerge-concat-match-strings' do the
job. However, I have no confidence about their implementation.
I've just concatenate the two conflict areas other than the area where the 
mouse cursor is...

>>> 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.

I think applying highlight and keymap to the same region is not so
complex....probably I have not understood the intent of your
suggestions.

>> +  "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."
I've done this.

>> +  (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.

I've removed smerge-reset-all-overlays. Instead I use evaporate property
of overlays. When an overlay is created, I put t to evaporate property of 
the overlay. The overlay will be deleted when its region becomes empty.

> > +  (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.

I've done all.
Regards,

2003-09-19  Masatake YAMATO  <address@hidden>

        * smerge-mode.el (smerge-overlays): New variable.
        (smerge-mode-popup-menu): New menu.
        (smerge-auto-leave): Return nil if a conflict is found.
        (smerge-keep-current-by-mouse): New function.
        (smerge-put-overlay): New function.
        (smerge-delete-overlays): New function.
        (smerge-mode): Add code to manage overlays.
        (smerge-keep-all, smerge-resolve, smerge-keep-base)
        (smerge-keep-other, smerge-keep-mine, smerge-keep-current): 
        invoke `smerge-reset-all-overlays'.
        (smerge-reset-all-overlays): New function.
        (smerge-put-overlays): New function.
        (smerge-popup-menu-map): New key map.
        (smerge-delete-current): New function.
        (smerge-concat-match-strings): New function.
        (smerge-activate-popup-menu): New function.

cvs diff: warning: unrecognized response `access control disabled, clients can 
connect from any host' from cvs server
Warning: Remote host denied X11 forwarding.

Index: lisp/smerge-mode.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/smerge-mode.el,v
retrieving revision 1.22
diff -u -r1.22 smerge-mode.el
--- lisp/smerge-mode.el 1 Sep 2003 15:45:14 -0000       1.22
+++ lisp/smerge-mode.el 19 Sep 2003 09:20:13 -0000
@@ -159,6 +159,21 @@
      :help "Use Ediff to resolve the conflicts"]
     ))
 
+(defvar smerge-overlays nil "Overlays managed by smerge-mode")
+(easy-mmode-defmap smerge-popup-menu-map
+  `((,smerge-command-prefix . ,smerge-basic-map)
+    ([down-mouse-3] . smerge-activate-popup-menu))
+  "Keymap for popup menu appeared on conflicts area.")
+(easy-menu-define smerge-mode-popup-menu smerge-popup-menu-map
+  "Popup menu for `smerge-mode'."
+  '(nil
+    ["Keep This"   smerge-keep-current :help "Use current (at point) version"]
+    ["Delete This" smerge-delete-current :help "Delete current (at point) 
version"]
+    ["Keep All"    smerge-keep-all :help "Keep all three versions"]
+    "---"
+    ["More..." (popup-menu smerge-mode-menu) :help "Show full SMerge mode 
menu"]
+    ))
+
 (defconst smerge-font-lock-keywords
   '((smerge-find-conflict
      (1 smerge-mine-face prepend t)
@@ -199,11 +214,13 @@
     (error (format "No `%s'" (aref smerge-match-names n)))))
 
 (defun smerge-auto-leave ()
+  "If no conflict left, turn off Smerge mode.
+Return non-nil if the mode was indeed turned off."
   (when (and smerge-auto-leave
             (save-excursion (goto-char (point-min))
                             (not (re-search-forward smerge-begin-re nil t))))
-    (smerge-mode -1)))
-
+    (smerge-mode -1)
+    t))
 
 (defun smerge-keep-all ()
   "Keep all three versions.
@@ -301,6 +318,43 @@
       (replace-match (match-string i) t t)
       (smerge-auto-leave))))
 
+(defun smerge-delete-current ()
+  "Delete the current (under the cursor) version."
+  (interactive)
+  (smerge-match-conflict)
+  (let ((i 3) newstr)
+    (while (or (not (match-end i))
+              (< (point) (match-beginning i))
+              (>= (point) (match-end i)))
+      (decf i))
+    (if (<= i 0) (error "Not inside a version")
+      (setq newstr (smerge-concat-match-strings (delete i '(1 2 3))))
+      (replace-match newstr t t)
+      (smerge-auto-leave))))
+
+(defun smerge-concat-match-strings (match-indexes)
+  "Concatenated match-strings taking their buffer position into account.
+MATCH-INDEXES is a list. MATCH-INDEXES should hold two integers(i, j). 
+ (match-string i) and (match-string j) are concatenated."
+  (let ((i (nth 0 match-indexes))
+       (j (nth 1 match-indexes)))
+    (cond
+     ((and (null i) (null j))
+      "")
+     ((or (null i) (null (match-end i)) (= 0 (length (match-string i))))
+      (or (match-string j) ""))
+     ((or (null j) (null (match-end j)) (= 0 (length (match-string j))))
+      (or (match-string i) ""))
+     ((< (match-end i) (match-end j))
+      (concat (match-string i) (match-string j)))
+     ((> (match-end i) (match-end j))
+      (concat (match-string j) (match-string i)))
+     ((<= (length (match-string i)) (length (match-string j)))
+      (match-string j))
+     (t
+      (match-string i)))))
+     
+
 (defun smerge-diff-base-mine ()
   "Diff 'base' and 'mine' version in current conflict region."
   (interactive)
@@ -316,6 +370,53 @@
   (interactive)
   (smerge-diff 1 3))
 
+(defun smerge-reset-all-overlays ()
+  "Delete all overlays of smerge-mode and put overlays on the all conflicts 
again."
+  (smerge-delete-overlays)
+  (save-excursion 
+    (goto-char (point-min))
+    (while (smerge-find-conflict)
+      (smerge-put-overlays (cddr (match-data))))))
+
+(defun smerge-put-overlays (match-data)
+  "Put overlays of smerge-mode on the place specified by MATCH-DATA."
+  (let (b e)
+    (while match-data
+      (setq b (car match-data)
+           e (cadr match-data)
+           match-data (cddr match-data))
+      (if (and b e (not (eq b e)))
+         (smerge-put-overlay b e)))))
+
+(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."
+  (let ((overlay (make-overlay start end)))
+    (overlay-put overlay 'evaporate t)
+    (overlay-put overlay 'help-echo "down-mouse-3: Show popup menu")
+    (overlay-put overlay 'keymap smerge-popup-menu-map)
+    (push overlay smerge-overlays)))
+
+(defun smerge-activate-popup-menu (event)
+  (interactive "e")
+  (with-current-buffer (window-buffer 
+                       (posn-window (event-end event)))
+    (save-excursion
+      (let (o)
+       (goto-char (posn-point (event-end event)))
+       (setq o (car (overlays-at (point))))
+       (unwind-protect
+           (progn
+             (overlay-put o 'face 'region)
+             (sit-for 0)               ; redisplay
+             (popup-menu smerge-mode-popup-menu))
+         (overlay-put o 'face nil))))))
+
+(defun smerge-delete-overlays ()
+  "Delete all overlays made by  `smerge-put-overlay'."
+  (mapc 'delete-overlay smerge-overlays)
+  (setq smerge-overlays nil))
+
 (defun smerge-match-conflict ()
   "Get info about the conflict.  Puts the info in the `match-data'.
 The submatches contain:
@@ -522,6 +623,12 @@
   "Minor mode to simplify editing output from the diff3 program.
 \\{smerge-mode-map}"
   nil " SMerge" nil
+  ;; overlays management
+  (if smerge-mode
+      ;; entering smerge-mode
+      (make-variable-buffer-local 'smerge-overlays)
+    ;; leaving smerge-mode
+    (smerge-delete-overlays)) 
   (when (and (boundp 'font-lock-mode) font-lock-mode)
     (set (make-local-variable 'font-lock-multiline) t)
     (save-excursion
@@ -531,7 +638,8 @@
       (goto-char (point-min))
       (while (smerge-find-conflict)
        (save-excursion
-         (font-lock-fontify-region (match-beginning 0) (match-end 0) nil))))))
+         (font-lock-fontify-region (match-beginning 0) (match-end 0) nil)
+         (smerge-put-overlays (cddr (match-data))))))))
 
 
 (provide 'smerge-mode)