feat: allow `Border`s and `popup`s to be resized #180

l-kershaw · 2021-07-04T15:27:58Z

Adds a function to resize a Border based on updated content_win_options and border_win_options

Initial commit is a first attempt and probably needs looking through closely to make sure I haven't missed anything.

- adds table to keep track of `Border`s associated to `popup`s - adds function to resize a `popup` - relies on nvim-lua/plenary.nvim#180

l-kershaw · 2021-07-04T15:41:57Z

@Conni2461 could you have a look at this and nvim-lua/popup.nvim#15 and let me know what you think.

I'm trying to set something up that would be useful for rearranging windows within telescope.nvim without breaking any of the other plugins that use either plenary.nvim or popup.nvim.

Conni2461 · 2021-07-04T16:50:28Z

This looks already great. Thanks for tackling that :) (We should reduce duplication between, :new and :resize tho)

I am thinking that with a api like that we can resolve these telescope issues:

Am i missing something that you want to do ("rearranging windows")

Also we need to see that in action at the same time, interested in developing vimresize or toggle preview, while implementing this? vimresize can maybe even happen on popup.nvim level, right?

Oh and right now i do not think you break other plugins, you are only extending api :) So thanks again

l-kershaw · 2021-07-04T19:24:58Z

Cool, I'll refactor stuff to reduce overlap between :new and :resize.

Those were the main uses in telescope that I was thinking of, but in principle this should allow any layout changes wanted for telescope (e.g. changing layout strategy on the fly, resizing the results window to fit things in,...).

Yeh, I was working on stuff to get VimResized working with this so I'll do that at the same time. As long as the API is robust enough, implementing other stuff shouldn't be too difficult afterwards.

- rename `Border:resize` to `Border:set_size` - allow `Border:set_size` to be passed on option to create a window rather than resize - remove overlapped parts from `Border:new` and use `Border:set_size` with `create_window` set to `true`

- implements `Picker:recalculate_layout` function - adds an autocommand to call this function on the `VimResized` event - relies on nvim-lua/plenary.nvim#180 and nvim-lua/popup.nvim#15 Currently has issues when the results window increases in height

l-kershaw · 2021-08-24T20:36:34Z

I have added the functionality from the popup PR and closed that PR.

I think this functionality is complete enough to merge now, I can't think of anything else to add.

@Conni2461 could you have a look and let me know your thoughts?

lua/plenary/window/border.lua

fdschmidt93 · 2021-08-30T08:28:26Z

Just 1-2 suggestions from my end but otherwise good job, looking forward for toggleable preview and VimResized :)

l-kershaw · 2021-09-04T18:08:33Z

I think the failed checks are due to neovim removing a "nightly" tagged release?

I still have a few things to change based on @fdschmidt93's feedback, but thought it best to mention this first as this will affect checks for any other PRs for plenary (and I think telescope too).

l-kershaw · 2021-09-07T15:56:35Z

I've had a bit more of a think about it and I think it would be better to factor out the posisitioning stuff from popup.create in a way that can be reused in popup.move. That way we get stuff like cursor relative positioning and pos handling for free without repeating a load of code.

I'll have a go at this today/tomorrow, and then hopefully the neovim nightly release is back available so the tests work again.

Also, once this and #227 are merged, I was thinking about refactoring popup in an object oriented way (as done with Border), which I think will make it easier to use, so you don't have to keep track of a load of win_ids everywhere.
It's not really an issue at the moment, as you currently just "set and forget" a popup, but once we're changing position and/or other config, it gets more confusing. Particularly for implementing popup_hide and popup_show this will make things a lot simpler.
I spoke with @tjdevries about this on discord a while ago, but would be good to get some other opinions too (@Conni2461 @fdschmidt93)

fdschmidt93 · 2021-09-07T16:08:13Z

Completely agree and also had similar things in mind. I briefly glanced at https://github.com/MunifTanjim/nui.nvim and fzf-lua's implementation which all around both look a lot more ergonomic pretty much going that route.

l-kershaw · 2021-09-07T20:15:51Z

Okay, from my perspective this is now ready to merge.
@Conni2461 could you let me know your thoughts?

@fdschmidt93 yes, nui.nvim was part of what inspired the discussion for using an object based model for popups. It's been a while since I looked at it, but from memory there were a few parts of the API that looked a lot easier to use than our current setup.

