bug#73734: [PATCH] Fix tmm menu layout

bug#73734: [PATCH] Fix tmm menu layout

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Tags: patch

Hi,

Depending on the frame and font size, some tmm menus could be somewhat
hard to read.  Here is what I get with the "Mark" menu from a Dired
buffer:

[-- Attachment #2: 2024-10-10T15:25+0200.png --]
[-- Type: image/png, Size: 43861 bytes --]

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


The selection character could be mistaken for part of another
keybinding.

This patch tries to solves this issue.  I tested it a bit with different
frame sizes and different fonts and get good enough results.

Here, how I justify the modification of `colwidth':

      - I don't think we need the "(min 30)" part since, if the frame is
        wide enough, we always get a colwidth of 30.
        
      - I don't think "(window-width)" is what we need since, by
        default, the *Completions* buffer will use the full frame width.

      - I tried to set the layout on three columns because "why not?"

While here, I had some clean up.

In GNU Emacs 31.0.50 (build 18, x86_64-unknown-openbsd7.6, X toolkit) of
 2024-10-09 built on computer
Repository revision: 9ed82c26a36caf9a9a85779cbb3a58b7ccd3dffb
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101013
System Description: OpenBSD computer 7.6 GENERIC.MP#344 amd64

Configured using:
 'configure CC=egcc CPPFLAGS=-I/usr/local/include
 LDFLAGS=-L/usr/local/lib MAKEINFO=gmakeinfo --prefix=/home/manuel/emacs
 --bindir=/home/manuel/bin --with-x-toolkit=lucid
 --with-toolkit-scroll-bars=no --without-cairo
 --without-compress-install'


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0001-Fix-tmm-menu-layout.patch --]
[-- Type: text/patch, Size: 2379 bytes --]

From 309a846fc13119484ea2f910d20e79fab29c8ea9 Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Mon, 7 Oct 2024 10:52:03 +0200
Subject: [PATCH] Fix tmm menu layout

* lisp/tmm.el (tmm-mb-map): Remove unused and unset keymap
variable.
* lisp/tmm.el (tmm-get-keymap): Fix menu layout.
---
 lisp/tmm.el | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/lisp/tmm.el b/lisp/tmm.el
index ed74c307009..1e3942761a1 100644
--- a/lisp/tmm.el
+++ b/lisp/tmm.el
@@ -82,9 +82,6 @@ tmm-mid-prompt
   :type '(choice (const :tag "No shortcuts" nil)
                  string))
 
-(defvar tmm-mb-map nil
-  "A place to store minibuffer map.")
-
 (defcustom tmm-completion-prompt
   "Press PageUp key to reach this buffer from the minibuffer.
 Alternatively, you can use Up/Down keys (or your History keys) to change
@@ -325,14 +322,14 @@ tmm-define-keys
         ;; downcase input to the same
         (define-key map (char-to-string (downcase c)) 'tmm-shortcut)
         (define-key map (char-to-string (upcase c)) 'tmm-shortcut)))
-    (if minibuffer
-	(progn
-          (define-key map [pageup] 'tmm-goto-completions)
-          (define-key map [prior] 'tmm-goto-completions)
-          (define-key map "\ev" 'tmm-goto-completions)
-          (define-key map "\C-n" 'next-history-element)
-          (define-key map "\C-p" 'previous-history-element)
-          (define-key map "^" 'self-insert-and-exit)))
+    (when minibuffer
+      (define-key map [pageup] 'tmm-goto-completions)
+      (define-key map [prior] 'tmm-goto-completions)
+      (define-key map "\ev" 'tmm-goto-completions)
+      (define-key map "\C-n" 'next-history-element)
+      (define-key map "\C-p" 'previous-history-element)
+      ;; Previous menu shortcut (see `tmm-prompt')
+      (define-key map "^" 'self-insert-and-exit))
     (prog1 (current-local-map)
       (use-local-map (append map (current-local-map))))))
 
