Re: [cvs] bug when using pc-selection-mode/transient-mark-mode

[Karl Chen <quarl@hkn.eecs.berkeley.edu>]

> In the C code there are just two places that deactivate the mark:
>
>     editfns.c:802:  current_buffer->mark_active = tem;
>     keyboard.c:1755:		  current_buffer->mark_active = Qnil;
>
> Once you are at that point, could you try enabling breakpoints at
> those two places and see if they go off?

It wasn't easy because the part in keyboard.c:1755 gets called whenever
deactivate-mark is non-nil, which is set in multiple places.

The ultimate source of deactivate-mark getting set is insdel.c:1967

prepare_to_modify_buffer() {
  ...
  Vdeactivate_mark = Qt;  // line 1967
}

So the bug indeed has to do with x-own-selection and sit-for (non-nil
post-command-idle-hook). Emacs will get X selection events and in
  sit_for (sec=0, ...)            will call
  swallow_events()                which will call
  x_handle_selection_request()    which will call
  x_get_local_selection()
which will go through selection-converter-alist and end up calling
  (xselect-convert-to-string ...) which will call
  (encode-coding-string ...)
  code_convert_string1()
  encode_coding_string()
  run_pre_post_conversion_on_str()       <---
  (erase-buffer /* buffer = "*code-converting-work*" */ )
  del_range()
  del_range_1()
  prepare_to_modify_buffer()

I'm giving you all this backtracing because I'm not sure where to fix the
bug.  I can comment out insdel.c:1967 and it will go away but that was
probably there for a reason.

My best guess is that when run_pre_post_conversion_on_str() calls
buffer modification functions such as (erase-buffer) and
insert_from_string() on its temporary buffer, it doesn't mean to set the
global deactivate-mark.

This patch fixes the problem.

--- coding.c.~1.257.~	2002-09-07 21:30:13.000000000 -0700
+++ coding.c	2002-09-20 19:19:31.000000000 -0700
@@ -5819,6 +5819,7 @@
   int multibyte = STRING_MULTIBYTE (str);
   Lisp_Object buffer;
   struct buffer *buf;
+  Lisp_Object old_deactivate_mark = Vdeactivate_mark;

   record_unwind_protect (Fset_buffer, Fcurrent_buffer ());
   record_unwind_protect (code_convert_region_unwind, Qnil);
@@ -5854,6 +5855,9 @@
     }
   inhibit_pre_post_conversion = 0;
   str = make_buffer_string (BEG, Z, 1);
+
+  Vdeactivate_mark = old_deactivate_mark;
+
   return unbind_to (count, str);
 }

---

I haven't looked yet in cvs what change was responsible for the behavior
manifesting.


HOWEVER, this is an ugly hack and I've noticed a lot of similar "save
deactivate_mark; do crap; restore deactivate_mark" code in Emacs. It just
smells of bad design when global variables have to be pushed and popped
all the time and someone calling a function needs to know which variables
to push/pop, especially when this is not documented.  Some ideas for a
more elegant (i.e. a lot more troublesome to implement)  solution:

  - per-buffer `deactivate-mark' variable.
  - maybe prepare_to_modify_buffer doesn't need to set deactivate-mark
    (probably does need to).
  - a "temporary buffer" status for buffers that are such. Could be either
    passed to get-buffer-create or set as a buffer-local variable.
  - notice whether the buffer being modified by in prepare_to_MB is the
    "active" one (current buffer? or any visible buffer) that the user has
    a selection in (and if not assume it's a temporary buffer)

In the meantime there should be a documented warning to emacs developers
that whenever they create and manipulate temporary buffers that they have
to save the deactivate-mark variable.

--
Karl Chen / quarl@quarl.org