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

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

[Karl Chen]

I think some bug was introduced recently (past couple days) in cvs.

I use pc-selection-mode (i.e. shift-movement-keys = extend selection,
which is visible) and the mark gets deactivated randomly very often - to
the point of unusability. This also occurs with transient-mark-mode. It
only occurs right after a keystroke (command).

This is my post-command-hook, if it matters:
post-command-hook's value is
(force-mode-line-update t)

Local in buffer *scratch*; global value is
(tooltip-activate-mouse-motions-if-enabled
semantic-post-change-major-mode-function global-font-lock-mode-buffers
highline-highlight-current-line ecb-handle-major-mode-activation)


-- 
Karl Chen / quarl@quarl.org

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

[Richard Stallman]

    I think some bug was introduced recently (past couple days) in cvs.

I don't see any change in the past 10 days that could be relevant.
Do you recall when you did the CVS update to build the previous version of
Emacs in which you did not see this problem?

Can you send one precise self-contained test case to try?
Please read the Bugs section in the Emacs manual, which provides
guidelines on how to write a bug report to give us the
necessary information so we can fix the bug.

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

[Karl Chen]

I'm sorry I can't be any more specific. I don't remember when exactly I
updated. I also installed ECB (emacs code browser) around the same time
(though Emacs 21.2 with ECB works fine).