Conni2461 · 2021-09-08T07:05:22Z

Also, once this and #227 are merged, I was thinking about refactoring popup in an object oriented way (as done with Border), which I think will make it easier to use, so you don't have to keep track of a load of win_ids everywhere.

tj said exactly the same thing to me a month ago 🤣 i just quote him

my answer was kinda like that. Good idea we should focus on building what works on us and we can always build the popup_* interface on top of that object oriented implementation later.

Conni2461

I think this looks good. Thanks :)

I'll run it for the day and maybe do some popup_move this evening, after that i would merge

Conni2461 · 2021-09-08T07:21:09Z

Oh also. If you do that popup refactor, do you want "access", permissions to merge stuff. I havent talked with tj about that (just came to my mind) but i hardly think he will have a issue with that 😆

If you want i can give you permissions later. Maybe that easier than always waiting for me 🤣

Conni2461 · 2021-09-08T07:51:42Z

I actually have issues with telescope, do we need some adjustments their?

Screenshots to demonstrate this behavior (i need to censor stuff that i am not allowed to share, sorry) but long story short i only see one result. But both show 132/132 results found
This PR

Master

l-kershaw · 2021-09-08T09:23:30Z

@Conni2461

If you want i can give you permissions later. Maybe that easier than always waiting for me 🤣

I'm happy to write the PR either way. If it makes it easier to have an extra person who can merge stuff for plenary, then I'm happy to help out with reviewing stuff over here.

I actually have issues with telescope, do we need some adjustments their?

Ah, that's weird. I hadn't had any issues, but was working with my layout refactor branch for telescope (see here).
When I switched to master, I had the same issue as you.
I don't really know how the PR branch would change this behaviour, but will have a dig through to see what I can figure out.

Conni2461 · 2021-09-08T16:34:04Z

Hmm maybe that would solve the CI in telescope 🤔 You could try switching the branch ci pulls in your telescope branch, maybe that fixes ci then could merge both at the same time.

I could run both PRs for the rest of the week. I think is super busy lately (i have caught him playing dota 🤣) the stuff i saw from that pr looked good i just also didnt understand the failing ci.

l-kershaw · 2021-09-08T17:16:51Z

@Conni2461 as suggested I tried switching the branch for ci in the telescope PR, but it doesn't fix the issues over there
Also, it set off merge conflicts, and after the merge, the issue is now present with that PR as well as master 😅

Don't really know what's going on now 🤣 Will see if I can figure stuff out

l-kershaw · 2021-09-09T17:02:31Z

@Conni2461 I've tried to find the point in the commit history for telescope where stuff diverged enough to cause this issue and it is before we move popup into plenary, so I can't find the root cause.
I think it is probably best to wait for the layout refactor branch to be merged into telescope, and then we can decipher the cause of this issue and probably write a fix within telescope for it. Until then, I think this PR is blocked 😢

- this then matches terminology for popups

l-kershaw · 2021-09-26T13:23:49Z

@Conni2461 I think I have fixed the stuff in this PR that was breaking telescope. So I think this PR could be merged without negatively affecting anything in telescope.

I don't think there is anything more to do on the plenary side in terms of moving popups around, but for resizing layouts in telescope there is still some work to do in terms of handling the results buffer. I'm happy to hold off on merging this in case we need some extra changes in plenary to get stuff to work, but it does seem like more of a telescope issue causing problems over there now.

Conni2461 · 2021-09-26T14:13:09Z

I tested it and i havent seen any regressions for telescope, so i dont see any reason why not merging right now :)

Thanks for all the hard work :)

- implements `Picker:recalculate_layout` function - adds an autocommand to call this function on the `VimResized` event - relies on nvim-lua/plenary.nvim#180 and nvim-lua/popup.nvim#15 Currently has issues when the results window increases in height

- implements `Picker:recalculate_layout` function - adds an autocommand to call this function on the `VimResized` event - relies on nvim-lua/plenary.nvim#180 and nvim-lua/popup.nvim#15 Currently has issues when the results window increases in height lint

feat: allow Borders to be resized

bb33ec3

l-kershaw added a commit to l-kershaw/popup.nvim that referenced this pull request Jul 4, 2021

feat: allow popups to be resized

9ebf0ce

- adds table to keep track of `Border`s associated to `popup`s - adds function to resize a `popup` - relies on nvim-lua/plenary.nvim#180

