Subject: bug#73734: [PATCH] Fix tmm menu layout

From: Manuel Giraud <[email-masked]>
Date: Sat, 12 Oct 2024 17:56:42 +0200

Eli Zaretskii <[email-masked]> writes:

>> From: Manuel Giraud <[email-masked]>
>> Cc: [email-masked],  [email-masked]
>> Date: Sat, 12 Oct 2024 16:10:10 +0200
>> 
>> Eli Zaretskii <[email-masked]> 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.

>From 3fdd7da0724ce4b335eabfd6bfed55f266909e19 Mon Sep 17 00:00:00 2001
From: Manuel Giraud <[email-masked]>
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

-- 
Manuel Giraud