I can't figure out how exactly to reproduce the problem because it doesn't
manifest as soon as I start emacs. I've tried going through all the
motions I usually do (invoking different autloads through buffers/files,
compiling, autorevert, etc.) in case something autoloads globally.  All I
can say (I know this isn't much use, sorry) is that after having edited
for a while the selection-deactivation behavior starts.

-- 
Karl Chen / quarl@quarl.org


On Mon, 16 Sep 2002, Richard Stallman wrote:

>     I think some bug was introduced recently (past couple days) in cvs.
>
> I don't see any change in the past 10 days that could be relevant.
> Do you recall when you did the CVS update to build the previous version of
> Emacs in which you did not see this problem?
>
> Can you send one precise self-contained test case to try?
> Please read the Bugs section in the Emacs manual, which provides
> guidelines on how to write a bug report to give us the
> necessary information so we can fix the bug.
>

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

[Richard Stallman]

    I can't figure out how exactly to reproduce the problem because it doesn't
    manifest as soon as I start emacs. I've tried going through all the
    motions I usually do (invoking different autloads through buffers/files,
    compiling, autorevert, etc.) in case something autoloads globally.  All I
    can say (I know this isn't much use, sorry) is that after having edited
    for a while the selection-deactivation behavior starts.

In that case, I think only you are in a position to debug it.
You could put (if debugging (debug)) into the code in simple.el
that deactivates the mark, and set debugging to t when you want
to debug.

(Save all your buffers before you start debugging this way.)

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

[Karl Chen]

So I added your debugging suggestion but before I could use it, by luck I
discovered what was causing this mark dactivation bug.

It happens whenever I copy or paste text. `interprogram-cut-function' is
x-select-text, which calls x-set-selection, which calls
x-own-selection-internal.

Summary of how to reproduce this bug:
M-x pc-selection-mode
M-: (x-own-selection-internal 'PRIMARY "quack")
C-x C-f bigfile
hold down S-down.

Without the x-own-selection-internal, the selection would be from the
start of the file to whatever line you were on. After any clipboard copy
operation, the selection will at random times get deactivated and the
selection won't be ever more than a couple lines.

I don't see at all what could cause this behavior in xselect.c, nor do I
see any relevant changes recently in xselect.c. I'll check out old
snapshots of emacs to see if they have this behavior.

-- 
Karl Chen / quarl@quarl.org


On Wed, 18 Sep 2002, Richard Stallman wrote:

>     I can't figure out how exactly to reproduce the problem because it doesn't
>     manifest as soon as I start emacs. I've tried going through all the
>     motions I usually do (invoking different autloads through buffers/files,
>     compiling, autorevert, etc.) in case something autoloads globally.  All I
>     can say (I know this isn't much use, sorry) is that after having edited
>     for a while the selection-deactivation behavior starts.
>
> In that case, I think only you are in a position to debug it.
> You could put (if debugging (debug)) into the code in simple.el
> that deactivates the mark, and set debugging to t when you want
> to debug.
>
> (Save all your buffers before you start debugging this way.)
>

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

[Karl Chen]

Addition to previous message on reproducing the bug:
The bug only exists when post-command-idle-hook is not nil; for example,
I have post-command-idle-hook as (which-func-update). So add:

M-: (add-hook 'post-command-idle-hook 'ignore)

-- 
Karl Chen / quarl@quarl.org

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

[Karl Chen]

Based on the fact that post-command-idle-hook must be non-nil, I believe
the bug has to do with the following functions:

    sit_for(... display = 1)
    wait_reading_process_input()

although I don't see anything that would affect mark-active or be affected
by x_own_selection().

[BTW: it's unneccessarily (except for historic reasons) confusing that the
lisp sit-for and the C sit_for have opposite meanings for their
arguments.]

-- 
Karl Chen / quarl@quarl.org

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

[Karl Chen]

Emacs from "cvs up -D '15 days ago'" has this bug but "cvs up -D '30 days
ago'" does not.

-- 
Karl Chen / quarl@quarl.org

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

[Richard Stallman]

    sit_for(... display = 1)
    wait_reading_process_input()

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?

You can also do this

  (add-hook 'deactivate-mark-hook (lambda () (x-display-color-cells)))

That won't do anything really, but if you put a breakpoint in C
at Fx_display_color_cells, that will stop in the C debugger
when the mark is deactivated.

etc/DEBUG gives adivce on how to get a Lisp backtrace when that
happens.

    [BTW: it's unneccessarily (except for historic reasons) confusing that the
    lisp sit-for and the C sit_for have opposite meanings for their
    arguments.]

I wouldn't say they are opposite.  The C function sit_for has DISPLAY
instead of NODISP--is that what you mean?  But it also has two
additional arguments.  WIth that much difference, they are not
really comparable.  If the only difference were DISPLAY vs NODISP
them it might be good to change the C function.

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

[Richard Stallman]

    Emacs from "cvs up -D '15 days ago'" has this bug but "cvs up -D '30 days
    ago'" does not.

If you try a binary search, you could narrow it down to one day and
maybe we would be led straght to the change that caused the problem.

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

[Karl Chen]

> 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

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

[Richard Stallman]

      - per-buffer `deactivate-mark' variable.

That might be the right thing to do.

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

It could check whether the buffer is displayed in a window.

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

[Stefan Monnier]

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

`mark-active' is buffer-local and I think `deactivate-mark' should
be buffer-local as well.  But that begs the question "when do we check
whether deactivate-mark was set and reset mark-active accordingly" ?
Are we going to loop through all buffers, looking for ones with
a non-nil deactivate-mark ?
So maybe we should not make it buffer-local but instead make it hold
a list of buffers whose mark should be deactivated.
Currently we only use t or nil where t means "current-buffer".


	Stefan

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

[Karl Chen]

On Sun, 22 Sep 2002, Stefan Monnier wrote:
> `mark-active' is buffer-local and I think `deactivate-mark' should
> be buffer-local as well.  But that begs the question "when do we check
> whether deactivate-mark was set and reset mark-active accordingly" ?
> Are we going to loop through all buffers, looking for ones with
> a non-nil deactivate-mark ?
> So maybe we should not make it buffer-local but instead make it hold
> a list of buffers whose mark should be deactivated.
> Currently we only use t or nil where t means "current-buffer".

Some options for a buffer-local deactivate-mark without checking all
buffers:

1. Only check deactivate-mark of the buffer that is current at the end of
each command (at the same places it's currently checked). This is similar
to current behavior (besides avoiding the temporary buffer modification
quirks); the current code doesn't support implicit mark deactivation using
deactivate-mark on buffers besides the one that's current at the end of
the command, either.  The downside is that if deactivate-mark IS set in a
non-current buffer then the behavior later can be non-deterministic (e.g.:
start in buffer B1. command C1 sets deactivate-mark in B2 but stays in B1.
... command C2 switches to B2. B2's mark will get deactivated after C2
because at C2, the command loop only cleared B1's deactivate-mark but then
checked B2's deactivate-mark).

2. In the command loop, check only the deactivate-mark of the buffer that
was current at the beginning of the command. However, if a command
switches to another buffer, modifies the new buffer, and stays in this
buffer, then the new buffer's mark SHOULD get deactivated.

3. Check the deactivate-mark of the buffer that was current at the
beginning of the command, and if the current buffer at the end of the
command is different, check that also.

Non-automatic mark deactivation on any buffer can always be done using the
deactivate-mark /function/. The small number of commands that want to
deactivate the mark in a non-current buffer (I can't think of any right
now) perhaps should required to do so explicitly.

-- 
Karl Chen / quarl@quarl.org

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

[Miles Bader]

On Sun, Sep 22, 2002 at 06:48:20PM -0400, Stefan Monnier wrote:
> `mark-active' is buffer-local and I think `deactivate-mark' should
> be buffer-local as well.  But that begs the question "when do we check
> whether deactivate-mark was set and reset mark-active accordingly" ?
> Are we going to loop through all buffers, looking for ones with
> a non-nil deactivate-mark ?

How about something like:

  (1) `mark-active' gets set to a (global) integer counter value instead of t.

  (2) In the current place where `deactivate-mark' is checked, it just
      handles the current buffer, but also increments the global counter.

  (3) `switch-to-buffer' checks to see if `mark-active' is less than the
      global counter value, and if so, checks the local value of
      `deactivate-mark' and perhaps sets mark-active to nil.

IOW, it lazily does the deactivation based on the local value of
deactivate-mark.

Is this too expensive a thing to take place in switch-to-buffer?

> So maybe we should not make it buffer-local but instead make it hold
> a list of buffers whose mark should be deactivated.
> Currently we only use t or nil where t means "current-buffer".

Doesn't that require all the places where deactivate-mark is set to be
changed (in a non-trivial way)?  Sounds very annoying for programmers...
[and a maintainance headache]

-Miles

-- 
P.S.  All information contained in the above letter is false,
      for reasons of military security.

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

[Miles Bader]

On Sun, Sep 22, 2002 at 09:18:42PM -0400, Miles Bader wrote:
> > So maybe we should not make it buffer-local but instead make it hold
> > a list of buffers whose mark should be deactivated.
> > Currently we only use t or nil where t means "current-buffer".
> 
> Doesn't that require all the places where deactivate-mark is set to be
> changed (in a non-trivial way)?  Sounds very annoying for programmers...
> [and a maintainance headache]

Ah, I realized that people usually only `bind protect' deactivate-mark, not
actually set it to an explicit value...

-miles

-- 
P.S.  All information contained in the above letter is false,
      for reasons of military security.

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

[Richard Stallman]

    So maybe we should not make it buffer-local but instead make it hold
    a list of buffers whose mark should be deactivated.

That seems like a good idea.

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

[Stefan Monnier]

> Is this too expensive a thing to take place in switch-to-buffer?

You must mean `set-buffer' ?

> > So maybe we should not make it buffer-local but instead make it hold
> > a list of buffers whose mark should be deactivated.
> > Currently we only use t or nil where t means "current-buffer".
> 
> Doesn't that require all the places where deactivate-mark is set to be
> changed (in a non-trivial way)?

Yes, except that we can still accept t as a special value meaning
`current-buffer', such that code that's not updated will not break
Emacs (it will just make it behave as it does now, which isn't
perfect but isn't terrible either).

> Sounds very annoying for programmers... [and a maintainance headache]

But since it can be made backward-compatible it's not such a big deal.
And (as you noticed) deactivate-mark is rarely set explicitly anyway.


	Stefan

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

[Kim F. Storm]

Miles Bader <miles@gnu.org> writes:

> How about something like:
> 
>   (1) `mark-active' gets set to a (global) integer counter value instead of t.

But then lisp code which sets mark-active must be changed to DTRT...
Sounds flaky to me.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

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

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

>     So maybe we should not make it buffer-local but instead make it hold
>     a list of buffers whose mark should be deactivated.
> 
> That seems like a good idea.

So does that mean that deactivate-mark (the function) would simply add
the current-buffer to that list?

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

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

[Richard Stallman]

      (1) `mark-active' gets set to a (global) integer counter value instead of t.

      (2) In the current place where `deactivate-mark' is checked, it just
	  handles the current buffer, but also increments the global counter.

      (3) `switch-to-buffer' checks to see if `mark-active' is less than the
	  global counter value, and if so, checks the local value of
	  `deactivate-mark' and perhaps sets mark-active to nil.

    IOW, it lazily does the deactivation based on the local value of
    deactivate-mark.

    Is this too expensive a thing to take place in switch-to-buffer?

It is not too expensive, I think, but the idea to make deactivate-mark
hold a list of buffers is far far simpler and seems better to me.

    Doesn't that require all the places where deactivate-mark is set to be
    changed (in a non-trivial way)?  Sounds very annoying for programmers...
    [and a maintainance headache]

The places that just bind it to nil to ignore modifications don't need
to be changed.  The places that set it to nil don't need to be
changed.  The other references are very few.  In the toplevel directory,
I found two files that set the variable but really ought to bind it
to nil instead; I found two places (delsel.el and simple.el) that set
it to t.  Nothing else.