Re: Selection changes

[David De La Harpe Golden <david@harpegolden.net>]

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

On 17/07/10 05:13, Chong Yidong wrote:
> Chong Yidong<cyd@stupidchicken.com>  writes:
>
>> David De La Harpe Golden<david@harpegolden.net>  writes:
>>
>>> The bad "(not (eq (region-beginning) (region-end)))" check is still
>>> present in deactivate-mark (~simple.el line 3690) and should just be
>>> removed, I did [try to] explain the problem with it in a previous
>>> mail, that's a further source of some  poor behaviour (perhaps
>>> counterintuitively, but that's lazy stuff for you) that might be
>>> related to some of your points below.
>>
>> Let's come up with a proper fix, first.
>
> To be precise, the check is necessary because otherwise mouse-1 (which
> deactivates the mark) destroys the primary selection.
>


Okay, just saw rev. 100841:

Do you still think the remaining zero-length problems (still present in 
certain situations) are worth addressing?

Personally I'm not all that bothered by them, but I've been using an 
emacs with select-active-regions on for some time before any attempt to 
address them, so may have adjusted to avoid gotchas.

Anyway, I did say I'd propose something:

My initial stab wound up fugly, negating the efficiency of the lazy 
buffer query approach, doing buffer-substring ops before every command
to freeze off a copy of the region out of paranoia.

*** Then I hit upon an extra pair of markers to freeze the 
pre-command-execution extent of the region before each command, since 
x-set-selection can also take a cons of markers to lazily query.

It seems to work quite well, and doesn't buffer-substring gratuitously. 
I've yet to convince myself my actual implementation is 100% correct and 
it may be still kind of ugly, but shouldn't be grossly inefficient.

On the plus side, with the patch, C-SPC C-SPC sequences and clumsy 
clicks no longer nuke primary.

Anyway, see attached.  Perhaps not quite suitable for inclusion in its 
current form, but maybe the approach/idea is basically viable?

I have included debugging messages in the patch in its current state: It 
may be useful to split your window to show *Messages* and (setq 
select-active-region-debugging t) with it applied and try some 
selections. Or just confusing.

Other dumb issue: turns out I don't know how to make C code use 
defcustom variables - really, the calls introduced in the command loop 
need to be avoided if at all possible, given where they are, and so 
should also be guarded by !NILP select-active-regions, but apparently 
declaring the variable on the C side is the wrong thing to do.








