appointment display during isearch replaces buffer contents with history-element

appointment display during isearch replaces buffer contents with history-element

[Martin Fischer]

Please write in English if possible, because the Emacs maintainers
usually do not have translators to read other languages for them.

Your bug report will be posted to the bug-gnu-emacs@gnu.org mailing list,
and to the gnu.emacs.bug news group.

Please describe exactly what actions triggered the bug
and the precise symptoms of the bug:

  8<-------------------------------------------------------------------

  Start isearch-forward, type M-p and scroll the history buffer with
  <up> (previous-history-element).

  If at the same time emacs tries to display an appointment in the
  same frame, the contents of the buffer, where you wanted to isearch,
  is replaced by one of the history elements.

  The buffer may be restored by undo.

  8<-------------------------------------------------------------------

If Emacs crashed, and you have the Emacs process in the gdb debugger,
please include the output from the following gdb commands:
    `bt full' and `xbacktrace'.
If you would like to further debug the crash, please read the file
d:/aemf/emacs/etc/DEBUG for instructions.


In GNU Emacs 22.1.1 (i386-mingw-nt5.1.2600)
 of 2007-06-02 on RELEASE
Windowing system distributor `Microsoft Corp.', version 5.1.2600
configured using `configure --with-gcc (3.4) --cflags -Ic:/gnuwin32/include'

Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: DEU
  locale-coding-system: cp1252
  default-enable-multibyte-characters: t

Major mode: Lisp Interaction

Minor modes in effect:
  encoded-kbd-mode: t
  tooltip-mode: t
  tool-bar-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  unify-8859-on-encoding-mode: t
  utf-translate-cjk-mode: t
  auto-compression-mode: t
  line-number-mode: t

Recent input:
C-w <right> C-s C-w <right> C-s C-w <right> <f6> <f6> 
<f6> <f6> <f6> <f6> <f6> <lwindow> | <C-kp-2> <C-kp-2> 
<C-kp-2> <C-kp-2> <C-kp-2> <C-kp-2> <C-kp-2> <C-kp-2> 
<C-kp-2> <C-kp-2> <C-kp-2> <C-kp-2> <C-kp-2> <C-kp-2> 
<C-kp-2> <C-kp-2> <C-kp-2> <C-kp-2> <C-kp-2> <C-kp-2> 
<tab> <help-echo> <escape> x <up> <up> <return> <up> 
<up> C-e <backspace> <backspace> 4 5 <return> t e s 
t 1 <return> <escape> x <up> <up> <return> <escape> 
x <up> <return> C-x 1 <escape> x <up> <return> <escape> 
x <up> <return> <help-echo> <help-echo> <escape> <f12> 
<help-echo> <escape> x <up> <up> <return> 1 4 : 5 4 
<backspace> 5 <return> t e s t 2 <backspace> 3 <return> 
<escape> x <up> <up> <return> C-s <escape> p <up> <up> 
<up> <up> <up> <up> <down> <down> <down> <down> <down> 
<down> <down> <up> <up> <up> <up> <up> <up> <up> <up> 
<up> <up> <up> <down> <down> <down> <down> <down> <down> 
<down> <down> <down> <down> C-s C-w <right> C-s C-w 
<right> C-s C-w <right> C-s C-w <right> C-s C-w <right> 
C-s C-w <right> C-s C-w <right> C-s C-w <right> C-s 
C-w <right> <escape> x <up> <return> C-x 1 <escape> 
x c u s t o m <tab> - v a <tab> <return> a p p t _ 
<tab> <backspace> - <tab> d <tab> i <tab> <tab> a <tab> 
<backspace> l <tab> i <tab> <return> <down-mouse-1> 
<mouse-1> <backspace> 1 <help-echo> <down-mouse-1> 
<lwindow> C-x b <return> C-s <escape> x <up> <up> <return> 
<escape> x <up> <return> C-x 1 C-s <escape> p p <up> 
<up> <up> <up> <up> <up> <up> <up> <up> <up> <up> <up> 
<up> <up> <up> <up> <up> <up> <up> <down> <down> <down> 
<down> <down> <down> <down> <down> <down> <down> <help-echo> 
<help-echo> <help-echo> <help-echo> <help-echo> <help-echo> 
<help-echo> <help-echo> <help-echo> <help-echo> <help-echo> 
<help-echo> <help-echo> <help-echo> <help-echo> <help-echo> 
<menu-bar> <help-menu> <report-emacs-bug> C-g <help-echo> 
<help-echo> <help-echo> <help-echo> <help-echo> <help-echo> 
<help-echo> <help-echo> <menu-bar> <help-menu> <re
port-emacs-bug>

