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 customization of all floating window borders #4215

Merged
merged 2 commits into from
May 27, 2022

Conversation

djpohly
Copy link
Contributor

@djpohly djpohly commented May 25, 2022

Users may not necessarily want the same border character for top+bottom or left+right, so allow all eight border characters to be configured in g:ale_floating_window_border.

For backwards compatibility, the old rules are still applied if only six elements are given.

Users may not necessarily want the same border character for top+bottom
or left+right, so allow all eight border characters to be configured in
g:ale_floating_window_border.

For backwards compatibility, the old rules are still applied if only six
elements are given.
@djpohly
Copy link
Contributor Author

djpohly commented May 26, 2022

As I made this, I wondered if it would be a even better solution to allow the user to specify a g:ale_floating_window_options dictionary that is merged into the options for popup_create(). I'd be willing to code that up if it seems better to the devs.

@hsanson
Copy link
Contributor

hsanson commented May 26, 2022

The current array configuration is good enough I think. If we allow users to set all possible options to popup_create we must be careful dealing with differences between vim popup_create and neovim nvim_open_win functions.

@djpohly
Copy link
Contributor Author

djpohly commented May 27, 2022

I see - does ALE intentionally try to abstract away any differences between the two? (Unfortunately Neovim decided to put the array in a different order from Vim when they implemented borders... do they do this to us on purpose?)

I was thinking ahead to ways I'd like to be able to customize the appearance of ALE popups, and it seemed like the option names could get unwieldy (g:ale_floating_window_border_highlight_groups??). Would it make sense to group these into a dictionary if it's not given directly to either application's window function but rather translated by ALE itself to be cross-"platform"?

(These are sort of tangential to the PR, so I can ask elsewhere if there's a better place; I'm just interested in making the ALE popups look... better.)

@hsanson
Copy link
Contributor

hsanson commented May 27, 2022

ALE intentionally try to abstract away any differences between the two?

Not really but seems natural that ALE functions seamessly on both Vim and Neovim with the same configuration.

do they do this to us on purpose?

Cannot say but I envision the situation will get even worse as Neovim moves to Lua and features start diverging more.

Would it make sense to group these into a dictionary if it's not given directly to either application's window function but rather translated by ALE itself to be cross-"platform"?

Yes, makes sense to me at least. If we are going to be adding more options to customize the look of windows then having 10 different arrays would be confusing and error prone to users.

I'm just interested in making the ALE popups look... better.

Great!!, any contributions are welcome. I did some thinkering myself and my conclusion was that eyecandy is highly subjective (everyone has different preference) and it falls outside ALE's main scope.

Instead of ALE implementing eyecandy features that people can customize to their liking I think is better if ALE provides ways to integrate with other plugins that are specifically created for this like nvim-notify, and fidget.

ALE can provide bare minimum visual tools like current floating windows, preview, echo, virtual-text, and add some callbacks that allow users to plug their own window rendering. Similar to how integration with other auto-completion plugins work currently.

Of course does not have to be a plugin. A user that knows how to use popup_create() and nvim_create_win() functions can override the callback to customize the windows to their liking.

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 changes. Looks good.

@hsanson hsanson merged commit ae44f05 into dense-analysis:master May 27, 2022
@djpohly djpohly deleted the popup-full-border branch May 27, 2022 18:27
@djpohly
Copy link
Contributor Author

djpohly commented May 27, 2022

Of course does not have to be a plugin. A user that knows how to use popup_create() and nvim_create_win() functions can override the callback to customize the windows to their liking.

Thanks - I didn't even realize ALE had customizable callbacks for displaying info! I haven't found anything in the README, but I'll dig through ale.txt and the source if necessary to see what I can learn. If you know offhand of a better explanation somewhere, let me know where to look!

@hsanson
Copy link
Contributor

hsanson commented May 27, 2022

Sorry, what I meant was that we should implement those callbacks instead of adding options for customization. These callbacks still do not exist in ALE.

cyyever pushed a commit to cyyever/ale that referenced this pull request Jul 11, 2022
* Allow customization of all floating window borders

Users may not necessarily want the same border character for top+bottom
or left+right, so allow all eight border characters to be configured in
g:ale_floating_window_border.

For backwards compatibility, the old rules are still applied if only six
elements are given.

* Reorder popup border array for compatibility
cyyever pushed a commit to cyyever/ale that referenced this pull request Jul 11, 2022
* Allow customization of all floating window borders

Users may not necessarily want the same border character for top+bottom
or left+right, so allow all eight border characters to be configured in
g:ale_floating_window_border.

For backwards compatibility, the old rules are still applied if only six
elements are given.

* Reorder popup border array for compatibility
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

2 participants