@@ -487,7 +484,7 @@ tmm-get-keymap
             (when binding
               (setq binding (key-description binding))
               ;; Try to align the keybindings.
-              (let ((colwidth (min 30 (- (/ (window-width) 2) 10))))
+              (let ((colwidth (- (/ (frame-width) 3) 15)))
                 (setq str
                       (concat str
                               (make-string (max 2 (- colwidth
-- 
2.46.1


[-- Attachment #5: Type: text/plain, Size: 19 bytes --]


-- 
Manuel Giraud

bug#73734: [PATCH] Fix tmm menu layout

[Eli Zaretskii]

> Date: Thu, 10 Oct 2024 15:53:12 +0200
> From:  Manuel Giraud via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Depending on the frame and font size, some tmm menus could be somewhat
> hard to read.  Here is what I get with the "Mark" menu from a Dired
> buffer:
> 
> The selection character could be mistaken for part of another
> keybinding.

What is the "selection character" in this image?

> This patch tries to solves this issue.

Can you tell how?  All I see is a different value for colwidth.  I
guess I'm missing something.

> Here, how I justify the modification of `colwidth':
> 
>       - I don't think we need the "(min 30)" part since, if the frame is
>         wide enough, we always get a colwidth of 30.
>         
>       - I don't think "(window-width)" is what we need since, by
>         default, the *Completions* buffer will use the full frame width.

Martin, is that guaranteed?

And even if it is, what's the harm in keeping window-width?

Finally, does this change some user-facing aspect of the tmm behavior?
If so, maybe we need a NEWS entry.

Thanks.

bug#73734: [PATCH] Fix tmm menu layout

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Thu, 10 Oct 2024 15:53:12 +0200
>> From:  Manuel Giraud via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> Depending on the frame and font size, some tmm menus could be somewhat
>> hard to read.  Here is what I get with the "Mark" menu from a Dired
>> buffer:
>> 
>> The selection character could be mistaken for part of another
>> keybinding.
>
> What is the "selection character" in this image?

On the image below, "n" is what I called the selection character for
"Next Marked" while "* c" is the keybinding for "Change Marks...".  The
proximity of those two makes this hard to read IMO.

[-- Attachment #2: 2024-10-10T15:25+0200.png --]
[-- Type: image/png, Size: 58285 bytes --]

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


>> This patch tries to solves this issue.
>
> Can you tell how?  All I see is a different value for colwidth.  I
> guess I'm missing something.

Yes but this different colwidth would leave more space between a
keybinding and the "selection character" of the next column.  But, I
have to say that it would be hard to prove that it will work for every
possible settings.

>> Here, how I justify the modification of `colwidth':
>> 
>>       - I don't think we need the "(min 30)" part since, if the frame is
>>         wide enough, we always get a colwidth of 30.
>>         
>>       - I don't think "(window-width)" is what we need since, by
>>         default, the *Completions* buffer will use the full frame width.
>
> Martin, is that guaranteed?
>
> And even if it is, what's the harm in keeping window-width?

I don't think that a full frame width *Completions* buffer is
guaranteed: it is only what I see with "emacs -Q".

Keeping window-width in this calculation seems a bit strange because, by
default, it has nothing to do with the *Completions* buffer window
width.

> Finally, does this change some user-facing aspect of the tmm behavior?
> If so, maybe we need a NEWS entry.

I don't think there is behavioral change only a layout change here.
-- 
Manuel Giraud

bug#73734: [PATCH] Fix tmm menu layout

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: martin rudalics <rudalics@gmx.at>,  73734@debbugs.gnu.org
> Date: Thu, 10 Oct 2024 20:00:59 +0200
> 
> > What is the "selection character" in this image?
> 
> On the image below, "n" is what I called the selection character for
> "Next Marked" while "* c" is the keybinding for "Change Marks...".  The
> proximity of those two makes this hard to read IMO.

Ah, I see now what you meant.  But if this is the problem, then
changing the column width is not a reliable solution, because you
cannot know in advance what will be the width of the SPC glyph in a
font people use.  I suggest to use 'display' properties instead, for
example '(space . (:width N)), where N is the number of canonical
columns we want the space to take on display.

> >> This patch tries to solves this issue.
> >
> > Can you tell how?  All I see is a different value for colwidth.  I
> > guess I'm missing something.
> 
> Yes but this different colwidth would leave more space between a
> keybinding and the "selection character" of the next column.  But, I
> have to say that it would be hard to prove that it will work for every
> possible settings.

Yes, I think using the 'display' property will be more reliable.

> >> Here, how I justify the modification of `colwidth':
> >> 
> >>       - I don't think we need the "(min 30)" part since, if the frame is
> >>         wide enough, we always get a colwidth of 30.
> >>         
> >>       - I don't think "(window-width)" is what we need since, by
> >>         default, the *Completions* buffer will use the full frame width.
> >
> > Martin, is that guaranteed?
> >
> > And even if it is, what's the harm in keeping window-width?
> 
> I don't think that a full frame width *Completions* buffer is
> guaranteed: it is only what I see with "emacs -Q".
> 
> Keeping window-width in this calculation seems a bit strange because, by
> default, it has nothing to do with the *Completions* buffer window
> width.

Which window's width does this return in this case?

bug#73734: [PATCH] Fix tmm menu layout

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Manuel Giraud <manuel@ledu-giraud.fr>
>> Cc: martin rudalics <rudalics@gmx.at>,  73734@debbugs.gnu.org
>> Date: Thu, 10 Oct 2024 20:00:59 +0200
>> 
>> > What is the "selection character" in this image?
>> 
>> On the image below, "n" is what I called the selection character for
>> "Next Marked" while "* c" is the keybinding for "Change Marks...".  The
>> proximity of those two makes this hard to read IMO.
>
> Ah, I see now what you meant.  But if this is the problem, then
> changing the column width is not a reliable solution, because you
> cannot know in advance what will be the width of the SPC glyph in a
> font people use.  I suggest to use 'display' properties instead, for
> example '(space . (:width N)), where N is the number of canonical
> columns we want the space to take on display.

Sure this will be more reliable for proportional font's users but we'll
still need to calculate this N that would be (- colwidth (string-width
str) (string-width binding)) I think.  And so we still need a value for
colwidth, no?

[...]

>> >> Here, how I justify the modification of `colwidth':
>> >> 
>> >>       - I don't think we need the "(min 30)" part since, if the frame is
>> >>         wide enough, we always get a colwidth of 30.
>> >>         
>> >>       - I don't think "(window-width)" is what we need since, by
>> >>         default, the *Completions* buffer will use the full frame width.
>> >
>> > Martin, is that guaranteed?
>> >
>> > And even if it is, what's the harm in keeping window-width?
>> 
>> I don't think that a full frame width *Completions* buffer is
>> guaranteed: it is only what I see with "emacs -Q".
>> 
>> Keeping window-width in this calculation seems a bit strange because, by
>> default, it has nothing to do with the *Completions* buffer window
>> width.
>
> Which window's width does this return in this case?

AFAIU, this window-width call is done from the user's current window.
-- 
Manuel Giraud

bug#73734: [PATCH] Fix tmm menu layout

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: rudalics@gmx.at,  73734@debbugs.gnu.org
> Date: Fri, 11 Oct 2024 08:37:00 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Ah, I see now what you meant.  But if this is the problem, then
> > changing the column width is not a reliable solution, because you
> > cannot know in advance what will be the width of the SPC glyph in a
> > font people use.  I suggest to use 'display' properties instead, for
> > example '(space . (:width N)), where N is the number of canonical
> > columns we want the space to take on display.
> 
> Sure this will be more reliable for proportional font's users but we'll
> still need to calculate this N that would be (- colwidth (string-width
> str) (string-width binding)) I think.  And so we still need a value for
> colwidth, no?

Yes, we will still need a value for colwidth.  But string-width counts
columns, so it returns the same value for fixed-pitch and
variable-pitch fonts alike; it doesn't count pixels.  Using 'display'
properties, we can force the SPC characters used as delimiters to take
exactly one canonical column, even if the font actually used has
narrower glyphs for SPC.  So the computed colwidth value will be
displayed reliably, and will never cause the visual confusion which
you show on your screenshot.  In which case we can leave the
currently-computed colwidth value alone, as it is not what causes the
problem.  (We could also decide to use other values for colwidth, but
it would be a separate issue, unrelated to the confusing display.)

> >> >>       - I don't think "(window-width)" is what we need since, by
> >> >>         default, the *Completions* buffer will use the full frame width.
> >> >
> >> > Martin, is that guaranteed?
> >> >
> >> > And even if it is, what's the harm in keeping window-width?
> >> 
> >> I don't think that a full frame width *Completions* buffer is
> >> guaranteed: it is only what I see with "emacs -Q".
> >> 
> >> Keeping window-width in this calculation seems a bit strange because, by
> >> default, it has nothing to do with the *Completions* buffer window
> >> width.
> >
> > Which window's width does this return in this case?
> 
> AFAIU, this window-width call is done from the user's current window.

Then it's indeed not relevant, but let's wait for Martin to chime in.

bug#73734: [PATCH] Fix tmm menu layout

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Manuel Giraud <manuel@ledu-giraud.fr>
>> Cc: rudalics@gmx.at,  73734@debbugs.gnu.org
>> Date: Fri, 11 Oct 2024 08:37:00 +0200
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > Ah, I see now what you meant.  But if this is the problem, then
>> > changing the column width is not a reliable solution, because you
>> > cannot know in advance what will be the width of the SPC glyph in a
>> > font people use.  I suggest to use 'display' properties instead, for
>> > example '(space . (:width N)), where N is the number of canonical
>> > columns we want the space to take on display.
>> 
>> Sure this will be more reliable for proportional font's users but we'll
>> still need to calculate this N that would be (- colwidth (string-width
>> str) (string-width binding)) I think.  And so we still need a value for
>> colwidth, no?
>
> Yes, we will still need a value for colwidth.  But string-width counts
> columns, so it returns the same value for fixed-pitch and
> variable-pitch fonts alike; it doesn't count pixels.  Using 'display'
> properties, we can force the SPC characters used as delimiters to take
> exactly one canonical column, even if the font actually used has
> narrower glyphs for SPC.  So the computed colwidth value will be
> displayed reliably, and will never cause the visual confusion which
> you show on your screenshot.  In which case we can leave the
> currently-computed colwidth value alone, as it is not what causes the
> problem.  (We could also decide to use other values for colwidth, but
> it would be a separate issue, unrelated to the confusing display.)

Ok.  I'll try this and see for the result.
-- 
Manuel Giraud

bug#73734: [PATCH] Fix tmm menu layout

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Manuel Giraud <manuel@ledu-giraud.fr>
>> Cc: rudalics@gmx.at,  73734@debbugs.gnu.org
>> Date: Fri, 11 Oct 2024 08:37:00 +0200
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > Ah, I see now what you meant.  But if this is the problem, then
>> > changing the column width is not a reliable solution, because you
>> > cannot know in advance what will be the width of the SPC glyph in a
>> > font people use.  I suggest to use 'display' properties instead, for
>> > example '(space . (:width N)), where N is the number of canonical
>> > columns we want the space to take on display.
>> 
>> Sure this will be more reliable for proportional font's users but we'll
>> still need to calculate this N that would be (- colwidth (string-width
>> str) (string-width binding)) I think.  And so we still need a value for
>> colwidth, no?
>
> Yes, we will still need a value for colwidth.  But string-width counts
> columns, so it returns the same value for fixed-pitch and
> variable-pitch fonts alike; it doesn't count pixels.  Using 'display'
> properties, we can force the SPC characters used as delimiters to take
> exactly one canonical column, even if the font actually used has
> narrower glyphs for SPC.  So the computed colwidth value will be
> displayed reliably, and will never cause the visual confusion which
> you show on your screenshot.  In which case we can leave the
> currently-computed colwidth value alone, as it is not what causes the
> problem.  (We could also decide to use other values for colwidth, but
> it would be a separate issue, unrelated to the confusing display.)

With the following patch:

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

diff --git a/lisp/tmm.el b/lisp/tmm.el
index 1e3942761a1..8d1f9b15e82 100644
--- a/lisp/tmm.el
+++ b/lisp/tmm.el
@@ -484,13 +484,12 @@ tmm-get-keymap
             (when binding
               (setq binding (key-description binding))
               ;; Try to align the keybindings.
-              (let ((colwidth (- (/ (frame-width) 3) 15)))
+              (let* ((colwidth (min 30 (- (/ (window-width) 2) 10)))
+                     (spaces (max 2 (- colwidth (string-width str)
+                                       (string-width binding)))))
                 (setq str
                       (concat str
-                              (make-string (max 2 (- colwidth
-                                                     (string-width str)
-                                                     (string-width binding)))
-                                           ?\s)
+                              (propertize " " 'display `(space . (:width ,spaces)))
                               binding)))))))
       (and km (stringp km) (setq str km))
       ;; Verify that the command is enabled;

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


The result is even worse because now there is wrapping involved:

[-- Attachment #4: 2024-10-11T10:06+0200.png --]
[-- Type: image/png, Size: 70509 bytes --]

[-- Attachment #5: Type: text/plain, Size: 18 bytes --]

-- 
Manuel Giraud

bug#73734: [PATCH] Fix tmm menu layout

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: rudalics@gmx.at,  73734@debbugs.gnu.org
> Date: Fri, 11 Oct 2024 10:10:25 +0200
> 
> With the following patch:
> 
> diff --git a/lisp/tmm.el b/lisp/tmm.el
> index 1e3942761a1..8d1f9b15e82 100644
> --- a/lisp/tmm.el
> +++ b/lisp/tmm.el
> @@ -484,13 +484,12 @@ tmm-get-keymap
>              (when binding
>                (setq binding (key-description binding))
>                ;; Try to align the keybindings.
> -              (let ((colwidth (- (/ (frame-width) 3) 15)))
> +              (let* ((colwidth (min 30 (- (/ (window-width) 2) 10)))
> +                     (spaces (max 2 (- colwidth (string-width str)
> +                                       (string-width binding)))))
>                  (setq str
>                        (concat str
> -                              (make-string (max 2 (- colwidth
> -                                                     (string-width str)
> -                                                     (string-width binding)))
> -                                           ?\s)
> +                              (propertize " " 'display `(space . (:width ,spaces)))
>                                binding)))))))
>        (and km (stringp km) (setq str km))
>        ;; Verify that the command is enabled;
> 
> The result is even worse because now there is wrapping involved:

This is not what I suggested.  I suggested to leave the spaces in the
string intact and just put on each of them a display property whose
value is '(space . (:width 1)).  This cannot possibly cause wrapping,
unless the original string already does, because we don't change how
many SPC characters are used, we just force each one of them to take
exactly one canonical column.

Now, I don't know what is wrong with the code you used that it caused
wrapping, but your code is more complex because it attempts to
calculate the number of spaces required to show the menu, whereas the
method I suggested doesn't.

bug#73734: [PATCH] Fix tmm menu layout

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Eli Zaretskii <eliz@gnu.org> writes:

[...]

>> The result is even worse because now there is wrapping involved:
>
> This is not what I suggested.  I suggested to leave the spaces in the
> string intact and just put on each of them a display property whose
> value is '(space . (:width 1)).  This cannot possibly cause wrapping,
> unless the original string already does, because we don't change how
> many SPC characters are used, we just force each one of them to take
> exactly one canonical column.

Sorry, I did not understand that.  Do you mean something like this:

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

diff --git a/lisp/tmm.el b/lisp/tmm.el
index 1e3942761a1..4ddf61cc9b3 100644
--- a/lisp/tmm.el
+++ b/lisp/tmm.el
@@ -484,13 +484,16 @@ tmm-get-keymap
             (when binding
               (setq binding (key-description binding))
               ;; Try to align the keybindings.
-              (let ((colwidth (- (/ (frame-width) 3) 15)))
+              (let* ((colwidth (min 30 (- (/ (window-width) 2) 10)))
+                     (spaces (make-string (max 2 (- colwidth
+                                                    (string-width str)
+                                                    (string-width binding)))
+                                          ?\s)))
+                (dotimes (i (length spaces))
+                  (put-text-property i (1+ i) 'display '(space . (:width 1)) spaces))
                 (setq str
                       (concat str
-                              (make-string (max 2 (- colwidth
-                                                     (string-width str)
-                                                     (string-width binding)))
-                                           ?\s)
+                              spaces
                               binding)))))))
       (and km (stringp km) (setq str km))
       ;; Verify that the command is enabled;

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


But then, the result is that, now, the keybinding is placed just *one*
space after its menu entry's name.  So yes, the confusion I was talking
about is gone but we completly loose the spacing between entry name and
keybinding as well as the vertical alignment of keybindings.
-- 
Manuel Giraud

bug#73734: [PATCH] Fix tmm menu layout

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: rudalics@gmx.at,  73734@debbugs.gnu.org
> Date: Fri, 11 Oct 2024 12:45:21 +0200
> 
> diff --git a/lisp/tmm.el b/lisp/tmm.el
> index 1e3942761a1..4ddf61cc9b3 100644
> --- a/lisp/tmm.el
> +++ b/lisp/tmm.el
> @@ -484,13 +484,16 @@ tmm-get-keymap
>              (when binding
>                (setq binding (key-description binding))
>                ;; Try to align the keybindings.
> -              (let ((colwidth (- (/ (frame-width) 3) 15)))
> +              (let* ((colwidth (min 30 (- (/ (window-width) 2) 10)))
> +                     (spaces (make-string (max 2 (- colwidth
> +                                                    (string-width str)
> +                                                    (string-width binding)))
> +                                          ?\s)))
> +                (dotimes (i (length spaces))
> +                  (put-text-property i (1+ i) 'display '(space . (:width 1)) spaces))
>                  (setq str
>                        (concat str
> -                              (make-string (max 2 (- colwidth
> -                                                     (string-width str)
> -                                                     (string-width binding)))
> -                                           ?\s)
> +                              spaces
>                                binding)))))))
>        (and km (stringp km) (setq str km))
>        ;; Verify that the command is enabled;
> 
> But then, the result is that, now, the keybinding is placed just *one*
> space after its menu entry's name.  So yes, the confusion I was talking
> about is gone but we completly loose the spacing between entry name and
> keybinding as well as the vertical alignment of keybindings.

I meant this:

diff --git a/lisp/tmm.el b/lisp/tmm.el
index ed74c30..258f1a5 100644
--- a/lisp/tmm.el
+++ b/lisp/tmm.el
@@ -487,13 +487,15 @@ tmm-get-keymap
             (when binding
               (setq binding (key-description binding))
               ;; Try to align the keybindings.
-              (let ((colwidth (min 30 (- (/ (window-width) 2) 10))))
+              (let* ((colwidth (min 30 (- (/ (window-width) 2) 10)))
+                     (nspaces (max 2 (- colwidth
+                                        (string-width str)
+                                        (string-width binding)))))
                 (setq str
                       (concat str
-                              (make-string (max 2 (- colwidth
-                                                     (string-width str)
-                                                     (string-width binding)))
-                                           ?\s)
+                              (propertize (make-string nspaces ?\s)
+                                          'display
+                                          (cons 'space (list :width nspaces)))
                               binding)))))))
       (and km (stringp km) (setq str km))
       ;; Verify that the command is enabled;

This doesn't align bindings if a variable-pitch font is used, but
it still produces a better display than the current code does when
variable-pitch font is the default font.

bug#73734: [PATCH] Fix tmm menu layout

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

> This doesn't align bindings if a variable-pitch font is used, but
> it still produces a better display than the current code does when
> variable-pitch font is the default font.

Thanks, yes this is better to remove the confusion (but sometimes misses
bindings alignment).  To finalize this patch, I still have two questions
left:

        - What is the purpose of the "(min 30)"?  Because if the window
          is large enough this will always return 30.

        - Should we use frame-width instead of window-width?  It seems
          that when this function is called (get-buffer-window
          "*Completions*") always returns nil.
-- 
Manuel Giraud

bug#73734: [PATCH] Fix tmm menu layout

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: rudalics@gmx.at,  73734@debbugs.gnu.org
> Date: Sat, 12 Oct 2024 16:10:10 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> [...]
> 
> > This doesn't align bindings if a variable-pitch font is used, but
> > it still produces a better display than the current code does when
> > variable-pitch font is the default font.
> 
> Thanks, yes this is better to remove the confusion (but sometimes misses
> bindings alignment).  To finalize this patch, I still have two questions
> left:
> 
>         - What is the purpose of the "(min 30)"?  Because if the window
>           is large enough this will always return 30.

First, I didn't write this code and don't consider myself an expert,
so what follows are basically my guesses.  (In the code I proposed I
simply didn't touch those parts to clearly separate my changes from
the rest.)

I think the minimum of 30 is there to make sure we don't produce just
2 sets of columns when the window is wide enough to allow more.  This
uses the screen real estate more efficiently, so the *Completions*
window is not as tall as it would be with just 2 sets of columns.

>         - Should we use frame-width instead of window-width?  It seems
>           that when this function is called (get-buffer-window
>           "*Completions*") always returns nil.

I think we should fall back to frame-width, but use the width of the
*Completions* window if it exists.  I'm quite sure there's a sequence
of commands which will leave the *Completions* buffer on display, and
in that case the window showing it will be reused.

bug#73734: [PATCH] Fix tmm menu layout

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Manuel Giraud <manuel@ledu-giraud.fr>
>> Cc: rudalics@gmx.at,  73734@debbugs.gnu.org
>> Date: Sat, 12 Oct 2024 16:10:10 +0200
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> [...]
>> 
>> > This doesn't align bindings if a variable-pitch font is used, but
>> > it still produces a better display than the current code does when
>> > variable-pitch font is the default font.
>> 
>> Thanks, yes this is better to remove the confusion (but sometimes misses
>> bindings alignment).  To finalize this patch, I still have two questions
>> left:
>> 
>>         - What is the purpose of the "(min 30)"?  Because if the window
>>           is large enough this will always return 30.
>
> First, I didn't write this code and don't consider myself an expert,
> so what follows are basically my guesses.  (In the code I proposed I
> simply didn't touch those parts to clearly separate my changes from
> the rest.)
>
> I think the minimum of 30 is there to make sure we don't produce just
> 2 sets of columns when the window is wide enough to allow more.  This
> uses the screen real estate more efficiently, so the *Completions*
> window is not as tall as it would be with just 2 sets of columns.

Ok, that makes sense.  I was under the impression that 30 is too small
but you're right that it could (should?) stay the same.

>>         - Should we use frame-width instead of window-width?  It seems
>>           that when this function is called (get-buffer-window
>>           "*Completions*") always returns nil.
>
> I think we should fall back to frame-width, but use the width of the
> *Completions* window if it exists.  I'm quite sure there's a sequence
> of commands which will leave the *Completions* buffer on display, and
> in that case the window showing it will be reused.

Alright, so what about this new version of the patch?  FWIW, I have
tested it lightly and the only time the keybinding alignment is off is
when menu entry is quite long and we needed the 2 spaces separation with
the keybinding.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-tmm-menu-layout-Bug-73734.patch --]
[-- Type: text/x-patch, Size: 3507 bytes --]

From 3fdd7da0724ce4b335eabfd6bfed55f266909e19 Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Mon, 7 Oct 2024 10:52:03 +0200
Subject: [PATCH] Fix tmm menu layout (Bug#73734)

* lisp/tmm.el (tmm-mb-map): Remove unused and unset keymap
variable.
* lisp/tmm.el (tmm-get-keymap): Fix menu layout to avoid
confusion between the tmm-shortcut and the keybinding of the
previous column.
---
 lisp/tmm.el | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/lisp/tmm.el b/lisp/tmm.el
index ed74c307009..252c1e45afa 100644
--- a/lisp/tmm.el
+++ b/lisp/tmm.el
@@ -82,9 +82,6 @@ tmm-mid-prompt
   :type '(choice (const :tag "No shortcuts" nil)
                  string))
 
-(defvar tmm-mb-map nil
-  "A place to store minibuffer map.")
-
 (defcustom tmm-completion-prompt
   "Press PageUp key to reach this buffer from the minibuffer.
 Alternatively, you can use Up/Down keys (or your History keys) to change
@@ -325,14 +322,14 @@ tmm-define-keys
         ;; downcase input to the same
         (define-key map (char-to-string (downcase c)) 'tmm-shortcut)
         (define-key map (char-to-string (upcase c)) 'tmm-shortcut)))
-    (if minibuffer
-	(progn
-          (define-key map [pageup] 'tmm-goto-completions)
-          (define-key map [prior] 'tmm-goto-completions)
-          (define-key map "\ev" 'tmm-goto-completions)
-          (define-key map "\C-n" 'next-history-element)
-          (define-key map "\C-p" 'previous-history-element)
-          (define-key map "^" 'self-insert-and-exit)))
+    (when minibuffer
+      (define-key map [pageup] 'tmm-goto-completions)
+      (define-key map [prior] 'tmm-goto-completions)
+      (define-key map "\ev" 'tmm-goto-completions)
+      (define-key map "\C-n" 'next-history-element)
+      (define-key map "\C-p" 'previous-history-element)
+      ;; Previous menu shortcut (see `tmm-prompt')
+      (define-key map "^" 'self-insert-and-exit))
     (prog1 (current-local-map)
       (use-local-map (append map (current-local-map))))))
 
@@ -487,13 +484,20 @@ tmm-get-keymap
             (when binding
               (setq binding (key-description binding))
               ;; Try to align the keybindings.
-              (let ((colwidth (min 30 (- (/ (window-width) 2) 10))))
+              (let* ((window (get-buffer-window "*Completions*"))
+                     (colwidth (min 30 (- (/ (if window
+                                                 (window-width window)
+                                               (frame-width))
+                                             2)
+                                          10)))
+                     (nspaces (max 2 (- colwidth
+                                        (string-width str)
+                                        (string-width binding)))))
                 (setq str
                       (concat str
-                              (make-string (max 2 (- colwidth
-                                                     (string-width str)
-                                                     (string-width binding)))
-                                           ?\s)
+                              (propertize (make-string nspaces ?\s)
+                                          'display
+                                          (cons 'space (list :width nspaces)))
                               binding)))))))
       (and km (stringp km) (setq str km))
       ;; Verify that the command is enabled;
-- 
2.46.2


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

-- 
Manuel Giraud

bug#73734: [PATCH] Fix tmm menu layout

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: rudalics@gmx.at,  73734@debbugs.gnu.org
> Date: Sat, 12 Oct 2024 17:56:42 +0200
> 
> >>         - What is the purpose of the "(min 30)"?  Because if the window
> >>           is large enough this will always return 30.
> >
> > First, I didn't write this code and don't consider myself an expert,
> > so what follows are basically my guesses.  (In the code I proposed I
> > simply didn't touch those parts to clearly separate my changes from
> > the rest.)
> >
> > I think the minimum of 30 is there to make sure we don't produce just
> > 2 sets of columns when the window is wide enough to allow more.  This
> > uses the screen real estate more efficiently, so the *Completions*
> > window is not as tall as it would be with just 2 sets of columns.
> 
> Ok, that makes sense.  I was under the impression that 30 is too small
> but you're right that it could (should?) stay the same.
> 
> >>         - Should we use frame-width instead of window-width?  It seems
> >>           that when this function is called (get-buffer-window
> >>           "*Completions*") always returns nil.
> >
> > I think we should fall back to frame-width, but use the width of the
> > *Completions* window if it exists.  I'm quite sure there's a sequence
> > of commands which will leave the *Completions* buffer on display, and
> > in that case the window showing it will be reused.
> 
> Alright, so what about this new version of the patch?  FWIW, I have
> tested it lightly and the only time the keybinding alignment is off is
> when menu entry is quite long and we needed the 2 spaces separation with
> the keybinding.

LGTM, thanks.

bug#73734: [PATCH] Fix tmm menu layout

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

> LGTM, thanks.

Sorry, I spoke too quick.  I was not testing with the same font size and
frame width.  The keybinding/shortcut confusion could still appear even
with this patch.
-- 
Manuel Giraud

bug#73734: [PATCH] Fix tmm menu layout

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi,

I don't think there is fix that will work for everyone on this issue.  I
thought that this was due to the fixed 30 value for colwidth but even
when always using computed value (using frame-width for instance), there
is always some frame-width that will exhibit the « confusion ».

I have tested with set-frame-width and decrementing it one by one.

My conclusion is that the last patch could still go in but does not
really solve the original issue.
-- 
Manuel Giraud

bug#73734: [PATCH] Fix tmm menu layout

[martin rudalics via Bug reports for GNU Emacs, the Swiss army knife of text editors]

 > I don't think there is fix that will work for everyone on this issue.  I
 > thought that this was due to the fixed 30 value for colwidth but even
 > when always using computed value (using frame-width for instance), there
 > is always some frame-width that will exhibit the « confusion ».

We could also fill the line with dots before the key or insert some sort
of vertical separators to avoid the confusion.

martin

bug#73734: [PATCH] Fix tmm menu layout

[martin rudalics via Bug reports for GNU Emacs, the Swiss army knife of text editors]

 >> Here, how I justify the modification of `colwidth':
 >>
 >>        - I don't think we need the "(min 30)" part since, if the frame is
 >>          wide enough, we always get a colwidth of 30.
 >>
 >>        - I don't think "(window-width)" is what we need since, by
 >>          default, the *Completions* buffer will use the full frame width.
 >
 > Martin, is that guaranteed?

I don't think so.  At least we should check whether the buffer window of
the *Completions* buffer satisfies the 'window-full-width-p' predicate.
But (window-width) which, as Manuel later remarks, returns the width of
the selected window, does look like a bad choice here.  I'm not familiar
with tmm.el but would

(window-width (get-buffer-window "*Completions*"))

be wrong here?

martin

bug#73734: [PATCH] Fix tmm menu layout

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

martin rudalics <rudalics@gmx.at> writes:

>>> Here, how I justify the modification of `colwidth':
>>>
>>>        - I don't think we need the "(min 30)" part since, if the frame is
>>>          wide enough, we always get a colwidth of 30.
>>>
>>>        - I don't think "(window-width)" is what we need since, by
>>>          default, the *Completions* buffer will use the full frame width.
>>
>> Martin, is that guaranteed?
>
> I don't think so.  At least we should check whether the buffer window of
> the *Completions* buffer satisfies the 'window-full-width-p' predicate.
> But (window-width) which, as Manuel later remarks, returns the width of
> the selected window, does look like a bad choice here.  I'm not familiar
> with tmm.el but would
>
> (window-width (get-buffer-window "*Completions*"))
>
> be wrong here?

I've tried this and it seems that, at the time of calling this function,
the *Completions* buffer does not exist yet.  So (get-buffer-window
"*Completions*") returns nil and we still get the current window width.
-- 
Manuel Giraud

bug#73734: [PATCH] Fix tmm menu layout

[martin rudalics via Bug reports for GNU Emacs, the Swiss army knife of text editors]

 > I've tried this and it seems that, at the time of calling this function,
 > the *Completions* buffer does not exist yet.

I suppose that it is not yet displayed in a window and changing the
order of things here would be too invasive.  'tmm-add-prompt' uses the
value of 'get-buffer-window' after checking that it returns non-nil.
But that's probably too late to determine the number of available
columns.

 > So (get-buffer-window
 > "*Completions*") returns nil and we still get the current window width.

In that case we could use 'frame-width' as fallback but if
'get-buffer-window' always returns nil that would hardly make sense.
I'm absolutely not familiar with the order how tmm does things.

martin

bug#73734: [PATCH] Fix tmm menu layout

[Eli Zaretskii]

> Date: Fri, 11 Oct 2024 10:15:24 +0200
> Cc: Eli Zaretskii <eliz@gnu.org>, 73734@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> 
>  > So (get-buffer-window
>  > "*Completions*") returns nil and we still get the current window width.
> 
> In that case we could use 'frame-width' as fallback but if
> 'get-buffer-window' always returns nil that would hardly make sense.
> I'm absolutely not familiar with the order how tmm does things.

Yes, I think we should use get-buffer-window if the buffer exists and
frame-width as fallback otherwise.