Recent messages:
Loading easymenu...done
Loading cus-edit...done
Loading help-fns...done
Making completion list... [3 times]
Creating customization items...
Creating customization items ...done
Resetting customization items...done
Creating customization setup...done
Preparing diary...done
Mark saved where search started
Quit

Re: appointment display during isearch replaces buffer contents with history-element

[Johan Bockgård]

This report was not very detailed and I don't know how to reproduce the
bug. Meanwhile, I found this broken code in appt-disp-window

  (if (equal (selected-window) (minibuffer-window))
      (if (other-window 1)
          (select-window (other-window 1))
        (if (display-multi-frame-p)
            (select-frame (other-frame 1)))))

This code makes no sense. other-window and other-frame select a window
or frame--they don't return it. In fact, the return value is always nil,
so this can't possibly be right.

-- 
Johan Bockgård

Re: appointment display during isearch replaces buffer contents with history-element

[martin rudalics]

>   (if (equal (selected-window) (minibuffer-window))
>       (if (other-window 1)
>           (select-window (other-window 1))
>         (if (display-multi-frame-p)
>             (select-frame (other-frame 1)))))
> 
> This code makes no sense. other-window and other-frame select a window
> or frame--they don't return it. In fact, the return value is always nil,
> so this can't possibly be right.
> 

Does it work if you use

   (or (not (minibufferp))
       (and (display-multi-frame-p)
	   (progn
	     (other-frame 1)
	     (not (minibufferp))))
       (other-window 1))

instead?

Re: appointment display during isearch replaces buffer contents with history-element

[Johan Bockgård]

martin rudalics <rudalics@gmx.at> writes:

> Does it work if you use
>
>   (or (not (minibufferp))
>       (and (display-multi-frame-p)
> 	   (progn
> 	     (other-frame 1)
> 	     (not (minibufferp))))
>       (other-window 1))
>
> instead?

I don't use appt, so I can't tell how it's supposed to work.

-- 
Johan Bockgård

Re: appointment display during isearch replaces buffer contents with history-element

[Martin Fischer]

  >>>>> Johan Bockgård <bojohan> writes:
  > Date: Sat, 17 Nov 2007 11:44:00 +0100
  > 
  > This report was not very detailed and I don't know how to reproduce the
  > bug.

Hi,

make an appointment with appt-add or by adding and saving an entry in
your diary.

The appointment display will appear the first time at
appt-message-warning-time minutes before the appointment and after
that every appt-display-interval minutes.

If you watch your system clock, you will be able to enter and scroll
the isearch history buffer in time.

I may add further explanations if you tell us, how you tried to
reproduce the bug.

Martin
-- 
parozusa at web dot de

Re: appointment display during isearch replaces buffer contents with history-element

[Johan Bockgård]

Martin Fischer <parozusa@web.de> writes:

>   Start isearch-forward, type M-p and scroll the history buffer with
>   <up> (previous-history-element).
>
>   If at the same time emacs tries to display an appointment in the
>   same frame, the contents of the buffer, where you wanted to isearch,
>   is replaced by one of the history elements.
>
>   The buffer may be restored by undo.

