save-frame-excursion?

save-frame-excursion?

[Bastien]

Currently we use this in Org-mode to save frame excursion:

(defmacro org-save-frame-excursion (&rest body)
  "Eval BODY and return to the currently selected frame."
  (let ((frame-var (gensym "FRAME")))
    `(let ((,frame-var (selected-frame)))
       (unwind-protect
           (progn ,@body)
         (select-frame-set-input-focus ,frame-var)))))

This was required when selecting a date for an Org entry.  
Org has a mechanism that lets the user browse the calendar 
while the cursor is still in the minibuffer (see org-read-date).

When the calendar is displayed in a separate frame, this 
mechanism was broken because org-read-date was losing the 
focus.  

Is this a pattern that other developers already met?

I'm copying Robert, who brought this issue up and proposed 
the code above.

-- 
 Bastien

Re: save-frame-excursion?

[Stefan Monnier]

> Currently we use this in Org-mode to save frame excursion:
> (defmacro org-save-frame-excursion (&rest body)
>   "Eval BODY and return to the currently selected frame."
>   (let ((frame-var (gensym "FRAME")))
>     `(let ((,frame-var (selected-frame)))
>        (unwind-protect
>            (progn ,@body)
>          (select-frame-set-input-focus ,frame-var)))))

> This was required when selecting a date for an Org entry.
> Org has a mechanism that lets the user browse the calendar
> while the cursor is still in the minibuffer (see org-read-date).

> When the calendar is displayed in a separate frame, this
> mechanism was broken because org-read-date was losing the
> focus.

Do you know when/why it lost the focus?

What was used before this macro (i.e. which part of this macro is the
important one)?

> Is this a pattern that other developers already met?

Well, there's save-selected-window, as well as with-selected-window,
which cover similar grounds, but without knowing more details it's hard
to tell.


        Stefan

Re: save-frame-excursion?

[Robert Goldman]

