bug#14225: [PATCH] Winner mode fails to run mark activation and deactivation hooks

bug#14225: [PATCH] Winner mode fails to run mark activation and deactivation hooks

[Kelly Dean]

[-- Attachment #1: Type: text/plain, Size: 961 bytes --]

Using Emacs 24.3
emacs -Q
(winner-mode 1)
(add-hook 'deactivate-mark-hook (lambda () (setq cursor-type t)))
(add-hook 'activate-mark-hook (lambda () (setq cursor-type 'bar)))
C-x 2
C-c left

Notice that the cursor type is now a bar, not a block. It should be a block.
The attached winnerbug.patch fixes it.

BTW, by default in Emacs, when the region is active but empty, there's no visual indication of this status. Neither is there indication of the active region if point is one less than mark and blink-cursor-mode is off. Also, if point is greater than mark, the active region is highlighted, but the block cursor does an inverse-video highlight of the character following the region, which (speaking from experience) an Emacs newbie finds distracting, since it seems to indicate that that character is also part of the region. Setting the cursor type to bar when the region is active solves all those problems, and I recommend it as the default for Emacs.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: winnerbug.patch --]
[-- Type: text/x-diff; name="winnerbug.patch", Size: 634 bytes --]

--- emacs-24.3/lisp/winner.el
+++ emacs-24.3/lisp/winner.el
@@ -309,7 +309,9 @@
                  for entry = (cadr (assq buf winner-point-alist))
                  do (progn (set-buffer buf)
                            (set-mark (car entry))
-                           (setf (winner-active-region) (cdr entry)))))
+                           (setf (winner-active-region) (cdr entry))
+			   (if (cdr entry) (run-hooks 'activate-mark-hook)
+			     (run-hooks 'deactivate-mark-hook)))))
       ;; Delete windows, whose buffers are dead or boring.
       ;; Return t if this is still a possible configuration.
       (or (null xwins)

bug#14225: [PATCH] Winner mode fails to run mark activation and deactivation hooks

[Stefan Monnier]

> Notice that the cursor type is now a bar, not a block. It should be a block.
> The attached winnerbug.patch fixes it.

Thanks.  I installed a slightly different patch which uses higher-level
functions instead.

> BTW, by default in Emacs, when the region is active but empty, there's no
> visual indication of this status. Neither is there indication of the active
> region if point is one less than mark and blink-cursor-mode is off. Also, if
> point is greater than mark, the active region is highlighted, but the block
> cursor does an inverse-video highlight of the character following the
> region, which (speaking from experience) an Emacs newbie finds distracting,
> since it seems to indicate that that character is also part of the
> region. Setting the cursor type to bar when the region is active solves all
> those problems, and I recommend it as the default for Emacs.

I'd never thought about it, but it does sound like a good idea.
This said, I see two problems with it:
- we'll probably bump into more bugs along the lines of this one
  (should all be fixable, but).
- every once will be exposed to this change, and someone out there is
  bound to dislike it.
So could you send a separate email about it to emacs-devel?  No need to
participate in the ensuing flamewar, but it's important to let people
use their flamethrowers to "let it out" ;-)


        Stefan


=== modified file 'lisp/ChangeLog'
--- lisp/ChangeLog	2013-04-18 06:24:10 +0000
+++ lisp/ChangeLog	2013-04-18 13:08:17 +0000
@@ -1,3 +1,10 @@
+2013-04-18  Stefan Monnier  <monnier@iro.umontreal.ca>
+
+	* winner.el (winner-active-region): Use region-active-p, activate-mark
+	and deactivate-mark (bug#14225).
+
+	* simple.el (deactivate-mark): Don't inline it.
+
 2013-04-18  Tassilo Horn  <tsdh@gnu.org>
 
 	* files.el (auto-mode-alist): Delete OpenDocument and StarOffice

=== modified file 'lisp/simple.el'
--- lisp/simple.el	2013-04-18 00:12:33 +0000
+++ lisp/simple.el	2013-04-18 13:05:44 +0000
@@ -4177,7 +4177,7 @@
       (marker-position (mark-marker))
     (signal 'mark-inactive nil)))
 
-(defsubst deactivate-mark (&optional force)
+(defun deactivate-mark (&optional force)
   "Deactivate the mark.
 If Transient Mark mode is disabled, this function normally does
 nothing; but if FORCE is non-nil, it deactivates the mark anyway.

=== modified file 'lisp/winner.el'
--- lisp/winner.el	2013-01-01 09:11:05 +0000
+++ lisp/winner.el	2013-04-18 13:04:51 +0000
@@ -45,10 +45,8 @@
                         (if (featurep 'xemacs)
                             `(if ,store (zmacs-activate-region)
                                (zmacs-deactivate-region))
-                          `(setq mark-active ,store)))))
-  (if (boundp 'mark-active)
-      mark-active
-    (region-active-p)))
+                          `(if ,store (activate-mark) (deactivate-mark))))))
+  (region-active-p))
 
 (defalias 'winner-edges
   (if (featurep 'xemacs) 'window-pixel-edges 'window-edges))