Ok. Here's a condensed recipe.

    $ emacs -Q

    (defun foo ()
      (save-excursion
        (other-window 1)))

    (push "foo" extended-command-history)

    (run-with-timer 5 nil 'foo)

M-x
Wait 5 seconds
Press <up>

The contents of the *scratch* buffer are replaced with "foo"; that is,
the key binding is looked up in the minibuffer's keymap but executed
in *scratch*.


In contrast, executing

    emacsclient --eval '(save-excursion (other-window 1))'

while Emacs is waiting at the M-x prompt doesn't confuse the keymaps.

Using this definition for foo also works correctly

    (defun foo ()
      (interactive)
      (other-window 1))


So the problem is when a timer changes the selected window, but not
the current buffer.




This is what I found

* read_process_output (and similar code in exec_sentinel); call
  record_asynch_buffer_change unconditionally (processes):

  #if 0 /* Call record_asynch_buffer_change unconditionally,
           because we might have changed minor modes or other things
           that affect key bindings.  */
        if (! EQ (Fcurrent_buffer (), obuffer)
            || ! EQ (current_buffer->keymap, okeymap))
  #endif
          /* But do it only if the caller is actually going to read events.
             Otherwise there's no need to make him wake up, and it could
             cause trouble (for example it would make sit_for return).  */
          if (waiting_for_user_input_p == -1)
            record_asynch_buffer_change ();


* wait_reading_process_output; (2 places) call
  record_asynch_buffer_change only if buffer has changed (timers):

    /* If a timer has run, this might have changed buffers
       an alike.  Make read_key_sequence aware of that.  */
    if (timers_run != old_timers_run
        && waiting_for_user_input_p == -1
        && old_buffer != current_buffer)
      record_asynch_buffer_change ();


* read_key_sequence; look for a BUFFER_SWITCH_EVENT (from
  record_asynch_buffer_change):

    if (BUFFERP (key))
      {
        timer_resume_idle ();

        mock_input = t;
        /* Reset the current buffer from the selected window
           in case something changed the former and not the latter.
           This is to be more consistent with the behavior
           of the command_loop_1.  */
        if (fix_current_buffer)
          {
            if (! FRAME_LIVE_P (XFRAME (selected_frame)))
              Fkill_emacs (Qnil);
            if (XBUFFER (XWINDOW (selected_window)->buffer) != current_buffer)
              Fset_buffer (XWINDOW (selected_window)->buffer);
          }

        orig_local_map = get_local_map (PT, current_buffer, Qlocal_map);
        orig_keymap = get_local_map (PT, current_buffer, Qkeymap);
        goto replay_sequence;
      }


* command_loop_1; set selected window's buffer as current between
  looking up and executing command:

    /* Read next key sequence; i gets its length.  */
    i = read_key_sequence (keybuf, sizeof keybuf / sizeof keybuf[0],
                           Qnil, 0, 1, 1);
    
    /* A filter may have run while we were reading the input.  */
    if (! FRAME_LIVE_P (XFRAME (selected_frame)))
      Fkill_emacs (Qnil);
    if (XBUFFER (XWINDOW (selected_window)->buffer) != current_buffer)
      set_buffer_internal (XBUFFER (XWINDOW (selected_window)->buffer));


-- 
Johan Bockgård

Re: appointment display during isearch replaces buffer contents with history-element

[Chong Yidong]

bojohan+news@dd.chalmers.se (Johan Bockgård) writes:

>     $ emacs -Q
>
>     (defun foo ()
>       (save-excursion
>         (other-window 1)))
>
>     (push "foo" extended-command-history)
>
>     (run-with-timer 5 nil 'foo)
>
> M-x
> Wait 5 seconds
> Press <up>
>
> The contents of the *scratch* buffer are replaced with "foo"; that is,
> the key binding is looked up in the minibuffer's keymap but executed
> in *scratch*.
>
> So the problem is when a timer changes the selected window, but not
> the current buffer.

The following patch fixes this problem.  Any objections?

*** emacs/src/process.c.~1.512.2.9.~  2008-01-02 08:51:29.000000000 -0500
--- emacs/src/process.c               2008-01-02 09:37:38.000000000 -0500
***************
*** 4697,4702 ****
--- 4697,4703 ----
    {
      int old_timers_run = timers_run;
        struct buffer *old_buffer = current_buffer;
+         Lisp_Object old_window = selected_window;
            int leave = 0;
  
          if (detect_input_pending_run_timers (do_display))
***************
*** 4710,4716 ****
         an alike.  Make read_key_sequence aware of that.  */
           if (timers_run != old_timers_run
                 && waiting_for_user_input_p == -1
!                      && old_buffer != current_buffer)
                           record_asynch_buffer_change ();
  
          if (leave)
--- 4711,4718 ----
         an alike.  Make read_key_sequence aware of that.  */
           if (timers_run != old_timers_run
                 && waiting_for_user_input_p == -1
!                      && (old_buffer != current_buffer
!                           || !EQ (old_window, selected_window)))
                                record_asynch_buffer_change ();
  
          if (leave)

Re: appointment display during isearch replaces buffer contents with history-element

[Glenn Morris]

Chong Yidong wrote:

> The following patch fixes this problem.  Any objections?

No objections in a week; works for me; installed with thanks.

Re: appointment display during isearch replaces buffer contents with history-element

[Johan Bockgård]

Chong Yidong <cyd@stupidchicken.com> writes:

>          an alike.  Make read_key_sequence aware of that.  */
>            if (timers_run != old_timers_run
[...]

There are two occurrences of this in wait_reading_process_output. Should
the other be changed too?

-- 
Johan Bockgård

Re: appointment display during isearch replaces buffer contents with history-element

[Johan Bockgård]

bojohan+news@dd.chalmers.se (Johan Bockgård) writes:

> Chong Yidong <cyd@stupidchicken.com> writes:
>
>>          an alike.  Make read_key_sequence aware of that.  */
>>            if (timers_run != old_timers_run
> [...]
>
> There are two occurrences of this in wait_reading_process_output. Should
> the other be changed too?

Anyone?  I'm talking about this change:

2008-01-10  Chong Yidong  <cyd@stupidchicken.com>

        * process.c (wait_reading_process_output): Check for window
        changes caused by timers.

wait_reading_process_output contains two conditionals that may run
record_asynch_buffer_change.  The check for window changes was only
added to the latter one:

process.c:4368

    /* If a timer has run, this might have changed buffers
       an alike.  Make read_key_sequence aware of that.  */
    if (timers_run != old_timers_run
        && old_buffer != current_buffer
        && waiting_for_user_input_p == -1)
      record_asynch_buffer_change ();

process.c:4692

    /* If a timer has run, this might have changed buffers
       an alike.  Make read_key_sequence aware of that.  */
    if (timers_run != old_timers_run
        && waiting_for_user_input_p == -1
        && (old_buffer != current_buffer
        || !EQ (old_window, selected_window)))
      record_asynch_buffer_change ();


Is this intentional?

-- 
Johan Bockgård

Re: appointment display during isearch replaces buffer contents with history-element

[Chong Yidong]

bojohan+news@dd.chalmers.se (Johan Bockgård) writes:

>> There are two occurrences of this in wait_reading_process_output. Should
>> the other be changed too?
>
> Anyone?  I'm talking about this change:
>
> 2008-01-10  Chong Yidong  <cyd@stupidchicken.com>
>
>         * process.c (wait_reading_process_output): Check for window
>         changes caused by timers.
>
> wait_reading_process_output contains two conditionals that may run
> record_asynch_buffer_change.  The check for window changes was only
> added to the latter one

It looks like you're right: the first piece of code should be changed.
If no one objects, I'll do it shortly.