bug#48669: Inconsistent overlay placement between minibuffer-message and set-minibuffer-message

bug#48669: Inconsistent overlay placement between minibuffer-message and set-minibuffer-message

[João Guerra]

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

`minibuffer-message' and `set-minibuffer-message' place the overlay
containing the message slightly differently, resulting in a different
behaviour when there are other overlays present at the end of the
minibuffer.

The overlay created by `minibuffer-message' always gets added to the
end of the minibuffer.
The overlay created by `set-minibuffer-message' also gets added to the
end of the minibuffer, but its properties make it so that it is shown
before other overlays at the position.
This is particularly important when the user uses packages that add
overlays to the minibuffer, such as vertical completion packages like
selectrum, vertico, and others.
The way `set-minibuffer-message' configures the overlays makes it so
that the overlay is displayed before the other overlays, which I
believe is more compatible with external packages (see attached image
for a visual example).

Would it make sense to change this behaviour (i.e. overlay
configuration) so that it is consistent in both functions?

[-- Attachment #2: image.png --]
[-- Type: image/png, Size: 21562 bytes --]

bug#48669: Inconsistent overlay placement between minibuffer-message and set-minibuffer-message

[Juri Linkov]

> `minibuffer-message' and `set-minibuffer-message' place the overlay
> containing the message slightly differently, resulting in a different
> behaviour when there are other overlays present at the end of the
> minibuffer.
>
> The overlay created by `minibuffer-message' always gets added to the
> end of the minibuffer.
> The overlay created by `set-minibuffer-message' also gets added to the
> end of the minibuffer, but its properties make it so that it is shown
> before other overlays at the position.
> This is particularly important when the user uses packages that add
> overlays to the minibuffer, such as vertical completion packages like
> selectrum, vertico, and others.
> The way `set-minibuffer-message' configures the overlays makes it so
> that the overlay is displayed before the other overlays, which I
> believe is more compatible with external packages (see attached image
> for a visual example).

To support completion packages such as icomplete, the overlay in
`set-minibuffer-message' was fixed, but not `minibuffer-message'.

> Would it make sense to change this behaviour (i.e. overlay
> configuration) so that it is consistent in both functions?

