Closed nimishjha closed 3 weeks ago
Hi guys, as discussed, this one modifies the CursorPageUp
and CursorPageDown
actions. A couple of questions:
CursorPage[Up|Down]
actions and implement these as new actions?scrollmargin
as the scroll overlap, or should this be a new option like pageoverlap
or something?Tried it and overall it seems to perform well. There is again this small bug we had in the previous PR:
scrollmargin
to defaultCursorPageDown
The same happens in the reverse direction when you place the cursor in the last line or the line before, do a CursorPageUp
and then move the cursor just one line up or down -> the view moves 1 or 2 lines.
Looks again like a side effect of considering scrollmargin
. Do we need this here?
the view will move with the cursor 1 or 2 lines depending on the line where we started
The only way to avoid this would be to check if the cursor is within the scrollmargin
, and if so, move it to scrollmargin
lines from the top or the bottom of the view. I.e. if the user opens a file and the cursor is at { X: 0, Y: 0 }
, when he hits Page Down
the cursor is moved to { X: 0, Y: view.StartLine + scrollmargin }
. This is how nvim does it.
If we implement this, I think this logic should probably be in the Cursor.GotoLoc
function itself: unless the view is anywhere but the beginning or end of the file, never move the cursor to within scrollmargin
from the top/bottom.
This logic is already implemented in h.Relocate()
. And CursorPage{Up,Down}
already uses it. But you removed it, hence the problem.
I.e. the problem can be fixed by not removing h.Relocate()
.
- Should we keep the old
CursorPage[Up|Down]
actions and implement these as new actions?
Like I said before, I think we should change the behavior of CursorPage[Up|Down]
, since we have no evidence that their current behavior is actually preferred by anyone. I suppose the current behavior was just the easiest to implement, no more than that.
If it turns out that some users actually prefer the existing behavior and complain about the change, we can revert it and add new actions instead, at least knowing that it makes sense to do so.
- Is it ok to reuse
scrollmargin
as the scroll overlap, or should this be a new option likepageoverlap
or something?
Like I said before, I'm not sure reusing scrollmargin
is a good idea. A user may want to have scrollmargin of e.g. 5 lines when "slowly" scrolling via cursor up/down keys etc, yet may not want such a big overlap when paging up/down. Or for example, a user may not want scrollmargin at all (thus sets scrollmargin
to 0) yet still want to have some overlap when paging up/down.
I'd prefer just to hardcode the overlap to be 1 or 2 lines for now, for simplicity. Or, if we insist on making it configurable right away, add a new option.
Also what about SelectPageUp
and SelectPageDown
? We should update them as well, right?
SelectPageUp
and SelectPageDown
have been updated to match.pageoverlap
has been added with a default value of 2.Due to the usage of
h.Relocate()
we still have a dependency toscrollmargin
:So what if the user likes to keep the cursor where he placed it...like in
nano
?
But isn't that the purpose of scrollmargin
? To never let the cursor within scrollmargin
number of lines from the top or bottom edge of the view (except at the beginning and end of the file)? If the user wants the cursor to go all the way to the top or bottom of the view, he should set scrollmargin
to 0
.
But isn't that the purpose of
scrollmargin
?
Hm, arguable. Maybe we can use it for this scenario too. nano
& gnome-text-editor
don't use it at all...at least not by default, but vim
on the other hand applies something similar when using the page-up and -down keys.
So maybe I'm already convinced.
I guess we could get fancy and check the direction the user is moving the cursor, and relocate the view only if the cursor is being moved into scrollmargin. For instance, if we have a view height of 100 lines, scrollmargin of 5, and the cursor is on line 7. The user moves the cursor up with the up arrow, 7, 6, 5, and on the next keypress we relocate the view because we're moving inwards into the scrollmargin.
But if the cursor is already inside the scrollmargin, say line 3, and the user goes to line 4, we're moving outwards from inside the scrollmargin, so we don't relocate.
The downside would be that it wouldn't match other editors, could be confusing, and could give rise to interesting edge cases.
As of now I'd say this fancy idea will make the things a bit too complicated.
@dmaluka: What do you think, leave it as is?
I agree with @nimishjha that we should keep it simple and just respect scrollmargin
, i.e. the behavior of the current version of this PR makes the most sense to me. (Maybe it's unfortunate that micro sets scrollmargin
to a non-zero value by default, but that's how it is.)
BTW vim also has a scrolloff
option, pretty much the same as micro's scrollmargin
, and its behavior wrt page up/down is like in this PR.
Modifies the
CursorPageUp
andCursorPageDown
actions to behave like nano. Adds a new configuration setting,pageoverlap
. Both actions now scroll the view by (view height - pageoverlap) and keep the cursor at the same relative position in the view. Resolves #1808.