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

Simplify edge calculation when scrolling down #1194

Merged
merged 4 commits into from
Apr 9, 2023
Merged

Simplify edge calculation when scrolling down #1194

merged 4 commits into from
Apr 9, 2023

Conversation

joelim-work
Copy link
Collaborator

When scrolling with a very large scrolloff value, the cursor will be positioned in the middle of the column, and the edge variable represents how far the cursor is from the top/bottom row. If the height is an odd number, then this is straightforward, but if the height is an even number, then there are two candidate rows for the middle (lf uses the bottom one).

For example:

  • If the height is 9, then the cursor should be located at index 4, and edge is simply 4 on both sides
  • If the height is 10, then the cursor should be located at index 5, and edge is 5 when scrolling up, but 4 when scrolling down

As part of the calculations, the current code effectively tries to determine min(nav.height/2, nav.height/2+nav.height%2-1) which can be simplified (h to represent the height for the following):

  • h % 2 - 1 is always equal to 0 or -1, which means h / 2 can never be smaller than h / 2 + h % 2 - 1, and therefore the calculation can be collapsed to the latter.
  • h / 2 + h % 2 - 1 means to halve and subtract 1 if the height is an even number, and integer divide by 2 if it is an odd number, so the calculation can further be simplified to (h - 1) / 2.

@gokcehan
Copy link
Owner

gokcehan commented Apr 8, 2023

@joelim-work Thanks for working out the math for this. It has been some time but I think the likely reason the calculation is written in two steps is that the comment itself only really belongs to the second step. So the first part is meant to be the rough calculation that one should be able to understand without bashing their head, and the second part is the hairy part to handle the exceptional case. Unless you do something with the comment as well, I worry this change could make things a little more difficult to understand. Is there a reason why you are working on this part of the code?

@joelim-work
Copy link
Collaborator Author

@gokcehan I was reading through this part of the code to understand all the navigation stuff while implementing #1196 and was wondering why nav.height was used multiple times to calculate the value of edge. I couldn't help but think that there would be a simpler way to express this logic.

I thought about updating the comment like below to explain the issue more clearly with an example, but I ended up deciding it was too verbose:

	// If scrolloff is set to a large value, then the cursor should be placed in
	// the middle. If nav.height is an even number, then there are actually two
	// candidate rows for being the middle. To ensure a smooth scrolling
	// experience where the cursor doesn't move when scrolling both up and down,
	// the bottom candidate row is used in both cases, however this means that
	// the half value has to be adjusted. For example, if the height is 10, then
	// the cursor should be positioned at index 5, which is 5 rows away from the
	// top row (relevant when scrolling up), but only 4 rows away from the
	// bottom row (relevant when scrolling down).

Anyway I changed the code to be more explicit about what it's trying to do and tidied up the comment a bit. Hopefully this is easier for others to understand, but if you prefer the code as it originally was, let me know and I will just simply close this PR.

@gokcehan
Copy link
Owner

gokcehan commented Apr 9, 2023

@joelim-work Looks good, thanks.

@gokcehan gokcehan merged commit 734f11d into gokcehan:master Apr 9, 2023
@joelim-work joelim-work deleted the simplify-edge-calc branch April 10, 2023 00:30
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