search

emacs-lsp / lsp-ui

UI integrations for lsp-mode
https://emacs-lsp.github.io/lsp-ui
GNU General Public License v3.0
1.03k stars 139 forks source link

Adjust lsp-ui-doc overlay inner padding (when not using a childframe) #546

Closed apiraino closed 3 years ago

apiraino commented 3 years ago

Hi,

this is an attempt to study the problem I've reported in #489. I've dug a bit into the code and I think the root of the issue is that lsp-ui has not a great support for emacsen that don't use a GUI (either running emacs -nw or without the GTK libs compiled in). Note: this is not meant to be a critique, just to frame the issue.

When not using the GUI childframe (i.e. lsp-ui-doc-use-childframe nil) it's not clear to me how the overlay is created and if I can interact with it. For example I cannot scroll a long page of documentation:

2021-01-01_180835

I understand the technical limits of a non-GUI overlay, I probably couldnt improve a lot here, but this PR is trying to fix at least one issue about the incorrect padding calculated. For some reason the overlay padding from the right border is not enough, therefore each line exceeding the overlay width "bleeds" into the main buffer messing the view (see my issue report).

I'm not converstant with elisp, so I'm very open to any explaination and suggestion to improve this patch.

Thanks for a review and for working on lsp-ui! :+1:

jcs090218 commented 3 years ago

Not sure if you can use let operator this way. Let's try this.

(defun lsp-ui-doc--inline-window-width nil
  "Calculate inner padding for the child frame showing the documentation"
  (let ((additional-width-spacing (if (bound-and-true-p lsp-ui-doc-use-childframe)
                                      8 1)))
    (- (min (window-text-width) (window-body-width))
       (if (bound-and-true-p display-line-numbers-mode)
           (+ 2 (line-number-display-width)) 0)
       additional-width-spacing)))

This is a much simplify version of function lsp-ui-doc--inline-window-width.

apiraino commented 3 years ago

embarassing how little elisp I know :smile:

and now the CI pass! thank you so much @jcs090218 for your help

jcs090218 commented 3 years ago

@apiraino No problem! Glad to help out!

Can someone test this patch? @sebastiencs @brotzeit

apiraino commented 3 years ago

Unfortunately I think there are still issues with the overlay width. Simply setting a larger value (8 instead of 1) is not useful. I am testing with more code and for example the overlay breaks with long function signatures (example +100 characters).

I welcome a review and suggestions, but I'd keep this PR in standby until I figure out a better approach (example: by dynamically calculating the width based on the longest text line that should fit).

jcs090218 commented 3 years ago

I might try to avoid magic number like 8 or 1. Calculating the padding by longest text lines sounds like a good approach. ;)

apiraino commented 3 years ago

I think I'll close this and clean up some mess I've left. I couldn't get around to fixing this and I can't reproduce it anymore after migrating to another Emacs config