-
Notifications
You must be signed in to change notification settings - Fork 48
Added boundaries for scrolling #43
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution!
I started a review but posted before I'd finished, so I'm doing it again properly. (In case you were confused about my disappearing comment.)
This is probably the kind of feature that a user should be able to toggle. xi-win doesn't have preferences right now, so your change might have to wait.
However, I have no idea how Raph feels about this, and it might very well be accepted by him.
(self.line_cache.height().saturating_sub(1)) as f32) - self.size.1 + LINE_SPACE; | ||
|
||
// Not allowing to scroll if the text is smaller than the window size | ||
if (LINE_SPACE * self.line_cache.height().saturating_sub(1) as f32) < self.size.1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: You're performing this calculation and typecast twice - might want to store it in a variable instead?
e.g. let visible_lines = LINE_SPACE * self.line_cache.height().saturating_sub(1) as f32
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Storing the calculation in a variable would be a better idea of course !
|
||
// Not allowing to scroll if the text is smaller than the window size | ||
if (LINE_SPACE * self.line_cache.height().saturating_sub(1) as f32) < self.size.1 { | ||
max_scroll = 0 as f32; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to lock the last line at the bottom of the screen if you have something larger than your screen. Was that the behavior you wanted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it was the behavior wanted. Maybe, it should have some extra lines at the end, but I think that this is pretty common behavior for most text editors.
For what it's worth I prefer this behavior, but it seems to be a preference, with the existing behavior default on Windows on Sublime Text. There's no preference infrastructure in place yet. What I'd like is a bool that's set to a const value (Sublime calls it The current behavior of showing a blank screen is going to be mitigated when there are line numbers. Thanks for the contribution! |
This is a really small contribution because I am beginning to use xi-win but I decided to change the boundaries for scrolling, not allowing to scroll when the text is smaller than the window size. It seems more normal to me this way.