l-kershaw mentioned this pull request Jul 4, 2021

WIP: allow popups to be resized nvim-lua/popup.nvim#15

Closed

l-kershaw mentioned this pull request Jul 5, 2021

feat: recalculate picker layout on VimResized nvim-telescope/telescope.nvim#959

Merged

l-kershaw added 4 commits August 24, 2021 18:58

Merge branch 'master' into feat/border_resize

277ba41

stylua

9bee005

feat: move popup resizing PR to plenary

68694e2

stylua

4624cbf

l-kershaw changed the title ~~WIP: allow Borders to be resized~~ WIP: allow Borders and popups to be resized Aug 24, 2021

l-kershaw changed the title ~~WIP: allow Borders and popups to be resized~~ feat: allow Borders and popups to be resized Aug 24, 2021

fdschmidt93 reviewed Aug 30, 2021

View reviewed changes

lua/plenary/window/border.lua Outdated Show resolved Hide resolved

fdschmidt93 reviewed Aug 30, 2021

View reviewed changes

lua/plenary/window/border.lua Outdated Show resolved Hide resolved

l-kershaw added 2 commits September 4, 2021 18:10

Merge branch 'master' into feat/border_resize

3bb4f86

chore: refactoring

75b0448

l-kershaw added 4 commits September 4, 2021 20:15

refactor: use builtin nil-check for create_window

52d00ac

refactor: factor out __update_lines_get_win_config

c49a622

fix: update __update_lines... name everywhere

c74cfad

fix: variable scope issue

4170030

l-kershaw mentioned this pull request Sep 4, 2021

feat: allow pos="center", row=0, line=0 #227

Merged

l-kershaw added 2 commits September 7, 2021 14:14

refactor: rename aligning function

cfdda64

refactor: rename to match vim's popup_move

772f4a4

l-kershaw added 4 commits September 7, 2021 19:09

refactor: factor our positional config parsing to reuse in popup.move

5c860dd

stylua

31cd991

chore: docs/add default height

8be532b

docs: comments

7bd4de3

Conni2461 approved these changes Sep 8, 2021

View reviewed changes

l-kershaw added 7 commits September 22, 2021 21:37

Merge branch 'master' into feat/border_resize

fc06d94

fix: deepcopy vim_options to avoid changes to supplied variables

ff2f3f2

refactor: rename set_size to move

86e34db

- this then matches terminology for popups

fix: update Border table values on move

4ab5362

stylua

4b0e018

refactor: calculate size before position

5e8ff87

stylua

8874495

Conni2461 merged commit 8c6cc07 into nvim-lua:master Sep 26, 2021

l-kershaw deleted the feat/border_resize branch September 26, 2021 14:14

Uh oh!

feat: allow Borders and popups to be resized #180

feat: allow Borders and popups to be resized #180

Uh oh!

Conversation

l-kershaw commented Jul 4, 2021

Uh oh!

l-kershaw commented Jul 4, 2021

Uh oh!

Conni2461 commented Jul 4, 2021

Uh oh!

l-kershaw commented Jul 4, 2021

Uh oh!

l-kershaw commented Aug 24, 2021

Uh oh!

Uh oh!

Uh oh!

fdschmidt93 commented Aug 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

l-kershaw commented Sep 4, 2021

Uh oh!

l-kershaw commented Sep 7, 2021

Uh oh!

fdschmidt93 commented Sep 7, 2021

Uh oh!

l-kershaw commented Sep 7, 2021

Uh oh!

Conni2461 commented Sep 8, 2021

Uh oh!

Conni2461 left a comment

Choose a reason for hiding this comment

Uh oh!

Conni2461 commented Sep 8, 2021

Uh oh!

Conni2461 commented Sep 8, 2021

Uh oh!

l-kershaw commented Sep 8, 2021

Uh oh!

Conni2461 commented Sep 8, 2021

Uh oh!

l-kershaw commented Sep 8, 2021

Uh oh!

l-kershaw commented Sep 9, 2021

Uh oh!

l-kershaw commented Sep 26, 2021

Uh oh!

Conni2461 commented Sep 26, 2021

Uh oh!

Uh oh!

feat: allow `Border`s and `popup`s to be resized #180

feat: allow `Border`s and `popup`s to be resized #180

fdschmidt93 commented Aug 30, 2021 •

edited

Loading