Re: Stop frames stealing eachothers' minibuffers!

[Alan Mackenzie]

Hello, Stefan,

thanks for the review.  In detail:

On Wed, Oct 21, 2020 at 14:32:21 -0400, Stefan Monnier wrote:
> Thanks Alan,

> This looks pretty good to me (haven't tried it yet).
> Some comments/questions below,


>         Stefan


> > @@ -86,6 +86,19 @@ useful on systems such as FreeBSD which ships only with 
> > "etc/termcap".
> >  * Changes in Emacs 28.1
> >  
> >  +++
> > +** Switching frames when a minibuffer is active has been rationalized.
> > +By default, the minibuffer is moved to the newly selected frame.  When
> > +the current command is continued (by completing the minibuffer
> > +action), it takes effect in the frame the minibuffer was first opened
> > +in.  An alternative behavior is available by customizing
> > +'minibuffer-follows-frame' to nil; here, the minibuffer stays on the
> > +frame it was first opened on, and you must switch back to this frame
> > +to continue or abort the current command.  The old (pre 28.1),
> > +somewhat chaotic behavior is no longer available.
> > +
> > ++++
> > +*** A new system for displaying documentation for groups of function is 
> > added.
> > +
> >  ** New system for displaying documentation for groups of function.
> >  This can either be used by saying 'M-x shortdoc-display-group' and
> >  choosing a group, or clicking a button in the *Help* buffers when

> Looks like a chunk of Lars's shortdoc got brought along.

Yes, sorry.  I made a mess of hand editing the result of the merge.  Now
fixed.

> > +static bool
> > +minibuf_follows_frame (void)
> > +{
> > +  return !NILP (Fdefault_toplevel_value (Qminibuffer_follows_frame));
> > +}

> I can't think of any reason why we'd need to bother with
> `Fdefault_toplevel_value` here.  I think the only justification would be
> if using some other value could result in a crash or in garbled display,
> but AFAICT it could only result (in the worst case) in a vaguely
> unexpected behavior where the mininbuffer follows the frame when it
> shouldn't or vice versa.

Maybe I'm being a bit too careful, but I've got bad feelings about what
might happen if a buffer local value was different from the canonical
value.  I simply can't picture what might happen, but like you say, it
could well end up with a mini-window being displayed on two frames, and
there being no way to fix that situation.

> > +          FOR_EACH_FRAME (tail, frame)
> > +            {
> > +              if (EQ (XWINDOW (XFRAME 
> > (frame)->minibuffer_window)->contents,
> > +                      buffer))
> > +                {
> > +                  minibuf_window = XFRAME (frame)->minibuffer_window;
> > +                  goto after_set;
> > +                }
> > +              minibuf_window = sf->minibuffer_window;
> > +            after_set: ;
> > +            }

> I don't understand the:

>     minibuf_window = sf->minibuffer_window

> Won't this undo the

>     minibuf_window = XFRAME (frame)->minibuffer_window;

> executed in a previous iteration?
> Should this be moved to just before the loop maybe?

Yes, you're right.  I put the "catch-all" setting of minibuf_window
wrongly inside the FOR_EACH_FRAME loop.  Like you say, a neater way of
expressing this is to put the "catch-all" setting before the loop, and
allow it to get overwritten by anything found in the loop.

> > +      /* OLD STOUGH, 2020-10-21 */

> Not sure how useful this is.  I'd either remove this comment or replace
> it with an actual explanation of what's going on and/or how the code
> used to work.

It's my own personal comment for restoring changes, and it should not
have found it's way into the patch.  Sorry.

I'll repost the patch, with your and Drew's corrections separately.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).