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

layout_strategy=vertical & layout_config.anchor='S' leaves one line #2851

Open
m-deg opened this issue Jan 6, 2024 · 8 comments · May be fixed by Weyaaron/telescope.nvim#2 or #3035
Open

layout_strategy=vertical & layout_config.anchor='S' leaves one line #2851

m-deg opened this issue Jan 6, 2024 · 8 comments · May be fixed by Weyaaron/telescope.nvim#2 or #3035
Labels
enhancement Enhancement to performance, inner workings or existent features good first issue Good for newcomers layouts

Comments

@m-deg
Copy link

m-deg commented Jan 6, 2024

Description

This is driving me crazy. It would be awesome if i can vertical stack prompt, previewer, results and snap it to the bottom of the page. I don't want to use ivy because it doesn't allow vertical stacking. Take a look at the screenshot below. That line "27: opts.window = opts.window or {}" is the actual buffer i am on. Telescope is on top of that line, and status line is on the bottom. I expected vertical layout strategy with anchor=S to leave no line between telescope and statusline (this is what ivy does)

image

Neovim version

deg2@DESKTOP-S6KAR7R:~/.install/nvim/nvim-linux64/bin$ ./nvim --version
NVIM v0.9.5
Build type: Release
LuaJIT 2.1.1692716794

   system vimrc file: "$VIM/sysinit.vim"
  fall-back for $VIM: "/__w/neovim/neovim/build/nvim.AppDir/usr/share/nvim"

Run :checkhealth for more info

Operating system and version

Debian 11

Telescope version / branch / rev

telescope master

checkhealth telescope

==============================================================================
telescope: require("telescope.health").check()

Checking for required plugins ~
- OK plenary installed.
- WARNING nvim-treesitter not found. (Required for `:Telescope treesitter`.)

Checking external dependencies ~
- OK rg: found ripgrep 13.0.0
- WARNING fd: not found. Install [sharkdp/fd](https://github.com/sharkdp/fd) for extended capabilities

===== Installed extensions ===== ~

Telescope Extension: `fzf` ~
- OK lib working as expected
- OK file_sorter correctly configured
- OK generic_sorter correctly configured

Telescope Extension: `live_grep_args` ~
- No healthcheck provided

Steps to reproduce

opts.layout_strategy = "vertical"
opts.border = true
opts.layout_config.anchor = "S"
opts.layout_config.prompt_position = "bottom"
opts.layout_config.width = {padding = 0}
opts.layout_config.height = 0.5
opts.layout_config.preview_height = 0.5

Expected behavior

Just like ivy theme, vertical layout strategy that is anchored to bottom of the window should be snapped right on top of statusline

Actual behavior

There is one annoying line between statusline and telescope

Minimal config

  opts.layout_strategy = "vertical"
  opts.border = true
  opts.layout_config.anchor = "S"
  opts.layout_config.prompt_position = "bottom"
  opts.layout_config.width = {padding = 0}
  opts.layout_config.height = 0.5
  opts.layout_config.preview_height = 0.5
@m-deg m-deg added the bug Something isn't working label Jan 6, 2024
@jamestrew
Copy link
Contributor

It looks to be by design (with tests)

pos[2] = math.ceil((max_lines - p_height) / 2) - 1

That - 1 being the difference maker.
I'm a little shaky on adding more config options to layout_config but I suppose it's possible to make this configurable. PR welcome.

@jamestrew jamestrew added enhancement Enhancement to performance, inner workings or existent features good first issue Good for newcomers layouts and removed bug Something isn't working labels Jan 7, 2024
@m-deg
Copy link
Author

m-deg commented Jan 7, 2024

Thanks @jamestrew , would it make sense to create a config like anchor_delta, with default value 0, and apply like below?

pos[2] = math.ceil((max_lines - p_height) / 2) - 1 + anchor_delta

I can then pass 1 to negate the -1. What do you think?

@jamestrew
Copy link
Contributor

I'm thinking maybe something more like anchor_padding that's a default of 1.
Then just do something like

  if anchor:find "W" then
    pos[1] = math.ceil((p_width - max_columns) / 2) + anchor_padding
  elseif anchor:find "E" then
    pos[1] = math.ceil((max_columns - p_width) / 2) - anchor_padding
  end
  if anchor:find "N" then
    pos[2] = math.ceil((p_height - max_lines) / 2) + anchor_padding
  elseif anchor:find "S" then
    pos[2] = math.ceil((max_lines - p_height) / 2) - anchor_padding
  end

@Weyaaron
Copy link

Weyaaron commented Mar 7, 2024

I am interested in contributing this as my first pr/contribution, looks straightforward to me. Is there anyone working at it at the moment?

@m-deg
Copy link
Author

m-deg commented Mar 17, 2024

I am not working on this @Weyaaron . Looking forward to this getting fixed tho :)

@Weyaaron
Copy link

I started work on it, I got some promising results and aim to open a pr this week :)

@Weyaaron
Copy link

Weyaaron commented Apr 7, 2024

Real-Life got in my way, but I am back on track. I got a quick question on my progress. After some digging, my current approach is adding the parameter to the resolve_anchor_pos function like this: (Somewhat cut short for brevity, it is analogous to the snippet done above).

resolver.resolve_anchor_pos = function(anchor, p_width, p_height, max_columns, max_lines, anchor_padding)
  anchor = anchor:upper()
  local pos = { 0, 0 }
  if anchor == "CENTER" then
    return pos
  end
  if anchor:find "W" then
    pos[1] = math.ceil((p_width - max_columns) / 2) + anchor_padding
  elseif anchor:find "E" then

This forces me to update all the tests. Is it sufficient to just call all the tests with 1 to mimic the old behavior, or should I add new tests?

Weyaaron added a commit to Weyaaron/telescope.nvim that referenced this issue Apr 8, 2024
Weyaaron added a commit to Weyaaron/telescope.nvim that referenced this issue Apr 8, 2024
Weyaaron added a commit to Weyaaron/telescope.nvim that referenced this issue Apr 8, 2024
Weyaaron added a commit to Weyaaron/telescope.nvim that referenced this issue Apr 8, 2024
@m-deg
Copy link
Author

m-deg commented May 21, 2024

looks good, thanks Weyaaron. @jamestrew do you think you can take a look at PR as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment