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: "spacers" #12

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: "spacers" #12

wants to merge 3 commits into from

Conversation

l-kershaw
Copy link
Contributor

This PR adds a new type of layout_element, a "spacer", which (if there is additional space on screen once everything else is drawn) fills the remaining space.

Multiple spacers can be used at once to give a "vertically centered" layout (one spacer at the top, one at the bottom).
You can also put a spacer in the middle to have a top aligned block and a bottom aligned block.

The spacers are adjustable by changing their val, which changes the ratio of their size in relation to the other spacers.


I think I have managed to get it to work with everything else (e.g. cursor_jumps) properly but would be good to have other people testing.

- can be used to give centering by adding spacers at top and bottom

- can be used to give top and bottom aligned sections with spacer in the middle
@goolord
Copy link
Owner

goolord commented Sep 9, 2021

why does this have to maintain state and move around cursor jumps? if i'm understanding the purpose of this widget correctly couldn't you just do arithmetic to calculate a percentage of the window height and then call layout_element.padding?

@goolord
Copy link
Owner

goolord commented Sep 9, 2021

The spacers are adjustable by changing their val, which changes the ratio of their size in relation to the other spacers.

ah, this is why. 🤔

@l-kershaw
Copy link
Contributor Author

The reason is that you need to do this after all of the other layout_elements have been rendered, so that you know how much space is left.

I suppose you could combine this into the padding element with a ratio key or something, but you would still need to store state somewhere to do things at the end.

@l-kershaw
Copy link
Contributor Author

I'm happy to change methodology if you would prefer a different setup (e.g. I wasn't 100% happy with the naming "spacer" but couldn't think of anything better).

@goolord
Copy link
Owner

goolord commented Sep 9, 2021

well, as far as vim goes drawing blank lines at the end of an unmodifiable buffer is pretty pointless, but if you used a spacer in the middle then yeah you would need to go back and adjust the space drawn at the beginning

@l-kershaw
Copy link
Contributor Author

Yeh, I thought about the blank lines at the end being pointless, but I also thought that doing an extra check in the loop would make the implementation more confusing, and since it was doing no harm, I left it as is.

@goolord
Copy link
Owner

goolord commented Sep 9, 2021

can we iterate over the layout before layout_element is called and stick any of the arithmetic needed in the state? should allow us to not have to move around cursor jumps

@goolord
Copy link
Owner

goolord commented Sep 9, 2021

keeping rendering happening from top to bottom sequentially just makes the state space of the program easier to think about

@l-kershaw
Copy link
Contributor Author

I agree that rendering from top to bottom makes things easier to think about 👍

We would just need to be careful about calculating sizes of elements before we render them.
For example, if a user provides a function for a text element, should we evaluate the function in the sizing part or the rendering part?

It's probably doable, but would just need some careful thought.

- only calculates cursor jumps based on final rendered position

- also reorganised `text` type resolving
@l-kershaw
Copy link
Contributor Author

@goolord
I've refactored stuff so that we only render in one pass, and we don't mess about with the cursor_jumps once they are calculated.
I need to sort out the merge conflicts, but this PR should now have the same functionality as before, with an easier to follow state space.

Any thoughts?

@goolord
Copy link
Owner

goolord commented Sep 15, 2021

seems better, i will give this a closer look when i have time

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.

2 participants