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

add actions CursorToViewTop, CursorToViewCenter, CursorToViewBottom #3506

Merged
merged 1 commit into from
Oct 23, 2024

Conversation

nimishjha
Copy link
Contributor

@nimishjha nimishjha commented Oct 16, 2024

This PR adds three new actions:

  • CursorToViewTop
  • CursorToViewCenter
  • CursorToViewBottom

Both the first and last actions apply an offset of scrollmargin to avoid the view jumping when the user hits a cursor movement key.

internal/action/actions.go Outdated Show resolved Hide resolved
internal/action/actions.go Outdated Show resolved Hide resolved
internal/action/actions.go Outdated Show resolved Hide resolved
@nimishjha nimishjha force-pushed the add-page-and-cursor-actions branch 2 times, most recently from 0dda2d6 to 2ceac22 Compare October 17, 2024 01:15
internal/action/actions.go Outdated Show resolved Hide resolved
internal/action/actions.go Outdated Show resolved Hide resolved
internal/action/actions.go Outdated Show resolved Hide resolved
internal/action/actions.go Outdated Show resolved Hide resolved
internal/action/actions.go Show resolved Hide resolved
internal/action/bufpane.go Outdated Show resolved Hide resolved
@dmaluka
Copy link
Collaborator

dmaluka commented Oct 17, 2024

Please squash the commits into one, there is no point in having a broken commit immediately followed by a commit that fixes it.

internal/action/actions.go Outdated Show resolved Hide resolved
internal/action/actions.go Outdated Show resolved Hide resolved
@nimishjha nimishjha force-pushed the add-page-and-cursor-actions branch 3 times, most recently from b755e9d to a5754fd Compare October 20, 2024 02:43
@dmaluka dmaluka requested a review from JoeKar October 20, 2024 09:28
internal/action/actions.go Outdated Show resolved Hide resolved
internal/action/actions.go Outdated Show resolved Hide resolved
internal/action/actions.go Outdated Show resolved Hide resolved
internal/action/actions.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@JoeKar JoeKar left a comment

Choose a reason for hiding this comment

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

Currently it's hard to identify the difference from the usual Page{Up|Down} to PageUp,CursorToViewTop resp. PageDown,CursorToViewBottom.
Do I something wrong?

Would be nice to have a CursorKeep for chaining to provide the possibility to align the behavior to many other editors.

internal/action/actions.go Show resolved Hide resolved
@nimishjha
Copy link
Contributor Author

Currently it's hard to identify the difference from the usual Page{Up|Down} to PageUp,CursorToViewTop resp. PageDown,CursorToViewBottom. Do I something wrong?

Well, PageUp and PageDown just move the view, leaving the cursor where it is. So you could have a cursor, multi-cursor, selection or multi-selection, you could PageDown a number of times, then PageUp back to where you where and your cursors/selections would be untouched.

Page[Up|Down],CursorToView[Top|Center|Bottom] give you the option to have the cursor in a predictable place when you page up or down. The CursorToView[Top|Center|Bottom] actions are useful by themelves as well, especially when you have a view that's over 100 lines tall and would like to move the cursor without holding down the arrrow keys.

Would be nice to have a CursorKeep for chaining to provide the possibility to align the behavior to many other editors.

Don't worry, that's coming up in the next PR. Instead of CursorKeep we're going to either modify the CursorPage[Up|Down] actions or create new ones that keep the cursor at the same relative location in the view while paging up/down.

@JoeKar
Copy link
Collaborator

JoeKar commented Oct 22, 2024

Well, PageUp and PageDown just move the view, leaving the cursor where it is.

Well, yes...but more precise it changes the view to scrollmargin (used with default value on my end) and from there moves the lines. When I combine/chain them with CursorToViewTop resp. CursorToViewBottom I had the same effect. Except I did something wrong.
When I combined the default bindings with CursorToViewBottomCursorToViewCenter then the cursor was indeed kept in the middle and the views scrolled, but just till I reached 50% of top or end. A further PageUp/-Down key didn't moved it to the top or end.
So maybe it is just me having again the totally wrong expectation.