Right, `minibuffer-message' should be consistent with `set-minibuffer-message'
by copying this part:

        ;; Make sure the overlay with the message is displayed before
        ;; any other overlays in that position, in case they have
        ;; resize-mini-windows set to nil and the other overlay strings
        ;; are too long for the mini-window width.  This makes sure the
        ;; temporary message will always be visible.
        (overlay-put minibuffer-message-overlay 'priority 1100)

bug#48669: Inconsistent overlay placement between minibuffer-message and set-minibuffer-message

[Juri Linkov]

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

> Would it make sense to change this behaviour (i.e. overlay
> configuration) so that it is consistent in both functions?

Thanks for the very useful suggestion.  This could be fixed
with the following patch where I tried to sync everything
overlay-related from set-minibuffer-message to minibuffer-message:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: minibuffer-message-overlay.patch --]
[-- Type: text/x-diff, Size: 1728 bytes --]

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index ffb74235e8..cb12226c07 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -741,7 +741,8 @@ minibuffer-message
                 ;; Don't overwrite the face properties the caller has set
                 (text-properties-at 0 message))
       (setq message (apply #'propertize message minibuffer-message-properties)))
-    (let ((ol (make-overlay (point-max) (point-max) nil t t))
+    (let* ((ovpos (minibuffer--message-overlay-pos))
+           (ol (make-overlay ovpos ovpos nil t t))
           ;; A quit during sit-for normally only interrupts the sit-for,
           ;; but since minibuffer-message is used at the end of a command,
           ;; at a time when the command has virtually finished already, a C-g
@@ -755,8 +756,14 @@ minibuffer-message
               ;; The current C cursor code doesn't know to use the overlay's
               ;; marker's stickiness to figure out whether to place the cursor
               ;; before or after the string, so let's spoon-feed it the pos.
-              (put-text-property 0 1 'cursor t message))
+              (put-text-property 0 1 'cursor 1 message))
             (overlay-put ol 'after-string message)
+            ;; Make sure the overlay with the message is displayed before
+            ;; any other overlays in that position, in case they have
+            ;; resize-mini-windows set to nil and the other overlay strings
+            ;; are too long for the mini-window width.  This makes sure the
+            ;; temporary message will always be visible.
+            (overlay-put ol 'priority 1100)
             (sit-for (or minibuffer-message-timeout 1000000)))
         (delete-overlay ol)))))
 

bug#48669: Inconsistent overlay placement between minibuffer-message and set-minibuffer-message

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Sun, 06 Jun 2021 00:43:54 +0300
> Cc: 48669@debbugs.gnu.org
> 
> 
> [1:text/plain Hide]
> 
> > Would it make sense to change this behaviour (i.e. overlay
> > configuration) so that it is consistent in both functions?
> 
> Thanks for the very useful suggestion.  This could be fixed
> with the following patch where I tried to sync everything
> overlay-related from set-minibuffer-message to minibuffer-message:
> 
> 
> [2:text/x-diff Hide]
> 
> diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
> index ffb74235e8..cb12226c07 100644
> --- a/lisp/minibuffer.el
> +++ b/lisp/minibuffer.el
> @@ -741,7 +741,8 @@ minibuffer-message
>                  ;; Don't overwrite the face properties the caller has set
>                  (text-properties-at 0 message))
>        (setq message (apply #'propertize message minibuffer-message-properties)))
> -    (let ((ol (make-overlay (point-max) (point-max) nil t t))
> +    (let* ((ovpos (minibuffer--message-overlay-pos))
> +           (ol (make-overlay ovpos ovpos nil t t))

Doesn't this rely too much on the internal details of
minibuffer--message-overlay-pos?  At least, without any comments, this
call looks like a riddle that isn't easy to unlock.

> -              (put-text-property 0 1 'cursor t message))
> +              (put-text-property 0 1 'cursor 1 message))

Why this change?

bug#48669: Inconsistent overlay placement between minibuffer-message and set-minibuffer-message

[Juri Linkov]

>> @@ -741,7 +741,8 @@ minibuffer-message
>>                  ;; Don't overwrite the face properties the caller has set
>>                  (text-properties-at 0 message))
>>        (setq message (apply #'propertize message minibuffer-message-properties)))
>> -    (let ((ol (make-overlay (point-max) (point-max) nil t t))
>> +    (let* ((ovpos (minibuffer--message-overlay-pos))
>> +           (ol (make-overlay ovpos ovpos nil t t))
>
> Doesn't this rely too much on the internal details of
> minibuffer--message-overlay-pos?  At least, without any comments, this
> call looks like a riddle that isn't easy to unlock.

If minibuffer--message-overlay-pos serves its purpose for
set-minibuffer-message, it seems suitable for
minibuffer-message as well.

>> -              (put-text-property 0 1 'cursor t message))
>> +              (put-text-property 0 1 'cursor 1 message))
>
> Why this change?

Only to make minibuffer-message the identical copy of
set-minibuffer-message.

bug#48669: Inconsistent overlay placement between minibuffer-message and set-minibuffer-message

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: joca.bt@gmail.com,  48669@debbugs.gnu.org
> Date: Sun, 06 Jun 2021 23:54:58 +0300
> 
> >> -    (let ((ol (make-overlay (point-max) (point-max) nil t t))
> >> +    (let* ((ovpos (minibuffer--message-overlay-pos))
> >> +           (ol (make-overlay ovpos ovpos nil t t))
> >
> > Doesn't this rely too much on the internal details of
> > minibuffer--message-overlay-pos?  At least, without any comments, this
> > call looks like a riddle that isn't easy to unlock.
> 
> If minibuffer--message-overlay-pos serves its purpose for
> set-minibuffer-message, it seems suitable for
> minibuffer-message as well.

So you object to adding a comment there which would explain what that
call does?

> >> -              (put-text-property 0 1 'cursor t message))
> >> +              (put-text-property 0 1 'cursor 1 message))
> >
> > Why this change?
> 
> Only to make minibuffer-message the identical copy of
> set-minibuffer-message.

I'd say make it the other way around.  Handling the 'cursor' property
with a numeric value is more expensive and tricky than if the value is
t, and in this case the effect is exactly the same.

bug#48669: Inconsistent overlay placement between minibuffer-message and set-minibuffer-message

[Juri Linkov]

tags 48669 fixed
close 48669 28.0.50
thanks

>> >> -    (let ((ol (make-overlay (point-max) (point-max) nil t t))
>> >> +    (let* ((ovpos (minibuffer--message-overlay-pos))
>> >> +           (ol (make-overlay ovpos ovpos nil t t))
>> >
>> > Doesn't this rely too much on the internal details of
>> > minibuffer--message-overlay-pos?  At least, without any comments, this
>> > call looks like a riddle that isn't easy to unlock.
>> 
>> If minibuffer--message-overlay-pos serves its purpose for
>> set-minibuffer-message, it seems suitable for
>> minibuffer-message as well.
>
> So you object to adding a comment there which would explain what that
> call does?

Now I added a comment, and pushed to master.

>> >> -              (put-text-property 0 1 'cursor t message))
>> >> +              (put-text-property 0 1 'cursor 1 message))
>> >
>> > Why this change?
>> 
>> Only to make minibuffer-message the identical copy of
>> set-minibuffer-message.
>
> I'd say make it the other way around.  Handling the 'cursor' property
> with a numeric value is more expensive and tricky than if the value is
> t, and in this case the effect is exactly the same.

So I changed the 'cursor' property to t.

bug#48669: Inconsistent overlay placement between minibuffer-message and set-minibuffer-message

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: joca.bt@gmail.com,  48669@debbugs.gnu.org
> Date: Tue, 08 Jun 2021 19:52:07 +0300
> 
> >> If minibuffer--message-overlay-pos serves its purpose for
> >> set-minibuffer-message, it seems suitable for
> >> minibuffer-message as well.
> >
> > So you object to adding a comment there which would explain what that
> > call does?
> 
> Now I added a comment, and pushed to master.
> 
> >> >> -              (put-text-property 0 1 'cursor t message))
> >> >> +              (put-text-property 0 1 'cursor 1 message))
> >> >
> >> > Why this change?
> >> 
> >> Only to make minibuffer-message the identical copy of
> >> set-minibuffer-message.
> >
> > I'd say make it the other way around.  Handling the 'cursor' property
> > with a numeric value is more expensive and tricky than if the value is
> > t, and in this case the effect is exactly the same.
> 
> So I changed the 'cursor' property to t.

Thank you.