Stefan Monnier wrote:
>> Currently we use this in Org-mode to save frame excursion:
>> (defmacro org-save-frame-excursion (&rest body)
>>   "Eval BODY and return to the currently selected frame."
>>   (let ((frame-var (gensym "FRAME")))
>>     `(let ((,frame-var (selected-frame)))
>>        (unwind-protect
>>            (progn ,@body)
>>          (select-frame-set-input-focus ,frame-var)))))
> 
>> This was required when selecting a date for an Org entry.
>> Org has a mechanism that lets the user browse the calendar
>> while the cursor is still in the minibuffer (see org-read-date).
> 
>> When the calendar is displayed in a separate frame, this
>> mechanism was broken because org-read-date was losing the
>> focus.
> 
> Do you know when/why it lost the focus?

The org frame lost focus because focus shifted to the frame that
contained the *Calendar* buffer.  The interaction in the *Calendar*
buffer was protected by save-excursion and save-window-excursion.  That
was enough to save the state /within/ the frame containing the org
buffer, but was /not/ enough to restore focus to the original frame.
> 
> What was used before this macro (i.e. which part of this macro is the
> important one)?
> 
>> Is this a pattern that other developers already met?
> 
> Well, there's save-selected-window, as well as with-selected-window,
> which cover similar grounds, but without knowing more details it's hard
> to tell.

I am not that experienced in this area, but as far as I can tell
save-window-excursion covers similar ground, but only controls what
window is selected /within a given frame/.  So this macro, on which I
based org-save-frame-excursion covers similar ground, but it does not
subsume this macro.

I hope this helps,

Robert

Re: save-frame-excursion?

[Stefan Monnier]

>> Do you know when/why it lost the focus?

> The org frame lost focus because focus shifted to the frame that
> contained the *Calendar* buffer.

OK, so the next question is "why/when did focus shift to the *Calendar"
buffer" (and just to be more clear by "why/when" I mean at the level of
the Elisp code, which trail of calls causes it).

> I am not that experienced in this area, but as far as I can tell
> save-window-excursion covers similar ground, but only controls what
> window is selected /within a given frame/.  So this macro, on which I
> based org-save-frame-excursion covers similar ground, but it does not
> subsume this macro.

I know neither subsumes the other (sadly).  I just hope new ones can be
made a bit more orthogonal (i.e. with less overlap).


        Stefan

Re: save-frame-excursion?

[Robert Goldman]

Stefan Monnier wrote:
>>> Do you know when/why it lost the focus?
> 
>> The org frame lost focus because focus shifted to the frame that
>> contained the *Calendar* buffer.
> 
> OK, so the next question is "why/when did focus shift to the *Calendar"
> buffer" (and just to be more clear by "why/when" I mean at the level of
> the Elisp code, which trail of calls causes it).

The calendar buffer was configured to be sticky in a particular frame,
so that when emacs wanted to show that calendar buffer, it showed it in
the different frame.

I don't entirely understand how the stickiness happens -- configuration
of the emacs calendar is far beyond me.
> 
>> I am not that experienced in this area, but as far as I can tell
>> save-window-excursion covers similar ground, but only controls what
>> window is selected /within a given frame/.  So this macro, on which I
>> based org-save-frame-excursion covers similar ground, but it does not
>> subsume this macro.
> 
> I know neither subsumes the other (sadly).  I just hope new ones can be
> made a bit more orthogonal (i.e. with less overlap).

Right.  I believe that these need to be done "inside to out" i.e.,

(save-excursion
  (save-window-excursion
    (org-save-frame-excursion
       ....)))

in order that they unwind properly.

What I think is at least a bit odd is that I don't know of a case where
one would /not/ want the current frame focus to be restored when doing a
save-excursion.  Admittedly it's unlikely (at least now) to get into a
state where you will change the frame focus, but whenever you /do/
change the frame focus, wouldn't you want it restored?

But this may simply expose my naiveté about emacs.

Best,
r

Re: save-frame-excursion?

[Stefan Monnier]

>>>> Do you know when/why it lost the focus?
>>> The org frame lost focus because focus shifted to the frame that
>>> contained the *Calendar* buffer.
>> OK, so the next question is "why/when did focus shift to the *Calendar"
>> buffer" (and just to be more clear by "why/when" I mean at the level of
>> the Elisp code, which trail of calls causes it).
> The calendar buffer was configured to be sticky in a particular frame,
> so that when emacs wanted to show that calendar buffer, it showed it in
> the different frame.

That still doesn't explain the shift of focus.

> I don't entirely understand how the stickiness happens -- configuration
> of the emacs calendar is far beyond me.

Doesn't matter.  If the code doesn't explicitly ask for a change in
focus, then there's no reason (other than to work around a bug
somewhere, probably) why we need to reset the focus via
select-frame-set-input-focus when unwinding.

That's why we need to know the detail.

> Admittedly it's unlikely (at least now) to get into a state where you
> will change the frame focus, but whenever you /do/ change the frame
> focus, wouldn't you want it restored?

Emacs almost never changes frame focus explicitly (for the good reason
that it's mighty hard to do it reliably for all possible WMs).


        Stefan

Re: save-frame-excursion?

[Robert Goldman]

Stefan Monnier wrote:
>>>>> Do you know when/why it lost the focus?
>>>> The org frame lost focus because focus shifted to the frame that
>>>> contained the *Calendar* buffer.
>>> OK, so the next question is "why/when did focus shift to the *Calendar"
>>> buffer" (and just to be more clear by "why/when" I mean at the level of
>>> the Elisp code, which trail of calls causes it).
>> The calendar buffer was configured to be sticky in a particular frame,
>> so that when emacs wanted to show that calendar buffer, it showed it in
>> the different frame.
> 
> That still doesn't explain the shift of focus.
> 
>> I don't entirely understand how the stickiness happens -- configuration
>> of the emacs calendar is far beyond me.
> 
> Doesn't matter.  If the code doesn't explicitly ask for a change in
> focus, then there's no reason (other than to work around a bug
> somewhere, probably) why we need to reset the focus via
> select-frame-set-input-focus when unwinding.
> 
> That's why we need to know the detail.

Well, the calendar has this funny configuration variable:

calendar-setup is a variable defined in `calendar.el'.
Its value is nil


Documentation:
The frame setup of the calendar.
The choices are: `one-frame' (calendar and diary together in one separate,
dedicated frame); `two-frames' (calendar and diary in separate, dedicated
frames); `calendar-only' (calendar in a separate, dedicated frame); with
any other value the current frame is used.  Using any of the first
three options overrides the value of `view-diary-entries-initially'.

You can customize this variable.


So presumably the calendar code at least thinks about frames.  Org-mode
itself only invokes the (calendar) function:

(defun calendar (&optional arg)
  "Choose between the one frame, two frame, or basic calendar displays.
If called with an optional prefix argument, prompts for month and year.

The original function `calendar' has been renamed `calendar-basic-setup'.
See the documentation of that function for more information."
  (interactive "P")
  (cond ((equal calendar-setup 'one-frame) (calendar-one-frame-setup arg))
        ((equal calendar-setup 'two-frames) (calendar-two-frame-setup arg))
        ((equal calendar-setup 'calendar-only)
         (calendar-only-one-frame-setup arg))
        (t (calendar-basic-setup arg))))

There are calls to SELECT-FRAME in the call tree below calendar.  So
this is, I believe, a violation of the generalization about Emacs almost
never changing frame focus explicitly.

I hope this helps.  We are now /way/ out of my area of expertise.  I was
just building some save-and-restore logic around calendar.  I don't have
any first-hand understanding of what goes on inside calendar.

Best,
Robert

> 
>> Admittedly it's unlikely (at least now) to get into a state where you
>> will change the frame focus, but whenever you /do/ change the frame
>> focus, wouldn't you want it restored?
> 
> Emacs almost never changes frame focus explicitly (for the good reason
> that it's mighty hard to do it reliably for all possible WMs).
> 
> 
>         Stefan

Re: save-frame-excursion?

[Stefan Monnier]

> There are calls to SELECT-FRAME in the call tree below calendar.  So
> this is, I believe, a violation of the generalization about Emacs almost
> never changing frame focus explicitly.

If the calendar code only calls select-frame, there's no reason to think
that you need a select-frame-set-input-focus (rather than just
a select-frame) to reset the focus.


        Stefan

Re: save-frame-excursion?

[Bastien]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> The calendar buffer was configured to be sticky in a particular frame,
>> so that when emacs wanted to show that calendar buffer, it showed it in
>> the different frame.
>
> That still doesn't explain the shift of focus.

Briefly, here is how org-read-date works:

- if org-read-date-popup-calendar is t, it pops up a calendar, either in
  a different window, either in a different frame, depending on the user
  configuration;

- it prompt the user for a date (and possibly more information like the
  time, the repeater, etc.);

- it add new commands to the minibuffer-local-map: these commands modify
  the calendar display.  For example, M-S-<left> will call this command:
  (org-eval-in-calendar '(calendar-backward-month 1))

- those minibuffer commands get a post-command hook which displays the
  calendar according to the minibuffer prompt.

That's the basic mechanism.

For org-read-date to work when the calendar is displayed in another
frame, we need to prevent org-eval-in-calendar from losing the frame
focus.

I've just pushed a change to Org that spares us org-save-frame-excursion
by making `org-eval-in-calendar' DTRT about restauring the frame focus.

>> I don't entirely understand how the stickiness happens -- configuration
>> of the emacs calendar is far beyond me.
>
> Doesn't matter.  If the code doesn't explicitly ask for a change in
> focus, then there's no reason (other than to work around a bug
> somewhere, probably) why we need to reset the focus via
> select-frame-set-input-focus when unwinding.

The code was indirectly asking for a change in focus by calling the
calendar from within a command.

>> Admittedly it's unlikely (at least now) to get into a state where you
>> will change the frame focus, but whenever you /do/ change the frame
>> focus, wouldn't you want it restored?
>
> Emacs almost never changes frame focus explicitly (for the good reason
> that it's mighty hard to do it reliably for all possible WMs).

I am not using frames so I was not aware of these issues until now, 
and I agree changing frame focus explicitely is not very good.

Maybe we should have an option `org-calendar-force-display-in-window'
defaulting to t to prevent the calendar to be displayed in a separate
frame.

-- 
 Bastien

Re: save-frame-excursion?

[Stefan Monnier]

>> Emacs almost never changes frame focus explicitly (for the good reason
>> that it's mighty hard to do it reliably for all possible WMs).

> I am not using frames so I was not aware of these issues until now, 
> and I agree changing frame focus explicitely is not very good.

I don't mean to say that it's bad to do it.  I'm saying it's odd that
you'd ned to reset it explicitly if the code doesn't itself set it
explicitly somewhere.

A select-frame inside calendar should be undoable with just
a select-frame in org-mode.


        Stefan

Re: save-frame-excursion?

[Bastien]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> A select-frame inside calendar should be undoable with just
> a select-frame in org-mode.

calendar-frame-setup uses `make-frame' which does not only select the
frame, but also focuses onto it.  Hence `select-frame-set-input-focus'
for restoring the focus in the minibuffer.

-- 
 Bastien

Re: save-frame-excursion?

[Stefan Monnier]

>> A select-frame inside calendar should be undoable with just
>> a select-frame in org-mode.
> calendar-frame-setup uses `make-frame' which does not only select the
> frame, but also focuses onto it.  Hence `select-frame-set-input-focus'
> for restoring the focus in the minibuffer.

Thanks for digging deeper, so the problem is not just that focus
switches to some other frame, but that a new frame is created.
Note also that make-frame's docstring says:

   This function itself does not make the new frame the selected frame.
   The previously selected frame remains selected.  However, the
   window system may select the new frame for its own reasons, for
   instance if the frame appears under the mouse pointer and your
   setup is for focus to follow the pointer.

So the really better fix would be to make sure make-frame doesn't change
the focus (especially since the above note is not quite correct: even
for click-to-focus WMs, the new frames may get selected just by virtue
of being new, which is most likely what you're seeing).


        Stefan

RE: save-frame-excursion?

[Drew Adams]

> so the problem is not just that focus switches to some
> other frame, but that a new frame is created.
> Note also that make-frame's docstring says:
> 
>    This function itself does not make the new frame the 
>    selected frame. The previously selected frame
>    remains selected.  However, the window system may
>    select the new frame for its own reasons, for
>    instance if the frame appears under the mouse pointer
>    and your setup is for focus to follow the pointer.
> 
> So the really better fix would be to make sure make-frame 
> doesn't change the focus (especially since the above note
> is not quite correct: even for click-to-focus WMs, the new
> frames may get selected just by virtue of being new, which
> is most likely what you're seeing).


1. Yes, please do make sure that `make-frame' does not change the focus. That
fix is long overdue. Thx.


2. In addition, please make `display-buffer' also DTRT in this regard.
`display-buffer' should never cause display of the buffer in another frame to
select the buffer (with its frame receiving the focus). But that is what
happens, on MS Windows for instance:

emacs -Q
(setq pop-up-frames t)
(display-buffer "*scratch*") ; with another buffer selected

The frame with *scratch* is raised and receives the focus - buffer *scratch* is
selected. This contradicts the documented behavior of `display-buffer', and it
has done so since day one AFAIK.

This is the case not only when *scratch*'s frame is created; it happens each
time `display-buffer' is called.

That is, fixing `make-frame' will not fix the problem of focus changing whenever
a buffer is displayed. There are two fixes that are needed.

Re: save-frame-excursion?

[Bastien]

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

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> So the really better fix would be to make sure make-frame doesn't change
> the focus (especially since the above note is not quite correct: even
> for click-to-focus WMs, the new frames may get selected just by virtue
> of being new, which is most likely what you're seeing).

This patch makes sure that `make-frame' gives the focus back to the
previously selected frame.

Shall I apply this?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: frame.el.patch --]
[-- Type: text/x-diff, Size: 709 bytes --]

--- frame.el.~1.300.~	2009-07-27 03:20:02.000000000 +0800
+++ frame.el	2009-07-27 11:40:23.000000000 +0800
@@ -749,7 +749,8 @@
 instance if the frame appears under the mouse pointer and your
 setup is for focus to follow the pointer."
   (interactive)
-  (let* ((w (cond
+  (let* ((f (selected-frame))
+	 (w (cond
 	     ((assq 'terminal parameters)
 	      (let ((type (terminal-live-p (cdr (assq 'terminal parameters)))))
 		(cond
@@ -777,6 +778,7 @@
         (let ((val (frame-parameter oldframe param)))
           (when val (set-frame-parameter frame param val)))))
     (run-hook-with-args 'after-make-frame-functions frame)
+    (x-focus-frame f)
     frame))
 
 (defun filtered-frame-list (predicate)

[-- Attachment #3: Type: text/plain, Size: 14 bytes --]


-- 
 Bastien

Re: save-frame-excursion?

[Stephen J. Turnbull]

Bastien writes:
 > Stefan Monnier <monnier@iro.umontreal.ca> writes:
 > 
 > > So the really better fix would be to make sure make-frame doesn't change
 > > the focus (especially since the above note is not quite correct: even
 > > for click-to-focus WMs, the new frames may get selected just by virtue
 > > of being new, which is most likely what you're seeing).
 > 
 > This patch makes sure that `make-frame' gives the focus back to the
 > previously selected frame.

I think the hook should be run *after* returning focus to the frame,
in case of a hook function that wants to do something with the focus.

I have no opinion on whether this patch is a good way to address the
issue, however.
w

Re: save-frame-excursion?

[Bastien]

"Stephen J. Turnbull" <turnbull@sk.tsukuba.ac.jp> writes:

> Bastien writes:
>  > Stefan Monnier <monnier@iro.umontreal.ca> writes:
>  > 
>  > > So the really better fix would be to make sure make-frame doesn't change
>  > > the focus (especially since the above note is not quite correct: even
>  > > for click-to-focus WMs, the new frames may get selected just by virtue
>  > > of being new, which is most likely what you're seeing).
>  > 
>  > This patch makes sure that `make-frame' gives the focus back to the
>  > previously selected frame.
>
> I think the hook should be run *after* returning focus to the frame,
> in case of a hook function that wants to do something with the focus.

Yes, now I think about it again I think you're right.

-- 
 Bastien

Re: save-frame-excursion?

[Stefan Monnier]

> This patch makes sure that `make-frame' gives the focus back to the
> previously selected frame.
> Shall I apply this?

Don't know.  Here are some comments:
- does it work (as in "with Metacity/W32/macosx/xmonad/addyourfavoriteWMhere")?
- as Stephen pointed out, it should be run before the hook.
- it's not clear that it does the right thing if the selected-frame
  is not the frame with focus (basically we'd need a focussed-frame
  function to figure it out).
- it brings us back to this problem of "focus via activation".
- is x-focus-frame always defined when this code is run (i.e. can't
  this code be run also on emacs-nox)?
- x-focus-frame should really be renamed (so as to get rid of the "x-"
  prefix at least).


        Stefan


> --- frame.el.~1.300.~	2009-07-27 03:20:02.000000000 +0800
> +++ frame.el	2009-07-27 11:40:23.000000000 +0800
> @@ -749,7 +749,8 @@
>  instance if the frame appears under the mouse pointer and your
>  setup is for focus to follow the pointer."
>    (interactive)
> -  (let* ((w (cond
> +  (let* ((f (selected-frame))
> +	 (w (cond
>  	     ((assq 'terminal parameters)
>  	      (let ((type (terminal-live-p (cdr (assq 'terminal parameters)))))
>  		(cond
> @@ -777,6 +778,7 @@
>          (let ((val (frame-parameter oldframe param)))
>            (when val (set-frame-parameter frame param val)))))
>      (run-hook-with-args 'after-make-frame-functions frame)
> +    (x-focus-frame f)
>      frame))
 
>  (defun filtered-frame-list (predicate)



> -- 
>  Bastien

Re: save-frame-excursion?

[Bastien]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> This patch makes sure that `make-frame' gives the focus back to the
>> previously selected frame.
>> Shall I apply this?
>
> Don't know.  Here are some comments:
> - does it work (as in "with Metacity/W32/macosx/xmonad/addyourfavoriteWMhere")?

I don't have access to all these WM, I'm sending the patch here to get
feedback about this.

> - as Stephen pointed out, it should be run before the hook.

Yes.

> - it's not clear that it does the right thing if the selected-frame
>   is not the frame with focus (basically we'd need a focussed-frame
>   function to figure it out).

Yes, a focused-frame would be nice.

> - it brings us back to this problem of "focus via activation".
> - is x-focus-frame always defined when this code is run (i.e. can't
>   this code be run also on emacs-nox)?

So maybe a variant of this patch should better be applied to
make-frame-command, which handle this distinction between x/nox.  

> - x-focus-frame should really be renamed (so as to get rid of the "x-"
>   prefix at least).

Okay.  I can't do the job about renaming x-focus-frame and creating the
focused-frame function so I guess my fix will wait for that.

Thanks,

-- 
 Bastien

Re: save-frame-excursion?

[Stefan Monnier]

>>> This patch makes sure that `make-frame' gives the focus back to the
>>> previously selected frame.
>>> Shall I apply this?
>> Don't know.  Here are some comments:
>> - does it work (as in "with Metacity/W32/macosx/xmonad/addyourfavoriteWMhere")?

> I don't have access to all these WM, I'm sending the patch here to get
> feedback about this.

I know, I just wanted to make it explicit, assuming other people are
following this as well.

>> - it's not clear that it does the right thing if the selected-frame
>> is not the frame with focus (basically we'd need a focussed-frame
>> function to figure it out).
> Yes, a focused-frame would be nice.

Please at least add a FIXME comment about it, and post a bug-report to
track this issue.

>> - it brings us back to this problem of "focus via activation".
>> - is x-focus-frame always defined when this code is run (i.e. can't
>> this code be run also on emacs-nox)?
> So maybe a variant of this patch should better be applied to
> make-frame-command, which handle this distinction between x/nox.

No, lower-level is better, but you may just want to wrap the call with
an (if (fboundp 'focus-frame) ...) or someting like that.  Or maybe
implement it at an even lower level (like x-create-frame).

>> - x-focus-frame should really be renamed (so as to get rid of the "x-"
>> prefix at least).
> Okay.  I can't do the job about renaming x-focus-frame and creating the
> focused-frame function so I guess my fix will wait for that.

In any case we need to test it first.  We may get away with using
selected-frame rather than focused-frame: maybe it doesn't make that
much of a difference in practice.


        Stefan

Re: save-frame-excursion?

[Carsten Dominik]

On Jul 25, 2009, at 1:51 PM, Bastien wrote:

> Hi Robert,
>
> Bastien <bastienguerry@googlemail.com> writes:
>
>> I've just pushed a change to Org that spares us org-save-frame- 
>> excursion
>> by making `org-eval-in-calendar' DTRT about restauring the frame  
>> focus.
>
> When you have time, can you test it a bit and report about any  
> problem?
>
>> Maybe we should have an option `org-calendar-force-display-in-window'
>> defaulting to t to prevent the calendar to be displayed in a separate
>> frame.
>
> Since the Calendar displayed by `org-read-date' is meant to be a
> temporary visuel feedback, maybe Org could override the user setup
> on this.  Not sure.


Yes, I agree, and I have made this change.

- Carsten

Re: save-frame-excursion?

[Robert Goldman]

Carsten Dominik wrote:
> 
> On Jul 25, 2009, at 1:51 PM, Bastien wrote:
> 
>> Hi Robert,
>>
>> Bastien <bastienguerry@googlemail.com> writes:
>>
>>> I've just pushed a change to Org that spares us org-save-frame-excursion
>>> by making `org-eval-in-calendar' DTRT about restauring the frame focus.
>>
>> When you have time, can you test it a bit and report about any problem?
>>
>>> Maybe we should have an option `org-calendar-force-display-in-window'
>>> defaulting to t to prevent the calendar to be displayed in a separate
>>> frame.
>>
>> Since the Calendar displayed by `org-read-date' is meant to be a
>> temporary visuel feedback, maybe Org could override the user setup
>> on this.  Not sure.
> 
> 
> Yes, I agree, and I have made this change.
> 
> - Carsten

I have been running with org-save-file-excursion in place.  I will pull
from git today and run with the new version and report how it's working.

best,
Robert