@nimishjha
Copy link
Contributor Author

When I combined the default bindings with CursorToViewBottom then the cursor was indeed kept in the middle and the views scrolled, but just till I reached 50% of top or end. A further PageUp/-Down key didn't moved it to the top or end. So maybe it is just me having again the totally wrong expectation.

I think you mean CursorToViewCenter. Anyway, your desired Page Up/Down behavior will be implemented in my next PR. (I already have the code ready to go).

@JoeKar
Copy link
Collaborator

JoeKar commented Oct 22, 2024

I think you mean CursorToViewCenter.

Yes.

Anyway, your desired Page Up/Down behavior will be implemented in my next PR. (I already have the code ready to go).

Are we still talking about CursorToViewCenter? In case of yes, why not here? We try to add a action, which doesn't fit in its behavior to CursorToViewTop & CursorToViewBottom (reaching first and last line via consecutive PageUp/PageDown keys).

Since I'm still puzzled about CursorToViewTop & CursorToViewBottom I trust you guys that they have their purpose.

@nimishjha
Copy link
Contributor Author

Are we still talking about CursorToViewCenter? In case of yes, why not here? We try to add a action, which doesn't fit in its behavior to CursorToViewTop & CursorToViewBottom (reaching first and last line via consecutive PageUp/PageDown keys).

Yeah, I think I haven't been able to make myself clear so far. The purpose of CursorToView[Top|Center|Bottom] is not to reach lines via PageUp/Down keys. The CursorTo... actions are completely independent of Page[Up|Down]. Their only purpose is to move the cursor to the top or bottom (accounting for scrollmargin) or the center of the view. In other words, their primary purpose is not to be chained with Page[Up|Down], but to be used as independent actions with their own key bindings. For instance, I have Alt-[q|w|e] bound to CursorToViewTop, CursorToViewCenter, and CursorToViewBottom.

You can certainly chain them with Page[Up|Down] or any other action if you wish, but their purpose is not to implement the correct Page up/down behavior by chaining them with the Page[Up|Down] actions.

To recap: this PR is solely to implement the CursorToView... actions. The next PR will implement the page up/down behavior you want as single actions, without any need for chaining anything.

@dmaluka
Copy link
Collaborator

dmaluka commented Oct 22, 2024

@JoeKar I also don't quite get your point. CursorToViewCenter moves the cursor to the view center, simple as that. CursorToView{Top,Center,Down} actions are independent of Page{Up,Down} (although of course can be chained with them, just like with any other actions.)

Also to remind just in case: Page{Up,Down} should not be confused with CursorPage{Up,Down}.

...If we change the behavior of CursorPage{Up,Down} to be nano-like, perhaps CursorToViewCenter,CursorPage{Up,Down} will do what you want? I.e.: CursorToViewCenter will move the cursor to the center if it isn't already there, and then CursorPage{Up,Down} will either move the cursor one page up/down if we are not on the first/last page, or move the cursor to the first/last line if we are already on the first/last page.

@nimishjha nimishjha changed the title add actions MoveCursorToView, PageDownAndMoveCursor, PageUpAndMoveCursor add actions CursorToViewTop, CursorToViewCenter, CursorToViewBottom Oct 22, 2024
@JoeKar
Copy link
Collaborator

JoeKar commented Oct 23, 2024

The purpose of CursorToView[Top|Center|Bottom] is not to reach lines via PageUp/Down keys. The CursorTo... actions are completely independent of Page[Up|Down].

@JoeKar I also don't quite get your point. CursorToViewCenter moves the cursor to the view center, simple as that.

Ok guys, excuse me. I was too stubborn and fixed/focus on this other "use case" (most probably it isn't even one in that combination) and totally ignored the stand-alone purpose. So let us bring this to an end here.

@JoeKar JoeKar merged commit b3227d6 into zyedidia:master Oct 23, 2024
6 checks passed
@nimishjha nimishjha deleted the add-page-and-cursor-actions branch October 23, 2024 06:20
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