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

feat: use relative = 'editor', add overlap options #44

Merged
merged 15 commits into from
Apr 3, 2024
Merged

Conversation

willothy
Copy link
Contributor

@willothy willothy commented Jul 31, 2023

When using plugins such as mini.animate, status windows on the right-hand side of the editor flicker and move when they should stay in place. This is because of the relative="win" option in the windows' config, which causes them to move automatically during resize at the same time as they're moved by the incline manager. Moving to relative="editor" and manually applying window position offsets fixes this, and results in much smoother movement.

This also allows the plugin to ignore the winbar for the top-level window if configured to do so.

I also added an option to overlap the tabline, in case anyone wants that (happy to remove that if it's not wanted).

Edit:
closes #27

@willothy
Copy link
Contributor Author

willothy commented Jul 31, 2023

I do need to add to the help file, but the doc tests are failing at the checkout step and the errors from the schema validation tests are a bit incomprehensible. Mind giving some info on what changes would be required in the config/* files to make this work? Thanks :)

Edit: actually looks like similar errors are occurring on main, so not quite sure how to proceed here.

@b0o
Copy link
Owner

b0o commented Jul 31, 2023

Thanks for these PRs. I'm going to look into the failing tests on main and will update you.

@willothy
Copy link
Contributor Author

Sounds good!

@b0o
Copy link
Owner

b0o commented Aug 26, 2023

Hi again! I'm sorry for the long delay, I was on vacation for about a month.

I fixed the failing tests and docs build on main. If you rebase on top of main, and then in test/config.lua, change your usages of Which.Is.A.DictLike to Which.Is.A.Table, the tests should pass.


Regarding the functionality, this change has a lot of edge cases to consider. There are many possible combinations of the following:

  • Is the tabline visible?
  • Is the winbar enabled/disabled?
  • Is the statusline per-window, global, or disabled?
  • Is the user's window.placement.vertical "top" or "bottom"?
  • What is the user's window.margin.vertical?
    • If it's 0, we already have special overlap behavior (described here)
    • If it's > 0, how does this interact with window.overlap?
  • If config.hide.cursorline is enabled, are we properly determining when to hide incline?

Here are a few issues I found:

  1. Using this this config:

    require'incline'.setup {
      window = {
        placement = { horizontal = 'right', vertical = 'top' },
        overlap = {
          winbar = false,
          tabline = false,
        },
        margin = {
          horizontal = 0,
          vertical = 0,
        },
      },
      hide = {
        cursorline = 'focused_win',
      },
    }

    If 'winbar' is disabled, the position of incline in the top row of windows should be line 1, but it's on line 2:
    2023-08-26_13-05-19_Alacritty

  2. Using this this config:

    require'incline'.setup {
      window = {
        placement = { horizontal = 'right', vertical = 'top' },
        overlap = {
          winbar = true,
          tabline = false,
        },
        margin = {
          horizontal = 0,
          vertical = 0,
        },
      },
      hide = {
        cursorline = 'focused_win',
      },
    }

    If 'winbar' is disabled, the position of incline is correct, but it's not hidden correctly according to hide.cursorline:
    2023-08-26_13-19-58_Alacritty
    (In the screenshot, the active cursor is on line 1 of baz.txt, so incline should be hidden in that window)

  3. Using this config:

    require('incline').setup {
      window = {
        placement = { horizontal = 'right', vertical = 'top' },
        overlap = {
          winbar = false,
          tabline = true,
        },
        margin = {
          horizontal = 0,
          vertical = 1,
        },
      },
      hide = {
        cursorline = 'focused_win',
      },
    }

    When the user has the winbar enabled, the incline window is overlapping all of the winbars:
    2023-08-26_14-16-24_Alacritty


There are also a few ambiguities:

  1. Using this config:

    require('incline').setup {
      window = {
        placement = { horizontal = 'right', vertical = 'top' },
        overlap = {
          winbar = true,
          tabline = true,
        },
        margin = {
          horizontal = 0,
          vertical = 0,
        },
      },
      hide = {
        cursorline = 'focused_win',
      },
    }

    When the user's tabline and winbar are both enabled, this is the result:

    2023-08-26_14-09-01_Alacritty

    Should the incline windows for bar.txt and qux.txt overlap the window border? Maybe we should add a window.overlap.borders option to control this (this would necessitate changing the old behavior described here).

  2. Using this config:

    require('incline').setup {
      window = {
        placement = { horizontal = 'right', vertical = 'bottom' },
        margin = {
          horizontal = 0,
          vertical = 0,
        },
      },
      hide = {
        cursorline = 'focused_win',
      },
    }

    The placement is different than it was before your changes. Before it looked like this:
    2023-08-26_13-46-21_Alacritty
    After it looks like this:
    2023-08-26_13-48-56_Alacritty

    Is this the desirable behavior? This is actually similar to what Allow overlapping window borders with window.placement.vertical = "bottom" as well #27 is requesting. Maybe we should have window.overlap.statusline and window.overlap.borders options as well to control this.


I know that's a lot to consider. Let me know if you have any thoughts or ideas. I really do like the direction this PR is going, I think if we can figure out these issues that it will be a big improvement over the current positioning behavior.

@willothy
Copy link
Contributor Author

willothy commented Aug 27, 2023

If you rebase on top of main, and then in test/config.lua, change your usages of Which.Is.A.DictLike to Which.Is.A.Table, the tests should pass.

Thanks! Will do.

Regarding the functionality, this change has a lot of edge cases to consider.

Definitely expected this to be the case, I'll start working through those this week. Thanks for the very detailed testing and info!

Maybe we should have window.overlap.statusline and window.overlap.borders options as well to control this.

I didn't intend for that to be the behavior of placement.vertical=bottom, but I think adding an option for it is a good idea. Should be trivial to implement as well.

If window.margin.vertical is 0, we already have special overlap behavior (described here)

Since this PR is already adding options for overlap, maybe this shouldn't be a special case anymore? Definitely seems more user-friendly to have a named option.

@b0o
Copy link
Owner

b0o commented Aug 27, 2023

Since this PR is already adding options for overlap, maybe this shouldn't be a special case anymore? Definitely seems more user-friendly to have a named option.

I agree, it would be better for it to be an explicit option.

@willothy
Copy link
Contributor Author

willothy commented Sep 7, 2023

I'm a bit busy right now but I'll try to get to this in the next week or two :)

@willothy
Copy link
Contributor Author

Oops I totally forgot about this PR, my bad! I definitely still want to move this forwards since I've been using it - I'll get started on those edge cases and the additional option ASAP.

@b0o
Copy link
Owner

b0o commented Dec 18, 2023

Awesome, looking forward to it!

@willothy
Copy link
Contributor Author

Oops I forgot about issue references lol my bad

@willothy
Copy link
Contributor Author

We are gonna need a lot of tests to make sure this actually works, there are so many possible configs

@willothy
Copy link
Contributor Author

Also the docs check is broken, failure is not related to this PR.

@willothy
Copy link
Contributor Author

Alright, I think this should be ready for another review pass. There may be some more edge cases to figure out since there are a lot of possible configs.

Also I will definitely simplify the main changed area as much as possible, it's definitely messier than it needs to be atm - just wanted to get it working for now lol.

@b0o
Copy link
Owner

b0o commented Jan 27, 2024

Thanks @willothy! I'll review this over the weekend. (Btw I disabled the docs workflow on PRs so that workflow failure should no longer appear)

warden7383 added a commit to warden7383/nvim that referenced this pull request Jan 28, 2024
@b0o
Copy link
Owner

b0o commented Jan 30, 2024

Overall this is looking good and feels more responsive!

Some edge cases I noticed where cursorline hiding is off by one line:

  1. When overlap.winbar = true and winbar is visible:
hide = {
  cursorline = true,
},
window = {
  margin = { horizontal = 0, vertical = 1 },
  placement = { horizontal = 'right', vertical = 'top' },
  overlap = {
    tabline = false,
    winbar = true,
    borders = false,
    statusline = false,
  },
}
  1. When overlap.tabline = true and tabline is not visible (showtabline is 0, or 1 with only one tabpage):
hide = {
  cursorline = true,
},
window = {
  margin = { horizontal = 0, vertical = 0 },
  placement = { horizontal = 'right', vertical = 'top' },
  overlap = {
    tabline = true,
    winbar = false,
    borders = false,
    statusline = false,
  },
  padding = 0,
  zindex = 49,
  winhighlight = {
    active = { Normal = 'Normal' },
    inactive = { Normal = 'Normal' },
  },
}

It would definitely be nice to figure out how to unit test the placement and hiding since there are so many edge cases, but we can figure that out later.

Other than that I think it looks good!

doc/incline.txt Outdated Show resolved Hide resolved
@willothy
Copy link
Contributor Author

Cool, glad it's working better than before! I'll look into those cursorline issues today.

@willothy
Copy link
Contributor Author

willothy commented Feb 2, 2024

Ok I think I've fixed that edge case with the tabline, and fixed another I found where if both tabline and winbar were hidden the window would not be visible.

@b0o
Copy link
Owner

b0o commented Feb 2, 2024

Nice! Seems like there's only one edge case now: When overlap.winbar = false and winbar is visible, cursorline hiding is off by one.

@willothy
Copy link
Contributor Author

willothy commented Feb 2, 2024

Awesome! I'll try and get that fixed today. I think I might've broken that with the last fix lol

@willothy
Copy link
Contributor Author

willothy commented Apr 1, 2024

Ahhhh I hope that didn't re-break something else lol

@b0o
Copy link
Owner

b0o commented Apr 3, 2024

That still isn't quite working. This seems to fix it:

diff --git a/lua/incline/winline.lua b/lua/incline/winline.lua
index c687b8d..aa2ceed 100644
--- a/lua/incline/winline.lua
+++ b/lua/incline/winline.lua
@@ -70,7 +70,7 @@ function Winline:get_win_geom_row()
     -- statusline if laststatus is not 3
 
     -- TODO(willothy): this can obviously be simplified a lot, there is a good bit of repetition
-    if vim.o.laststatus ~= 3 or a.nvim_win_get_position(self.target_win)[1] <= 1 then
+    if a.nvim_win_get_position(self.target_win)[1] <= 1 then
       if cw.margin.vertical.top == 0 then
         if
           config.window.overlap.tabline
@@ -223,7 +223,8 @@ function Winline:render(opts)
   end
   if
     (config.hide.cursorline == true or (config.hide.cursorline == 'focused_win' and self.focused))
-    and self:get_win_geom_row() == a.nvim_win_call(self.target_win, vim.fn.winline)
+    and (self:get_win_geom_row() + ((vim.wo[self.target_win].winbar == '') and 1 or 0))
+      == a.nvim_win_call(self.target_win, vim.fn.winline)
   then
     self:hide(HIDE_TEMP)
     return

Would you mind testing that and, if it seems to work for you, making a commit?

@willothy
Copy link
Contributor Author

willothy commented Apr 3, 2024

Looks like that works!

@b0o b0o merged commit 9136957 into b0o:main Apr 3, 2024
1 check passed
@b0o
Copy link
Owner

b0o commented Apr 3, 2024

Awesome, thank all of your work and persistence!

@willothy
Copy link
Contributor Author

willothy commented Apr 3, 2024

Super exciting! Took a while but I think the flexibility is awesome.

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.

Allow overlapping window borders with window.placement.vertical = "bottom" as well
2 participants