mrjones2014 / smart-splits.nvim

🧠 Smart, seamless, directional navigation and resizing of Neovim + terminal multiplexer splits. Supports tmux, Wezterm, and Kitty. Think about splits in terms of "up/down/left/right".
MIT License
973 stars 43 forks source link

feat(api): customizable `at_edge` behavior #80

Closed mrjones2014 closed 1 year ago

mrjones2014 commented 1 year ago

Changes config.wrap_at_edge = boolean to config.at_edge = 'wrap'|'split'|'stop'.

Fixes #79

mrjones2014 commented 1 year ago

@IndianBoy42 give this a test please

IndianBoy42 commented 1 year ago

at_edge="split" certainly works how I expected, thanks!

I dont use any mux so I can't check that,

mrjones2014 commented 1 year ago

Cool, I'm going to use this branch for a while to make sure I'm not introducing any regressions before merging.

mrjones2014 commented 1 year ago

@mehalter I'd like to get your thoughts on whether you think these types of changes should be considered breaking changes, and therefore require a Major release.

Technically nothing is "broken," as the changes in this PR will migrate configuration settings to their new equivalents, however, you'll see a deprecation notice if you've explicitly set any of the affected settings in config.

For example, if you have either of these explicitly in config:

require('smart-splits').setup({ wrap_at_edge = true })
-- or
require('smart-splits').setup({ wrap_at_edge = false })

Then the plugin will migrate this to the new config.at_edge = 'wrap' or config.at_edge = 'stop', respectively, and you'll get a deprecation notice that config.wrap_at_edge is deprecated, and use to use config.at_edge = 'wrap'|'split'|'stop' instead.

So I guess the tl;dr is: Should it be considered a "breaking change" if nothing is technically "broken," but explicitly setting the deprecated configs will print a deprecation notice using vim.deprecate()?

I'm bringing this up given the context that I recently learned this plugin is used in AstroNvim, and I want to know how you'd like to see the versioning scheme fit here.

mrjones2014 commented 1 year ago

Going to merge, but would still like some feedback about next release tag from @mehalter.

mehalter commented 1 year ago

@mrjones2014 I think tagging those options as deprecated and leaving some backwards compatibility for the true/false notation is definitely alright to avoid a breaking version right now! I will update AstroNvim's core code after you tag the next release (non-breaking) and then people who are using true/false as an override (which I am imagining are not many people) will be able to update their code.

mehalter commented 1 year ago

also sorry about the late reply, was out of town for the past few days and totally forgot to go back and answer this! Thanks so much for the heads up :) Love the community and joint collaboration ❤️

mehalter commented 1 year ago

Also just realized that AstroNvim doesn't actually override this setting, so the base astronvim doesn't require a code update, and it will just affect users who override this explicitly which there is a certain level of responsibility taken when customizing AstroNvim 😄

mrjones2014 commented 1 year ago

also sorry about the late reply

No worries!

Thanks for that feedback, I will tag a minor release shortly then! 💪

mehalter commented 1 year ago

@mrjones2014 just while we are chatting here and to avoid making an issue. Do you think it would make sense to have at_edge = "split" respect ignored_buftypes? Basically if I am in an ignored buftype and move in a direction that would create a split I think it would make sense to ignore it. It gets a bit weird if i'm in my tree for example on the left hand side (like a sidebar), moving left on accident, and getting a second tree split also on the side. Let me know what you think :)

mrjones2014 commented 1 year ago

Hmm, generally yeah that sounds like a good idea, but it might get a bit weird in certain edge cases, but maybe we can work around those.

What happens if your edge is an ignored window, and you have a mux integration enabled, but no mux window next to the ignored window? Possibly we could just ignore it and basically do at_edge = 'stop' or at_edge = 'wrap' in this case, but how do we decide which?

mehalter commented 1 year ago

I guess I was thinking split implies that you aren't wrapping. So I would think it would just do stop

mrjones2014 commented 1 year ago

Sounds fair enough. #83

mehalter commented 1 year ago

Thanks so much!