emacs-lsp / lsp-mode

Emacs client/library for the Language Server Protocol
https://emacs-lsp.github.io/lsp-mode
GNU General Public License v3.0
4.72k stars 860 forks source link

fix: correct padding for lsp-modeline-progress #4438

Closed LemonBreezes closed 3 weeks ago

LemonBreezes commented 2 months ago

Hello. This PR fixes the modeline progress status squashing together with mode line item after it. The standard is that modeline items have 1 space of padding to the right.

If you think about it, this makes sense: "item_A item_B item_C ..." each item needs exactly 1 space of padding to the right. That way each item doesn't need to know where in the mode line order it is.

jcs090218 commented 2 months ago

AFAIK, all packages should have space on the left, not the right. If the package (item on the right) doesn't support this, they should support it.

LemonBreezes commented 2 months ago

AFAIK, all packages should have space on the left, not the right. If the package (item on the right) doesn't support this, they should support it.

Basically, the issue is an inconsistency with whether to pad the left or the right.

AFAIK, all packages should have space on the left, not the right. If the package (item on the right) doesn't support this, they should support it.

There seems to be some variance among packages but I have Minions (by Tarsius) and Minions pads the mode-line string with one space to the right and to the left:

(defvar minions-mode-line-modes
  (let ((recursive-edit-help-echo "Recursive edit, type C-M-c to get out"))
    (list (propertize "%[" 'help-echo recursive-edit-help-echo)
          '(:eval (car minions-mode-line-delimiters))
          `(:propertize ("" mode-name)
                        help-echo "Major mode
mouse-1: Display major mode menu
mouse-2: Show help for major mode
mouse-3: Toggle minor modes"
                        mouse-face mode-line-highlight
                        local-map ,mode-line-major-mode-keymap)
          '("" mode-line-process)
          (propertize "%n" 'help-echo "mouse-2: Remove narrowing from buffer"
                      'mouse-face 'mode-line-highlight
                      'local-map (make-mode-line-mouse-map
                                  'mouse-2 #'mode-line-widen))
          `(:propertize ("" (:eval (minions--prominent-modes)))
                        mouse-face mode-line-highlight
                        help-echo "Minor mode
mouse-1: Display minor mode menu
mouse-2: Show help for minor mode
mouse-3: Toggle minor modes"
                        local-map ,mode-line-minor-mode-keymap)
          '(:eval (and (not (member minions-mode-line-lighter '("" nil))) " "))
          '(:eval (propertize minions-mode-line-lighter
                              'face minions-mode-line-face
                              'mouse-face 'mode-line-highlight
                              'help-echo "Minions
mouse-1: Display minor modes menu"
                              'local-map minions-mode-line-minor-modes-map))
          '(:eval (cdr minions-mode-line-delimiters))
          (propertize "%]" 'help-echo recursive-edit-help-echo)
          " "))
  "Alternative mode line construct for displaying major and minor modes.
Similar to `mode-line-modes' but instead of showing (a subset
of) the enable minor modes directly in the mode line, list all
minor modes in a space conserving menu.")

and LSP's own lsp-modeline--diagnostics-string is padded one string to the right as well:

(defun lsp-modeline--diagnostics-update-modeline ()
  "Update diagnostics modeline string."
  (cl-labels ((calc-modeline ()
                             (let ((str (lsp-modeline-diagnostics-statistics)))
                               (if (string-empty-p str) ""
                                 (concat str " ")))))
    (setq lsp-modeline--diagnostics-string
          (cl-case lsp-modeline-diagnostics-scope
            (:file (or lsp-modeline--diagnostics-string
                       (calc-modeline)))
            (:workspace
             (let ((wk (car (lsp-workspaces))))
               (or (plist-get lsp-modeline--diagnostics-wks->strings wk)
                   (let ((ml (calc-modeline)))
                     (setq lsp-modeline--diagnostics-wks->strings
                           (plist-put lsp-modeline--diagnostics-wks->strings wk ml))
                     ml))))
            (:global
             (or (plist-get lsp-modeline--diagnostics-wks->strings :global)
                 (let ((ml (calc-modeline)))
                   (setq lsp-modeline--diagnostics-wks->strings
                         (plist-put lsp-modeline--diagnostics-wks->strings :global ml))
                   ml)))))))

And it seems every other segment in lsp-modeline has one space of padding to the right, so although several packages have padding to the left, the issue is that the padding in LSP is inconsistent. All other LSP segments are padded to the right but the lsp-modeline--diagnostics-string has no padding at all.

LemonBreezes commented 2 months ago

I modified this PR to only add the one space of padding to lsp-modeline--diagnostics-string because the real issue for me is when the two segments get squished together and having an extra space does not really bother me.

LemonBreezes commented 2 months ago

@jcs090218 Hello. Currently all the other mode-line segments in lsp-modeline are right-padded so I decided to make this segment right-padded as well as less changes means less chances of things breaking. I think moving from right-padded segments to left-padded segments can be done in a separate PR.

jcs090218 commented 1 month ago

Got it! Can you take screenshots for before and after comparisons? 🤔 Thank you!

LemonBreezes commented 1 month ago

Got it! Can you take screenshots for before and after comparisons? 🤔 Thank you!

Ok. This is before the code change. You can see that whether the hourglass shows up on the left or right of the warnings number is basically a race condition and if it shows up on the left then it smashes together with the warnings number and has an extra padding to the left (because of minions adding a space to the right). 20240513_121032.jpg 20240513_120624.jpg

So I think the best solution is to just add one space after the LSP check progress and to remove one space from the left of lsp-progress-prefix.

Even if we don't use Minions, the minor mode list is padded by one space to the right (which seems to be why Tarsius padded the Minions modeline to the right): 20240513_123828.jpg

LemonBreezes commented 3 weeks ago

@jcs090218

yyoncho commented 3 weeks ago

Thank you!