Re: bug in 23.2.92 with anything
Thierry Volpiatto
>> in 23.2.92 of january 14, when i start anything and enter a character in
>> minibuffer, cursor switch to beginning of line in minibuffer.
>> In 23.2.91 (the last commit of the january 13) this doesn't happen.
>> In 24.0.50 of january 15 (today) this doesn't happen too.
>> So i wonder what changes have been made, possibly in minibuffer related
>> code, on the january 14 in emacs-23 branch.(couldn't find in changelog)
>
> In Fselect_window of window.c after the clause
>
> if (not_selected_before)
> {
> sf = SELECTED_FRAME ();
> if (XFRAME (WINDOW_FRAME (w)) != sf)
> ...
> selected_window = window;
> }
Sorry, i can't find such code.
> there should be the two lines
>
> else
> inhibit_point_swap = 0;
>
> I don't have Emacs 23 here so I can't check whether they are but somehow
> the diffs on emacs-diffs seem to miss them. Could you please have a look
> and, if they are missing, put them there, reinstall and try whether the
> bug is gone?
>
> Thanks, martin
--
A+ Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997
martin rudalics
Unfortunately no. You did what I meant.
> If yes, be more explicit, as my C skill is around 0.
As soon as I have found out why and how I changed the code in my branch
earlier.
Just to know: When I call `anything' here I get point after some prompt
like "nil" or "pattern" in the minibuffer. Is this what is expected?
martin
Stefan Monnier
> if (not_selected_before)
> {
> sf = SELECTED_FRAME ();
> if (XFRAME (WINDOW_FRAME (w)) != sf)
> ...
> selected_window = window;
> }
> there should be the two lines
> else
> inhibit_point_swap = 0;
Actually, using a global variable like this is butt-ugly.
Could some one rename Fselect_window to something else, add a third
parameter to it and create a new Fselect_window that calls it (and let
Fset_window_configuration call the new function with the new param), so
as to get rid of this global var?
Stefan
Thierry Volpiatto
Yes but after:
Here a cursor: !
the prompt is like this:
Pattern: !
i enter the letter a in prompt:
Pattern: !a
the cursor jump before the a
it should stay like this:
Pattern: a!
martin rudalics
>
> the prompt is like this:
>
> Pattern: !
>
> i enter the letter a in prompt:
>
> Pattern: !a
>
> the cursor jump before the a
>
> it should stay like this:
>
> Pattern: a!
I can't reproduce this with the current trunk (which includes the
change) and emacs -Q. The cursor always stays after the letters I
entered. Can you reproduce it with emacs -Q?
martin
Eli Zaretskii
> From: martin rudalics <ruda...@gmx.at>
> CC: Stefan Monnier <mon...@iro.umontreal.ca>, emacs...@gnu.org,
> thierry....@gmail.com
>
> >> Could some one rename Fselect_window to something else, add a third
> >> parameter to it and create a new Fselect_window that calls it (and let
> >> Fset_window_configuration call the new function with the new param), so
> >> as to get rid of this global var?
> >
> > discussions that this is THE right solution), but I'm traveling now
> > and will be back only a week from now. If no one beats me to it, I
> > will make this change.
>
> I posted a patch along these lines - please tell me whether it is what
> you had in mind.
Yes, it is. Thanks.
> I'm almost 100% convinced that it doesn't DTRT. That parameter would
> have to be passed on to Fselect_frame and from there back to
> Fselect_window. As I said earlier, the problem is that we temporarily
> violate the invariant window_frame (selected_window) == selected_frame.
IIUC, you are saying that there will be still additional problems left
even after this change. If so, I might agree, but what we are trying
to do here is to avoid the situation whereby selected_window is nil,
because that bombs inside window_text_bottom_y. Using the parameter
allows us to communicate to select-window that it should not swap out
point, without setting selected_window to nil. Would you agree that
this part of the problem is fixed by the change you posted? If not,
please explain why not.
Thierry Volpiatto
>> Actually, using a global variable like this is butt-ugly.
>
> Indeed.
>
>> Could some one rename Fselect_window to something else, add a third
>> parameter to it and create a new Fselect_window that calls it (and let
>> Fset_window_configuration call the new function with the new param), so
>> as to get rid of this global var?
>
> Thierry could you try the attached patch against the trunk window.c?
Your patch don't apply correctly, could you make it with git and tell me
on which revision number it apply?
> Thanks, martin
> === modified file 'src/window.c'
> *** src/window.c 2011-01-15 23:16:57 +0000
> --- src/window.c 2011-01-16 10:18:51 +0000
> ***************
> *** 85,90 ****
> --- 85,91 ----
> int (* fn) (struct window *, void *),
> void *);
> static Lisp_Object window_list_1 (Lisp_Object, Lisp_Object, Lisp_Object);
> + static Lisp_Object select_window (Lisp_Object, Lisp_Object, int);
>
> /* This is the window in which the terminal's cursor should
> be left when nothing is being done with it. This must
> ***************
> *** 158,168 ****
>
> static int window_initialized;
>
> - /* Set in `set-window-configuration' to prevent "swapping out point"
> - in the old selected window. */
> -
> - static int inhibit_point_swap;
> -
> /* Hook to run when window config changes. */
>
> static Lisp_Object Qwindow_configuration_change_hook;
> --- 159,164 ----
> ***************
> *** 3550,3569 ****
> return Qnil;
> }
>
> - /* Note that selected_window can be nil when this is called from
> - Fset_window_configuration. */
>
> ! DEFUN ("select-window", Fselect_window, Sselect_window, 1, 2, 0,
> ! doc: /* Select WINDOW. Most editing will apply to WINDOW's buffer.
> ! If WINDOW is not already selected, make WINDOW's buffer current
> ! and make WINDOW the frame's selected window. Return WINDOW.
> ! Optional second arg NORECORD non-nil means do not put this buffer
> ! at the front of the list of recently selected ones and do not
> ! make this window the most recently selected one.
>
> ! Note that the main editor command loop selects the buffer of the
> ! selected window before each command. */)
> ! (register Lisp_Object window, Lisp_Object norecord)
> {
> register struct window *w;
> register struct window *ow;
> --- 3546,3560 ----
> return Qnil;
> }
>
>
> ! /* If select_window is called with inhibit_point_swap non-zero it will
> ! not store point of the old selected window's buffer back into that
> ! window's pointm slot. This is needed by Fset_window_configuration to
> ! avoid that the display routine is called with selected_window set to
> ! Qnil causing a subsequent crash. */
>
> ! static Lisp_Object
> ! select_window (Lisp_Object window, Lisp_Object norecord, int inhibit_point_swap)
> {
> register struct window *w;
> register struct window *ow;
> ***************
> *** 3603,3611 ****
> /* Store the current buffer's actual point into the
> old selected window. It belongs to that window,
> and when the window is not selected, must be in the window. */
> ! if (inhibit_point_swap)
> ! inhibit_point_swap = 0;
> ! else
> {
> ow = XWINDOW (selected_window);
> if (! NILP (ow->buffer))
> --- 3594,3600 ----
> /* Store the current buffer's actual point into the
> old selected window. It belongs to that window,
> and when the window is not selected, must be in the window. */
> ! if (! inhibit_point_swap)
> {
> ow = XWINDOW (selected_window);
> if (! NILP (ow->buffer))
> ***************
> *** 3639,3644 ****
> --- 3628,3648 ----
> return window;
> }
>
> + DEFUN ("select-window", Fselect_window, Sselect_window, 1, 2, 0,
> + doc: /* Select WINDOW. Most editing will apply to WINDOW's buffer.
> + If WINDOW is not already selected, make WINDOW's buffer current
> + and make WINDOW the frame's selected window. Return WINDOW.
> + Optional second arg NORECORD non-nil means do not put this buffer
> + at the front of the list of recently selected ones and do not
> + make this window the most recently selected one.
> +
> + Note that the main editor command loop selects the buffer of the
> + selected window before each command. */)
> + (register Lisp_Object window, Lisp_Object norecord)
> + {
> + select_window (window, norecord, 0);
> + }
> +
> static Lisp_Object
> select_window_norecord (Lisp_Object window)
> {
> ***************
> *** 6167,6174 ****
> out point" in the old selected window using the buffer that
> has been restored into it. We already swapped out that point
> from that window's old buffer. */
> ! inhibit_point_swap = 1;
> ! Fselect_window (data->current_window, Qnil);
> XBUFFER (XWINDOW (selected_window)->buffer)->last_selected_window
> = selected_window;
>
> --- 6171,6177 ----
> out point" in the old selected window using the buffer that
> has been restored into it. We already swapped out that point
> from that window's old buffer. */
> ! select_window (data->current_window, Qnil, 1);
> XBUFFER (XWINDOW (selected_window)->buffer)->last_selected_window
> = selected_window;
>
> ***************
> *** 7099,7106 ****
> window_scroll_preserve_hpos = -1;
> window_scroll_preserve_vpos = -1;
>
> - inhibit_point_swap = 0;
> -
> DEFVAR_LISP ("temp-buffer-show-function", &Vtemp_buffer_show_function,
> doc: /* Non-nil means call as function to display a help buffer.
> The function is called with one argument, the buffer to be displayed.
> --- 7102,7107 ----
martin rudalics
>> change) and emacs -Q. The cursor always stays after the letters I
>> entered. Can you reproduce it with emacs -Q?
> I have parsed my .emacs.el, and that start to happen after loading elscreen.el.
I downloaded Elscreen but it has a dependency on static.el which I can't
find. Do these compile correctly with Emacs 24?
Also with current trunk could you try changing the following part of
Fselect_window
if (EQ (window, selected_window))
return window;
to
if (EQ (window, selected_window))
{
inhibit_point_swap = 0;
return window;
}
and look whether the behavior changes back?
Thanks, martin
martin rudalics
> on which revision number it apply?
For some reason it had CRLF line endings, maybe that's the cause. I'll
attach it with a Unix line endings version I just applied to my trunk.
Generally, it should apply to latest Emacs 23 and 24 sources. My bzr
revision number is 102868.
martin
Thierry Volpiatto
> Also with current trunk could you try changing the following part of
> Fselect_window
>
> if (EQ (window, selected_window))
> return window;
>
> to
>
> if (EQ (window, selected_window))
> {
> inhibit_point_swap = 0;
> return window;
> }
>
> and look whether the behavior changes back?
Here what i changed:
,----
| if (NILP (norecord))
| {
| ++window_select_count;
| XSETFASTINT (w->use_time, window_select_count);
| }
|
| /* if (EQ (window, selected_window)) */
| /* return window; */
| if (EQ (window, selected_window))
| {
| inhibit_point_swap = 0;
| return window;
| }
|
`----
I have applied these changes on emacs-23 branch after this commit:
,----
| commit d0c73d59998f4686dbb2a0d7b05e16d7161fe75d
| Author: Stefan Monnier <mon...@iro.umontreal.ca>
| Date: Sun Jan 16 10:40:47 2011 -0500
|
| * image.c (syms_of_image): Don't access XSYMBOL's internals directly.
`----
and it seem to work fine :-)
Thanks.
Thierry Volpiatto
>> Which behavior?
>> On Emacs24, all work fine.
>
> Strange. The change from Emacs 23 has been propagated to the trunk
> in the meantime.
Sorry i was not up to date,
now i have updated the trunk to:
,----
| commit 43c8d47880f4103a5c8f8b2439cae0efa3338228
| Merge: 758a4ff c1c5267
| Author: Paul Eggert <egg...@cs.ucla.edu>
| Date: Sun Jan 16 23:46:36 2011 -0800
|
| * xfns.c (x_real_positions): Fix signedness of local var 'ign'.
`----
and confirm the bug still here.
>> But maybe you mean the head of 23.2.92 by trunk?
>
> Please try it there.
>
>> So where do you want i apply the changes?
>> on 23.2.92 or 24.0.50?
>> (just to be sure we don't misunderstand us)
>
> I'm afraid you don't have the latest sources of the trunk.
Yes i was not up to date.
>So please
> try everything on Emacs 23.2.92.
>
> martin
Eli Zaretskii
> From: martin rudalics <ruda...@gmx.at>
> CC: mon...@iro.umontreal.ca, emacs...@gnu.org,
> thierry....@gmail.com
>
> >> > /* When running redisplay, we play a bit fast-and-loose and allow e.g.
> >> > selected_frame and selected_window to be temporarily out-of-sync so
> >> > when we come back here via `goto retry', we need to resync because we
> >> > may need to run Elisp code (via prepare_menu_bars). */
> >> > select_frame_for_redisplay (old_frame);
> >> >
> >> > It would be good to get rid of such risky code.
> >>
> >> Is there any reason why this should not select the window too?
> >
> > By "this" you mean what? IOW, who or what should "select the window
> > too"?
>
> This code. That is, select_frame_for_redisplay.
What would be the utility of doing so? The redisplay will next call
redisplay_windows, which walks the entire window tree and redisplays
each one of them (temporarily selecting its buffer in the process).
How would selecting the frame's selected window help anything in this
procedure? I won't expect selected_window to play any significant
role in the redisplay process, because it redraws all windows.
martin rudalics
>>>> > selected_frame and selected_window to be temporarily out-of-sync so
>>>> > when we come back here via `goto retry', we need to resync because we
>>>> > may need to run Elisp code (via prepare_menu_bars). */
>>>> > select_frame_for_redisplay (old_frame);
>>>> >
>>>> > It would be good to get rid of such risky code.
>>>>
>>>> Is there any reason why this should not select the window too?
>>> By "this" you mean what? IOW, who or what should "select the window
>>> too"?
>> This code. That is, select_frame_for_redisplay.
>
> What would be the utility of doing so? The redisplay will next call
> redisplay_windows, which walks the entire window tree and redisplays
> each one of them (temporarily selecting its buffer in the process).
> How would selecting the frame's selected window help anything in this
> procedure? I won't expect selected_window to play any significant
> role in the redisplay process, because it redraws all windows.
I don't know enough about redisplay. IIUC select_frame_for_redisplay is
used mainly to set up frame local variables for displaying all buffers
shown in that frame. In display_mode_lines selected window and selected
frame are both temporarily deselected but in update_tool_bar only the
selected frame is deselected (just as in select_frame_for_redisplay).
If code is run in between, the fact that the selected window is not on
the selected frame might be surprising. But I don't know what any code
run by update_tool_bar or redisplay_internal really needs in this
context. So if you say it doesn't matter I take your word for it.
martin
Thierry Volpiatto
>> Here what i changed:
>>
>> ,----
>> | if (NILP (norecord))
>> | {
>> | ++window_select_count;
>> | XSETFASTINT (w->use_time, window_select_count);
>> | }
>> |
>> | /* if (EQ (window, selected_window)) */
>> | /* return window; */
>> | if (EQ (window, selected_window))
>> | {
>> | inhibit_point_swap = 0;
>> | return window;
>> | }
>> |
>> `----
>>
>> I have applied these changes on emacs-23 branch after this commit:
>>
>> ,----
>> | commit d0c73d59998f4686dbb2a0d7b05e16d7161fe75d
>> | Author: Stefan Monnier <mon...@iro.umontreal.ca>
>> | Date: Sun Jan 16 10:40:47 2011 -0500
>> |
>> | * image.c (syms_of_image): Don't access XSYMBOL's internals directly.
>> `----
>>
>> and it seem to work fine :-)
>> Thanks.
>
> So this should hopefully fix my initial proposal. Could you also try
> the other patch I sent you? You would have to revert to the state of
> the repository for that purpose.
It still doesn't apply correctly, could you make a patch in git style on
top of:(i use a git repo)
,----[ emacs-23 branch ]
| commit d0c73d59998f4686dbb2a0d7b05e16d7161fe75d
| Author: Stefan Monnier <mon...@iro.umontreal.ca>
| Date: Sun Jan 16 10:40:47 2011 -0500
|
| * image.c (syms_of_image): Don't access XSYMBOL's internals directly.
`----
--
martin rudalics
>
> ,----
> | if (NILP (norecord))
> | {
> | ++window_select_count;
> | XSETFASTINT (w->use_time, window_select_count);
> | }
> |
> | /* if (EQ (window, selected_window)) */
> | /* return window; */
> | if (EQ (window, selected_window))
> | {
> | inhibit_point_swap = 0;
> | return window;
> | }
> |
> `----
>
> I have applied these changes on emacs-23 branch after this commit:
>
> ,----
> | commit d0c73d59998f4686dbb2a0d7b05e16d7161fe75d
> | Author: Stefan Monnier <mon...@iro.umontreal.ca>
> | Date: Sun Jan 16 10:40:47 2011 -0500
> |
> | * image.c (syms_of_image): Don't access XSYMBOL's internals directly.
> `----
>
> and it seem to work fine :-)
> Thanks.
So this should hopefully fix my initial proposal. Could you also try
the other patch I sent you? You would have to revert to the state of
the repository for that purpose.
martin