[-- Attachment #2: select-active-regions_nozerosized_r1.diff --]
[-- Type: text/x-patch, Size: 9655 bytes --]

=== modified file 'lisp/simple.el'
--- lisp/simple.el	2010-07-17 20:21:51 +0000
+++ lisp/simple.el	2010-07-18 16:14:28 +0000
@@ -3667,35 +3667,67 @@
     (signal 'mark-inactive nil)))
 
 (defcustom select-active-regions t
   "If non-nil, an active region automatically becomes the window selection."
   :type 'boolean
   :group 'killing
   :version "24.1")
 
 (declare-function x-selection-owner-p "xselect.c" (&optional selection))
 
+;; internal state tracking vars for select-active-regions
+(defvar select-active-region-deferred nil)
+(defvar select-active-region-frozen nil)
+(defvar select-active-region-frozen-start (make-marker))
+(defvar select-active-region-frozen-end (make-marker))
+
+(defvar select-active-region-debugging nil)
+
+(defun select-active-region-debug (at)
+  (when select-active-region-debugging
+    (message "sar %s: m: %s d: %s f: %s R: %s..%s F: %s..%s"
+             at
+             mark-active
+             select-active-region-deferred
+             select-active-region-frozen
+             (if (and (mark t) mark-active) (region-beginning) "nil")
+             (if (and (mark t) mark-active) (region-end) "nil")
+             (marker-position select-active-region-frozen-start)
+             (marker-position select-active-region-frozen-end))))
+
 ;; Many places set mark-active directly, and several of them failed to also
 ;; run deactivate-mark-hook.  This shorthand should simplify.
 (defsubst deactivate-mark (&optional force)
   "Deactivate the mark by setting `mark-active' to nil.
 Unless FORCE is non-nil, this function does nothing if Transient
 Mark mode is disabled.
 This function also runs `deactivate-mark-hook'."
+  (select-active-region-debug "dea-m 1")
   (when (or transient-mark-mode force)
     ;; Copy the latest region into the primary selection, if desired.
-    (and select-active-regions
-	 mark-active
-	 (display-selections-p)
-	 (x-selection-owner-p 'PRIMARY)
-	 (x-set-selection 'PRIMARY (buffer-substring-no-properties
-				    (region-beginning) (region-end))))
+    (when (and select-active-regions
+	       mark-active
+	       (display-selections-p)
+	       (x-selection-owner-p 'PRIMARY))
+      (if select-active-region-frozen
+          (and (marker-position select-active-region-frozen-start)
+               (marker-position select-active-region-frozen-end)
+               (eq (marker-buffer select-active-region-frozen-start)
+                   (current-buffer))
+               (x-set-selection 'PRIMARY (buffer-substring-no-properties
+                                          select-active-region-frozen-start
+                                          select-active-region-frozen-end)))
+      (x-set-selection 'PRIMARY (buffer-substring-no-properties
+				 (region-beginning) (region-end)))))
+    (select-active-region-debug "dea-m 2")
+    (setq select-active-region-frozen nil) ; well, it is still potentially "frozen", but string-frozen not marker-frozen.
+    (setq select-active-region-deferred nil)
     (if (and (null force)
 	     (or (eq transient-mark-mode 'lambda)
 		 (and (eq (car-safe transient-mark-mode) 'only)
 		      (null (cdr transient-mark-mode)))))
 	;; When deactivating a temporary region, don't change
 	;; `mark-active' or run `deactivate-mark-hook'.
 	(setq transient-mark-mode nil)
       (if (eq (car-safe transient-mark-mode) 'only)
 	  (setq transient-mark-mode (cdr transient-mark-mode)))
       (setq mark-active nil)
@@ -3704,23 +3736,58 @@
 (defun activate-mark ()
   "Activate the mark."
   (when (mark t)
     (setq mark-active t)
     (unless transient-mark-mode
       (setq transient-mark-mode 'lambda))
     (select-active-region)))
 
 (defsubst select-active-region ()
   "Set the PRIMARY X selection if `select-active-regions' is non-nil."
+  (select-active-region-debug "sar 1")
   (and select-active-regions
        (display-selections-p)
-       (x-set-selection 'PRIMARY (current-buffer))))
+       (if (eq (region-beginning) (region-end))
+           (setq select-active-region-deferred t)
+         (progn
+           (setq select-active-region-deferred nil)
+           (setq select-active-region-frozen nil)
+           (x-set-selection 'PRIMARY (current-buffer)))))
+  (select-active-region-debug "sar 2"))
+
+
+(defun select-active-region-precommand ()
+  (select-active-region-debug "pre 1")
+  (when select-active-region-deferred
+    (select-active-region))
+  (when (and select-active-regions
+             mark-active
+             (display-selections-p)
+             (x-selection-owner-p 'PRIMARY))
+    (when (not (eq (region-beginning) (region-end)))
+      (set-marker select-active-region-frozen-start (region-beginning))
+      (set-marker select-active-region-frozen-end (region-end)))
+    (when (not (eq (marker-position select-active-region-frozen-start)
+                   (marker-position select-active-region-frozen-end)))
+      (x-set-selection 'PRIMARY (cons select-active-region-frozen-start
+				      select-active-region-frozen-end))
+      (setq select-active-region-frozen t)))
+  (select-active-region-debug "pre 2"))
+
+
+(defun select-active-region-postcommand ()
+  (select-active-region-debug "post 1")
+  ;; maybe thaw after each command.
+  (when (or select-active-region-deferred select-active-region-frozen)
+    (select-active-region))
+  (select-active-region-debug "post 2"))
+
 
 (defun set-mark (pos)
   "Set this buffer's mark to POS.  Don't use this function!
 That is to say, don't use this function unless you want
 the user to see that the mark has moved, and you want the previous
 mark position to be lost.
 
 Normally, when a new mark is set, the old one should go on the stack.
 This is why most applications should use `push-mark', not `set-mark'.
 

=== modified file 'src/keyboard.c'
--- src/keyboard.c	2010-07-13 10:57:00 +0000
+++ src/keyboard.c	2010-07-18 16:14:28 +0000
@@ -385,20 +385,26 @@
 Lisp_Object Qinput_method_function;
 
 /* When we call Vinput_method_function,
    this holds the echo area message that was just erased.  */
 Lisp_Object Vinput_method_previous_message;
 
 /* Non-nil means deactivate the mark at end of this command.  */
 Lisp_Object Vdeactivate_mark;
 Lisp_Object Qdeactivate_mark;
 
+/* Commands are bracketed with these.
+ */
+Lisp_Object Qselect_active_region_precommand;
+Lisp_Object Qselect_active_region_postcommand;
+
+
 /* Menu bar specified in Lucid Emacs fashion.  */
 
 Lisp_Object Vlucid_menu_bar_dirty_flag;
 Lisp_Object Qrecompute_lucid_menubar, Qactivate_menubar_hook;
 
 Lisp_Object Qecho_area_clear_hook;
 
 /* Hooks to run before and after each command.  */
 Lisp_Object Qpre_command_hook, Vpre_command_hook;
 Lisp_Object Qpost_command_hook, Vpost_command_hook;
@@ -1692,25 +1698,30 @@
 	{
 	  Lisp_Object cmd1;
 	  if (cmd1 = Fcommand_remapping (cmd, Qnil, Qnil), !NILP (cmd1))
 	    cmd = cmd1;
 	}
 
       /* Execute the command.  */
 
       Vthis_command = cmd;
       real_this_command = cmd;
+
+      if (!NILP (current_buffer->mark_active) && !NILP (Vrun_hooks))
+        call0 (Qselect_active_region_precommand);
+
       /* Note that the value cell will never directly contain nil
 	 if the symbol is a local variable.  */
       if (!NILP (Vpre_command_hook) && !NILP (Vrun_hooks))
 	safe_run_hooks (Qpre_command_hook);
 
+
       already_adjusted = 0;
 
       if (NILP (Vthis_command))
 	{
 	  /* nil means key is undefined.  */
 	  Lisp_Object keys = Fvector (i, keybuf);
 	  keys = Fkey_description (keys, Qnil);
 	  bitch_at_user ();
 	  message_with_string ("%s is undefined", keys, 0);
 	  current_kboard->defining_kbd_macro = Qnil;
@@ -1781,20 +1792,22 @@
 	  if (!CONSP (last_command_event))
 	    current_kboard->Vlast_repeatable_command = real_this_command;
 	  cancel_echoing ();
 	  this_command_key_count = 0;
 	  this_command_key_count_reset = 0;
 	  this_single_command_key_start = 0;
 	}
 
       if (!NILP (current_buffer->mark_active) && !NILP (Vrun_hooks))
 	{
+          call0 (Qselect_active_region_postcommand);
+
 	  /* In Emacs 22, setting transient-mark-mode to `only' was a
 	     way of turning it on for just one command.  This usage is
 	     obsolete, but support it anyway.  */
 	  if (EQ (Vtransient_mark_mode, Qidentity))
 	    Vtransient_mark_mode = Qnil;
 	  else if (EQ (Vtransient_mark_mode, Qonly))
 	    Vtransient_mark_mode = Qidentity;
 
 	  if (!NILP (Vdeactivate_mark))
 	    call0 (Qdeactivate_mark);
@@ -12059,20 +12072,26 @@
 
   DEFVAR_LISP ("deactivate-mark", &Vdeactivate_mark,
 	       doc: /* If an editing command sets this to t, deactivate the mark afterward.
 The command loop sets this to nil before each command,
 and tests the value when the command returns.
 Buffer modification stores t in this variable.  */);
   Vdeactivate_mark = Qnil;
   Qdeactivate_mark = intern_c_string ("deactivate-mark");
   staticpro (&Qdeactivate_mark);
 
+  Qselect_active_region_precommand = intern_c_string ("select-active-region-precommand");
+  staticpro (&Qselect_active_region_precommand);
+
+  Qselect_active_region_postcommand = intern_c_string ("select-active-region-postcommand");
+  staticpro (&Qselect_active_region_postcommand);
+
   DEFVAR_LISP ("command-hook-internal", &Vcommand_hook_internal,
 	       doc: /* Temporary storage of `pre-command-hook' or `post-command-hook'.  */);
   Vcommand_hook_internal = Qnil;
 
   DEFVAR_LISP ("pre-command-hook", &Vpre_command_hook,
 	       doc: /* Normal hook run before each command is executed.
 If an unhandled error happens in running this hook,
 the hook value is set to nil, since otherwise the error
 might happen repeatedly and make Emacs nonfunctional.  */);
   Vpre_command_hook = Qnil;