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

Neighboring window changes #2840

Merged
merged 3 commits into from Jul 12, 2020

Conversation

juho-p
Copy link
Contributor

@juho-p juho-p commented Jul 9, 2020

This PR changes the way how neighbors are handled, mainly for the "splits" layout. In current master, the neighbors are little unintuitive, for me at least. For example, in tmux, they seem more useful, so these changes make them work more like in tmux.

For example, consider splits where initial layout is split with v-split and both resulting windows are split with h-split

 V1H1 | V2H1
 ---- | ----
 V1H2 | V2H2

Currently, when activating right neighbor from V1H2, the activated window is V2H1. When activating left from V2H2, then V1H1 is activated. Those are not really neighbors, at least with tmux or vim or bspwm, or i3 windowing they wouldn't be, but kitty considers them as such. The first commit in the PR changes this to work so that right from V1H2 would be V2H2 and so on.

Second issue is that activating and moving neighbor always works on the first one only. In tmux (and also in tiling WM like bspwm), what happens with multiple neighboring windows, is that the most recently activated window is affected, not the "first one on the list". At least for me this makes sense. This change is done in the second commit of this PR. To clarify this with an example, consider following layout: (so should we activate V1H1 or V1H2 when activating left from V2 and so on):

 V1H1 |
 ---- | V2
 V1H2 |

Lastly, I understand if this is something you don't want to merge as it is, if you either have some other plans for the splits layout or the solution I present here is not something you want for any other reason. I probably should have created an issue first, but creating PR seemed much more interesting. Also, if you think that the idea is good but requires some changes, then I probably have time to make changes to this PR in the next few days and/or weeks.

PS. I tried to create a kitten to fix this, but I think this should be in Kitty itself and also my kitten have to contain some hairy code, at least for the moving windows part: https://github.com/juho-p/improved-neighbor-kitten

Let me know what you think about this.

@kovidgoyal
Copy link
Owner

Certainly worth looking at. But if you are doing this, you should do it for the grid and tall/fat layouts as well, as behavior should be more or less consistent across layouts.

@juho-p
Copy link
Contributor Author

juho-p commented Jul 11, 2020

As far as I understand, tall and fat layouts alrady work consistently with this PR. Grid layout however does no, at least regarding the second issue that I described in the PR description. I'll see if I can fix that.

@juho-p juho-p force-pushed the neighboring_window_changes branch from ddadaef to 335aa4c Compare July 11, 2020 20:49
This affects both activating and moving neighbor windows.
Grid layout can have multiple neighbors in one side when either current
column or neighboring column is special.
@juho-p juho-p force-pushed the neighboring_window_changes branch from 335aa4c to ea30c84 Compare July 11, 2020 20:52
@juho-p
Copy link
Contributor Author

juho-p commented Jul 11, 2020

I cleaned up those two previous commits a bit and changed the grid layout neighbors behavior to work well with this. I played around with layouts and now they all seem to work well. Hopefully I didn't miss anything important... Does it look good to you now?

@kovidgoyal
Copy link
Owner

I'll take a look when I have a moment.

@kovidgoyal kovidgoyal merged commit 294be2a into kovidgoyal:master Jul 12, 2020
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.

None yet

2 participants