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

accept count for goto_window #1033

Merged
merged 8 commits into from
Nov 29, 2021
Merged

accept count for goto_window #1033

merged 8 commits into from
Nov 29, 2021

Conversation

QiBaobin
Copy link
Contributor

@QiBaobin QiBaobin commented Nov 9, 2021

also fix view is not fulfilled issue.

9gt would go to the 9th line from window top;
3gb would go to the 3th line from window bottom;

Comment on lines 593 to 594
let scrolloff =
count.unwrap_or_else(|| cx.editor.config.scrolloff.min(height.saturating_sub(1) / 2));
Copy link
Contributor

@pickfire pickfire Nov 14, 2021

Choose a reason for hiding this comment

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

I think it would better to follow the vim behavior here of having count starting from scrolloff point, otherwise 1gt would result in moving the screen by scrolloff.

But vim behavior here is good I think. If the screen is at the corner of the screen (0 or file end), 1H will go to the first line without scrolloff (same for the end), and if the screen is being scrolled to the middle, 1H will go to the first scrolloff position rather than the top. This is the nicer behavior but I think it is a bit more complicated to implement.

And with large scrolloff count it doesn't move the screen, so <n>H in vim won't change the screen in any bits, it will move to the last scrolloff point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, please take a another look

QiBaobin and others added 4 commits November 15, 2021 09:03
Co-authored-by: Ivan Tham <pickfire@riseup.net>
Co-authored-by: Ivan Tham <pickfire@riseup.net>
Align::Bottom => last_line.saturating_sub(scrolloff),
Align::Top => (view.offset.row + scrolloff + count),
Align::Center => (view.offset.row + ((last_line - view.offset.row) / 2)),
Align::Bottom => last_line.saturating_sub((scrolloff + count).min(last_line)),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Align::Bottom => last_line.saturating_sub((scrolloff + count).min(last_line)),
Align::Bottom => last_line.saturating_sub((scrolloff + count)),

saturating_sub should already protect against overflow here.

}
.min(last_line.saturating_sub(scrolloff));
.min((view.offset.row + height - 1).saturating_sub(scrolloff))
Copy link
Member

Choose a reason for hiding this comment

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

What was wrong with the previous bound? I don't think this is correct, since view.offset + height - 1 can be past last_line of the document

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I already used last_line in the before computation , I think we need ensure we won't go beyond the scroll off line of the bottom. For example if the content is only half page, we subtract last_line with scroll off is pointless because it's far from scrolling.

@archseer archseer merged commit 42fde95 into helix-editor:master Nov 29, 2021
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.

3 participants