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

Add popup support to ALEHover and ALEDetails #2925

Closed
wants to merge 9 commits into from

Conversation

ian-howell
Copy link

Addresses #2922.

This causes the behavior of ALEHover and ALEDetails to change when g:ale_set_balloons = 1, causing each command to output information to a balloon rather than using the preview window.

It doesn't appear that the modified functions have tests, and I'm honestly not sure how to go about testing such a change. If anyone has any suggestions, I'll write them.

@dotdash This change is merely a slight change to the patch you proposed in #2922. If you'd like to take ownership of this change, I will gladly abandon this PR.

let l:lines = split(l:message, "\n")
call ale#preview#Show(l:lines, {'stay_here': l:stay_here})

if g:ale_set_balloons == 1 && has('balloon_eval')
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Popups are more general and separate from the balloon feature. The feature check needs to check for textprop instead of balloon_eval.

There should probably also be a separate option, since at least I want popups, but no balloons as I find the mouse hover feature rather irritating.

@dotdash
Copy link

dotdash commented Dec 6, 2019

Hey, thanks for picking this up 👍

I'd rather have you push it across the finish line than take ownership if that's fine with you.

Copy link
Member

@w0rp w0rp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the newer popups if available is a good idea. We need to check if the popup_atcursor function exists, and let's separate the popup usage from the balloons. Let's have some kind of separate option for popups.

@ian-howell ian-howell changed the title Balloonify WIP: Balloonify Jan 2, 2020
@ian-howell
Copy link
Author

I'm going to continue working on this when I find some free time. I'm planning on taking an approach similar to the strategy in #2828.

I'll squash all the commits when I'm finished.

@ian-howell
Copy link
Author

@w0rp I've added the g:ale_hover_to_popup option as well as various functionality checks.

I know that #2828 is very much fresh off the burner, but I think it may be a good idea to merge g:ale_hover_to_popup and g:ale_hover_to_preview to a single string value, perhaps g:ale_hover_window_type. That would reduce the complexity of having one option override the other.

Otherwise, this PR should be good to go - I just need to squash and clean up the PR and commit messages.

Let me know your thoughts.

This prevents popups from getting too cramped when the cursor is near
the right edge of the window
@sbromberger
Copy link

Is there any chance this might be merged soon? I'd really like to use popups instead of the preview window. Thanks.

@ilya-m32
Copy link

ilya-m32 commented Jun 22, 2020

Hi! First of all, thanks to @ian-howell for this awesome feature. I've tried this branch locally and it looks very promising.

Is there anything blocking this PR from being merged to master? If yes, I'd like to help to speed things up (if possible). Right now PR looks abandoned without a reason.

@dotdash
Copy link

dotdash commented Jun 24, 2020

@ian-howell since this actually good to go, it might be a good idea to remove the WIP prefix and then ping the maintainer(s?) again

@ian-howell ian-howell changed the title WIP: Balloonify Allow using balloons for ALEHover and ALEDetails Jun 24, 2020
@ian-howell
Copy link
Author

@w0rp This should be ready to go

@ilya-m32
Copy link

ilya-m32 commented Jul 27, 2020

@w0rp can you take a look please? Is there something left to accept the PR?

@@ -125,9 +125,15 @@ let g:ale_close_preview_on_insert = get(g:, 'ale_close_preview_on_insert', 0)
" This flag can be set to 0 to disable balloon support.
let g:ale_set_balloons = get(g:, 'ale_set_balloons', has('balloon_eval') && has('gui_running'))

" This flag can be set to 0 to disable popup support.
let g:ale_set_popups = get(g:, 'ale_set_popups', has('popupwin'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove this setting and add a separate setting for showing :ALEDetail messages in popups, and let's turn it off by default. Most of the text I look at for details will fill the screen horizontally.

Update the documentation for g:ale_cursor_detail to mention the popup behaviour. It mentions only the preview-window at the moment.

It might be easier if you split your pull request into two parts. First add popup support for hover, and then add popup support for detailed messages for problems.

@w0rp w0rp changed the title Allow using balloons for ALEHover and ALEDetails Add popup support to ALEHover and ALEDetails Aug 7, 2020
@w0rp
Copy link
Member

w0rp commented Aug 7, 2020

I'm interested in adding popup support for mouseover hovers, which could be the default. I think :ALEDetail should still default to the preview window. I also want to add support for handling markdown and converting it into Vim syntax highlighting inside of the popups.

@stale
Copy link

stale bot commented Sep 25, 2020

This pull request has been automatically marked as stale because it has not been updated recently. Make sure to write tests and document your changes. See :help ale-dev for information on writing tests.
If your pull request is good to merge, bother w0rp or another maintainer again, and get them to merge it.

@stale stale bot added the stale PRs/Issues no longer valid label Sep 25, 2020
@stale stale bot closed this Oct 2, 2020
@tribou
Copy link

tribou commented Oct 14, 2020

@w0rp should this be reopened or does the work need to continue somewhere else? I'd love to see pop-ups in ALE as well!

@w0rp
Copy link
Member

w0rp commented Oct 15, 2020

I'd like to see a new pull request which implements just one of these two uses of popups with tests. I recall from reading this that one part was mostly good, but I wasn't sure about the other part.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale PRs/Issues no longer valid
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants