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

Allow callbacks for floating preview popups #4247

Merged
merged 9 commits into from
Aug 23, 2022

Conversation

shaunduncan
Copy link
Contributor

I'm not sure if this PR will be useful to anyone else, but I wanted a better way to customize how the floating preview popup was being displayed. The simplest way I could think to do this was to just allow a callback function that could supply custom popup options.

I have added some documentation for this and I am currently using it with vim 8.2. I haven't tried it with neovim but I will assume it doesn't work (I don't use neovim so I can't say for sure).

Just wanted to share :)

@michaelorr
Copy link

Yesssss. I love the floating window but having it in the upper right corner (for example) is way better than covering up the context around the issue. This allows for reasonable defaults but also full customization, love it

@hsanson
Copy link
Contributor

hsanson commented Jul 12, 2022

This sounds very similar to what #4221 wants to achieve.

@michaelorr
Copy link

@hsanson definitely similar in concept. I'd love to see either change land.
I've been using this patch daily for the past month and a half with great results and would love to see it merged

autoload/ale/floating_preview.vim Outdated Show resolved Hide resolved
doc/ale.txt Show resolved Hide resolved
doc/ale.txt Show resolved Hide resolved
@hsanson
Copy link
Contributor

hsanson commented Aug 16, 2022

I think this kind of new functionality should be implemented in a way compatible with both vim and neovim. Unfortuntelly I do not have resources (time) to try figuring out a common ground that would allow this to work equally on both.

Also would be nice if we spend some time trying to figure out how to write tests for this functionality to ensure future changes in ALE, vim, or neovim do not break it.

@shaunduncan
Copy link
Contributor Author

shaunduncan commented Aug 17, 2022

@hsanson I've included changes based on your suggestions. I am not well-versed enough on vim-vader tests to write some otherwise I would be happy to do so. I did, however, add the callback for showing the neovim floating preview. I am not a regular neovim user so it is somewhat difficult to test, but by doing a quick check it appeared to honor my configuration.

hsanson
hsanson previously approved these changes Aug 20, 2022
Copy link
Contributor

@hsanson hsanson left a comment

Choose a reason for hiding this comment

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

Thanks, looks good.

@hsanson
Copy link
Contributor

hsanson commented Aug 20, 2022

CI failed with some linter issues. Please add ale custom linter to your config so vim informs you of these issue:

'vim': ['ale_custom_linting_rules']

@shaunduncan
Copy link
Contributor Author

@hsanson I've made corrections based on the lint errors. For some reason though adding the vim lint configuration never showed any errors for me. Not sure why. Hopefully I've made the changes correctly.

Copy link
Contributor

@hsanson hsanson left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution.

@hsanson hsanson merged commit 6996d1c into dense-analysis:master Aug 23, 2022
mnikulin pushed a commit to mnikulin/ale that referenced this pull request Nov 12, 2023
* Add extra config options for virtualtext

* Undo virtualtext changes and move to floating preview

* revert changes to pass hightlight group to floating preview

* rename var

* Document changes

* Add updates based on feedback

* Check for string type and attempt to call the function

* Fix lint errors

Co-authored-by: Shaun Duncan <shaun@speedscale.com>
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.

None yet

3 participants