micro (original) (raw)

Modifies the CursorPageUp and CursorPageDown 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.

Hi guys, as discussed, this one modifies the CursorPageUp and CursorPageDown actions. A couple of questions:

Should we keep the old CursorPage[Up|Down] actions and implement these as new actions?
Is it ok to reuse 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:

set scrollmargin to default
move the cursor in the first or second line
perform CursorPageDown
move the cursor with the cursor keys one line up or down
-> the view will move with the cursor 1 or 2 lines depending on the line where we started

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 like pageoverlap 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.
A new option pageoverlap has been added with a default value of 2.

@JoeKar JoeKar left a comment • Loading

Choose a reason for hiding this comment

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

Due to the usage of h.Relocate() we still have a dependency to scrollmargin:

place the cursor in the first line
perform CursorPageDown()
-> The cursor will be placed to scrollmargin and doesn't stay at the first line...and vice versa with CursorPageUp()

So what if the user likes to keep the cursor where he placed it...like in nano?

Should we extract the content of Relocate() into a separate function to control this behavior?

e.g.:

func (w *BufWindow) Relocate() bool {
    return w.RelocateWithMargin(int(b.Settings["scrollmargin"].(float64))
}

func (w *BufWindow) RelocateWithMargin(scrollmargin int) bool {
    [...]
}

func (h *BufPane) CursorPageDown() bool {
    [...]
    h.RelocateWithMargin(0)
    [...]
}

Due to the usage of h.Relocate() we still have a dependency to scrollmargin:

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.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})