Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

LemonBreezes
Copy link
Contributor

@LemonBreezes LemonBreezes commented Apr 24, 2024

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
Copy link
Member

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 LemonBreezes force-pushed the fix-padding-for-lsp-modeline-progress branch from 01db3a4 to e0418e7 Compare April 26, 2024 22:13
@LemonBreezes
Copy link
Contributor Author

LemonBreezes commented Apr 26, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Member

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

@LemonBreezes LemonBreezes force-pushed the fix-padding-for-lsp-modeline-progress branch 2 times, most recently from c0df901 to fe86fe0 Compare May 13, 2024 17:20
@LemonBreezes
Copy link
Contributor Author

LemonBreezes commented May 13, 2024

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 LemonBreezes force-pushed the fix-padding-for-lsp-modeline-progress branch from fe86fe0 to 2344e9d Compare May 13, 2024 17:39
@LemonBreezes
Copy link
Contributor Author

LemonBreezes commented Jun 10, 2024

@jcs090218

@yyoncho yyoncho merged commit c7f048f into emacs-lsp:master Jun 10, 2024
10 of 13 checks passed
@yyoncho
Copy link
Member

yyoncho commented Jun 10, 2024

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants