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

feature(resize): Non directional resize #520

Merged
merged 20 commits into from
Nov 5, 2021
Merged

feature(resize): Non directional resize #520

merged 20 commits into from
Nov 5, 2021

Conversation

h3nill
Copy link
Member

@h3nill h3nill commented May 18, 2021

Closes: #163

Feel free to suggest better names for enums and funtions, I am not good with names : )


For future reference, I am mentioning what the code does:

  • First of all it calculates how many panes are inside viewport.
  • If there are only two panes it just resize in whatever direction there is space.
  • If there are more then two panes it checks:
    • if there is a + cross section in any of the four corners. If yes then resize accordingly.
    • If no, try to find T cross section and resize accordingly.

@imsnif
Copy link
Member

imsnif commented May 18, 2021

@henil - is this working for you? I can't get it to work with cargo make run, then doing ctrl-r and then + or -. Am I doing something wrong?

@h3nill
Copy link
Member Author

h3nill commented May 18, 2021

@imsnif Yes. Currently it tried to increase/decrease size on two direction at the same time so it would not work when there are only two panes open (without that constraint it would be very ambiguous to find when to resize what 😅 ) . Have you tried with more than 2 panes?

@imsnif
Copy link
Member

imsnif commented May 18, 2021

Yeah, I'm just trying again. Tbh I'm not even reaching the function (put a debug there and I'm not getting to it). Still investigating

@imsnif
Copy link
Member

imsnif commented May 18, 2021

Right! I got it to work :) I think the issue was on my side because of my convoluted environment, sorry!

This is a great start and I'm happy to see you picked it up. A few things:

  1. I think you need to add a self.render() at the end of the methods, otherwise you won't see the increase/decrease immediately.
  2. Do you think we can get it to increase/decrease even if there are just 2 panes? What's the challenge you see there?
  3. When there are 4 panes, right now it resizes the panes in one direction and only then the other. I understand why you did it like that - it integrates much better with the current functions we have - but it's not very intuitive to the user. Do you think we can write a specialized function to get it to increase in both directions simultaneously if possible?

@h3nill
Copy link
Member Author

h3nill commented May 20, 2021

I think you need to add a self.render() at the end of the methods, otherwise you won't see the increase/decrease immediately.

Will do.

Do you think we can get it to increase/decrease even if there are just 2 panes? What's the challenge you see there?

That shouldn't be any problem, I could just add an extra condition that checks if 2 panes are open and do a normal resize in that case.

When there are 4 panes, right now it resizes the panes in one direction and only then the other. I understand why you did it like that - it integrates much better with the current functions we have - but it's not very intuitive to the user. Do you think we can write a specialized function to get it to increase in both directions simultaneously if possible?

I am afraid I don't understand what you mean by increase in both direction, Can we talk about this more on discord.

@imsnif
Copy link
Member

imsnif commented Jun 28, 2021

Hey @henil - great progress!

With 4 panes this works like a charm. That's exactly what I meant and it looks great. I think we need to go about this in a bit of a different way though, because if the same situation exists with more than 4 panes it still reverts to the previous behaviour.

To reproduce:

  1. Start Zellij
  2. Split down
  3. Split down again
  4. Split right
  5. Move focus up
  6. Split right
  7. Move focus down
  8. Do a non-directional pane size increase

While there are 5 panes on the screen now, we still want it to behave in the same way.
Makes sense? If you want help or suggestions about how to approach this, I'd be happy to think about it together.

@h3nill
Copy link
Member Author

h3nill commented Jun 29, 2021

Yes. That makes sense, but I will need some help about how to go about implementing it. Will reach out to you on discord.

@h3nill h3nill marked this pull request as ready for review October 2, 2021 07:38
@imsnif
Copy link
Member

imsnif commented Oct 4, 2021

@henil - this is really awesome!!

I haven't looked a the code yet, just trying it out. But I think I noticed a bug, seems like panes can now be reduced in size so that they become too small (both with ctrl-n + - and just with normal resize). To reproduce:

  1. Start Zellij, split down
  2. Reduce the size of the bottom pane (either with ctrl-n + - or with ctrl-n + j) until the edge of the screen.
  3. You'll see that it becomes too small to hold the cursor

The minimum height should be at least one line of "content" (aside from the frame). You can compare the behaviour to how it is in main.

@h3nill
Copy link
Member Author

h3nill commented Oct 9, 2021

@imsnif I am able to reproduce that bug on main as well. Also I am only using "higher level" function call so that shouldn't be a problem in this PR I think.

Copy link
Member

@imsnif imsnif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@henil - for some reason I stopped being able to reproduce it either here or on main. Oi! Might have to do with more specific circumstances. Let's leave it for now. So, on to the review:

I went over all the code and functionality, and I must say: this is very good work! One can see that you put a lot of thought and effort into this. The changes are elegant and you have a meticulous attention to details. I can very much congratulate you on that!

I left a few comments in the code - but nothing very major I think. I'm eager to get this into main and release it as soon as possible. Please let me know if anything is unclear or you'd like any help.

EDIT:
Almost forgot. Do you think you can add some unit tests for this? Preferably for each case. I think you can start with the resize tests here: https://github.com/zellij-org/zellij/blob/main/zellij-server/src/unit/tab_tests.rs#L2529

zellij-server/src/route.rs Outdated Show resolved Hide resolved
zellij-server/src/tab.rs Outdated Show resolved Hide resolved
zellij-server/src/tab.rs Outdated Show resolved Hide resolved
zellij-server/src/tab.rs Outdated Show resolved Hide resolved
zellij-server/src/tab.rs Outdated Show resolved Hide resolved
zellij-server/src/tab.rs Outdated Show resolved Hide resolved
zellij-server/src/tab.rs Outdated Show resolved Hide resolved
zellij-utils/assets/config/default.yaml Show resolved Hide resolved
zellij-utils/src/input/mod.rs Outdated Show resolved Hide resolved
@imsnif
Copy link
Member

imsnif commented Oct 25, 2021

@henil - let me know when everything is implemented and you'd like another review.

@h3nill
Copy link
Member Author

h3nill commented Nov 1, 2021

@imsnif I have implemented suggested changes and added test, can you take a look when you get time.

@imsnif
Copy link
Member

imsnif commented Nov 4, 2021

Hey @henil - I think I found a way to simplify the methods and share lots of logic between them. I made the changes (and also fixed some small cosmetic stuff in the status bar). What do you think? I'm good with merging this now if you're cool with it.

@h3nill
Copy link
Member Author

h3nill commented Nov 5, 2021

Yes. Looks good to me!

@imsnif
Copy link
Member

imsnif commented Nov 5, 2021

Awesome! Thank you very much for your hard work on this. You did a great job and I think this is an amazing feature.

@imsnif imsnif merged commit 4ac9344 into main Nov 5, 2021
@h3nill h3nill deleted the nondirectional-resize branch November 10, 2021 06:00
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.

Feature: non-directional increase/decrease pane size
4 participants