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.

Sign up for GitHub

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

Jump to bottom

Add terminal_info_inline_borders option #5102

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

Add terminal_info_inline_borders option #5102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions doc/pages/options.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,10 @@ are exclusively available to built-in options.
set the maximum allowable width of an info box. set to zero for
no limit.

*terminal_info_inline_borders*:::
if *yes* or *true*, borders will be drawn around info boxes with the
*inline* style.

[[startup-info]]
*startup_info_version* `int`::
_default_ 0 +
Expand Down
3 changes: 2 additions & 1 deletion src/main.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -597,7 +597,8 @@ void register_options()
" terminal_shift_function_key int\n"
" terminal_padding_char codepoint\n"
" terminal_padding_fill bool\n"
" terminal_info_max_width int\n",
" terminal_info_max_width int\n"
" terminal_info_inline_borders bool\n",
UserInterface::Options{});
reg.declare_option("modelinefmt", "format string used to generate the modeline",
"%val{bufname} %val{cursor_line}:%val{cursor_char_column} {{context_info}} {{mode_info}} - %val{client}@[%val{session}]"_str);
Expand Down
5 changes: 3 additions & 2 deletions src/terminal_ui.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,7 @@ void TerminalUI::Screen::output(bool force, bool synchronized, Writer& writer)
{
for (int line = 0; line < (int)size.line; ++line)
{
auto hash = hash_line(lines[line]);
auto hash = hash_line(lines[line]);
if (hash == hashes[line])
continue;
hashes[line] = hash;
Expand Down Expand Up @@ -1300,7 +1300,7 @@ void TerminalUI::info_show(const DisplayLine& title, const DisplayLineList& cont
m_info.face = face;
m_info.style = style;

const bool framed = style == InfoStyle::Prompt or style == InfoStyle::Modal;
const bool framed = style == InfoStyle::Prompt or style == InfoStyle::Modal or m_info_inline_borders;
const bool assisted = style == InfoStyle::Prompt and m_assistant.size() != 0;

DisplayCoord max_size = m_dimensions;
Expand Down Expand Up @@ -1555,6 +1555,7 @@ void TerminalUI::set_ui_options(const Options& options)
m_padding_fill = find("terminal_padding_fill").map(to_bool).value_or(false);

m_info_max_width = find("terminal_info_max_width").map(str_to_int_ifp).value_or(0);
m_info_inline_borders = find("terminal_info_inline_borders").map(to_bool).value_or(false);
}

}
1 change: 1 addition & 0 deletions src/terminal_ui.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited

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 •
edited

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 •
edited

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.

};

}
Expand Down