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 terminal_info_inline_borders option #5102

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

raiguard
Copy link
Contributor

Before:
image

After:
image

@@ -175,6 +175,7 @@ private:

ColumnCount m_status_len = 0;
ColumnCount m_info_max_width = 0;
bool m_info_inline_borders = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this should be the default except maybe for tiny info boxes that only span one line.

Also isn't it a bit weird that this is an option? I can imagine wanting two info boxes with different styles in the same window. For example lsp-signature-help is just one line so it shouldn't have borders while lsp-hover does want borders. So shouldn't it be a flag to info? Even more so because info already has a border by default. Only info -style doesn't

Copy link
Contributor Author

@raiguard raiguard Feb 18, 2024

Choose a reason for hiding this comment

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

The idea behind the option was to avoid changing the default behavior, because I assumed the default behavior was the way it was for a reason. I would not complain if the default was changed and the option was removed.

As for single line boxes, I think it should still draw borders then. The whole point is that with my color scheme it is really difficult to distinguish what's in an info box and not because it doesn't use a super high contrast background.

Copy link
Owner

Choose a reason for hiding this comment

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

This is a tricky problem, the reason i did not make the inline info bordered was to make it possible to align info with the anchor position. For example ctags-enable-autoinfo will display the info box aligned on the ( opening the function call. Additionally, borders on small, inline info boxes hide a lot more of the buffer.

I initially thought a switch on the info command made most sense, but info box borders are a UI only concept, so adding this to the info command would move that knowledge in the UI agnostic part of Kakoune. I think we should find a switch/info style name that hints the UI it can use borders. That said, it seems the root of the issue is @raiguard colorscheme not distinguishing clearly info boxes background, maybe changing that would be a better fix.

Copy link
Contributor Author

@raiguard raiguard Feb 21, 2024

Choose a reason for hiding this comment

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

The reason my colorscheme has a low-contrast info box background is so that I can have syntax coloring in the info boxes. Especially in large projects, it makes LSP information much easier to parse:

One Darker:
image

Gruvbox Dark:
image

I have trouble reading (potentially dyslexia, but not diagnosed yet) so syntax highlighting is an enormous help for me.

Copy link
Owner

Choose a reason for hiding this comment

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

Wouldn't it work to use a high enough color difference between Default and Information backgrounds so that its clear you are looking at an info box while still keeping Information background dark enough so that highlighted code looks okay ?

Copy link
Contributor Author

@raiguard raiguard Mar 27, 2024

Choose a reason for hiding this comment

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

Perhaps. But the reality is that having the border will always make it easier to read. Instead of having characters directly next to each other only separated by color, it has a very clear and easy to parse separation. In my opinion it is a significant improvement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
With ctags-enable-autoinfo

Copy link
Owner

Choose a reason for hiding this comment

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

Yep, this autoinfo was trying to align its opening ( to the one in the file (it is not anchored on the cursor), but cannot anymore because it would need to predict if a border is applied and how many characters it adds. Once you remove the ability to align it begs the question of why we have parenthesis in that info box in the first place...

On the other hand, I was envisioning the info boxes to be allowed to use a different font / font size on GUI implementations, which would also introduce the same issue breaking info alignement to buffer text.

I am still not fully sold on that change because it feels like allowing scripts to align info to buffer content is a nice feature, and I think the readability concern can be solved with the colorscheme.

@raiguard raiguard force-pushed the terminal-ui-info-inline-borders branch 2 times, most recently from e90308d to 1e66089 Compare March 13, 2024 16:40
@raiguard raiguard force-pushed the terminal-ui-info-inline-borders branch 3 times, most recently from b5aecaa to 077e9e7 Compare April 3, 2024 01:40
@raiguard raiguard force-pushed the terminal-ui-info-inline-borders branch from 077e9e7 to 1545bca Compare April 21, 2024 08:23
@raiguard raiguard force-pushed the terminal-ui-info-inline-borders branch from 1545bca to 07fcc97 Compare April 29, 2024 07:07
@raiguard raiguard force-pushed the terminal-ui-info-inline-borders branch from 07fcc97 to dd510be Compare June 3, 2024 08:37
@raiguard raiguard force-pushed the terminal-ui-info-inline-borders branch from dd510be to 161b9e7 Compare June 25, 2024 21:48
@raiguard raiguard force-pushed the terminal-ui-info-inline-borders branch from 161b9e7 to 66b550f Compare October 18, 2024 20:55
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