8000 Add vim-like page scrolling by BlakeJC94 · Pull Request #1072 · hrkfdn/ncspot · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add vim-like page scrolling #1072

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 20 commits into from
Mar 12, 2023
Merged

Add vim-like page scrolling #1072

merged 20 commits into from
Mar 12, 2023

Conversation

BlakeJC94
Copy link
Contributor
@BlakeJC94 BlakeJC94 commented Mar 4, 2023 •
edited
Loading

First pass at implementing page scrolling for list views and help views with <C-d>/<C-u> to scroll down/up by half a page and <C-f>/<C-b> to scroll down/up by a full page.

Addresses the first part of issue #959.

I've confirmed this build works with this in my .config/ncspot/config.toml:

[keybindings]
"Ctrl+f" = "move pagedown 1.0"
"Ctrl+b" = "move pageup 1.0"
"Ctrl+d" = "move pagedown 0.5"
"Ctrl+u" = "move pageup 0.5"

I've been using NCSpot for a while, and this is a feature I personally wanted as well. I was inspired to learn enough Rust so that I could make this contribution! Very open to feedback, let me know if adding this feature to one of the keybinds tables in the README.md would be appreciated.

BlakeJC94
BlakeJC94 commented Mar 4, 2023
View reviewed changes
@ThomasFrans
Copy link
Contributor

Ooh, I would love this feature as well! There is one thing that I'm a bit worried about when adding this, and it's something I also mentioned in my feature request. In Vim, you don't really delete things by accident by using d, and if you do, you can undo that. Here, if your ctrl key isn't working well (like mine is on my laptop atm), pressing ctrl+d several times to find a bunch of tracks gone is slightly worrying...

@BlakeJC94
Copy link
Contributor Author
BlakeJC94 commented Mar 4, 2023 •
edited
Loading

Dang, the rest of the feature request makes more sense now!

A suggestion: what if the default key to delete were changed from d to D? It reduces the risk overall for accidental changes to a playlist and addresses this specific concern (and it's still a valid vim key for that action)

EDIT: I just saw that D is already mapped to "remove currently playing item from library", perhaps mapping d/D to x/X could help avoid these mishaps?

@ThomasFrans
Copy link
Contributor

I know I suggested changing the key in the feature request as well but in hindsight, I'm not a big fan of that. I think programs should keep working the same for long time users of the software. Of course if hrkfdn thinks it's best to change the keymap, this issue of d and ctrl+d could be easily solved, but personally I'm not a big fan of changing the behavior, just like some other people that commented/reacted on the feature request. There are other ways to solve this problem, like a configuration option, or providing the extra vim commands as optional by forcing users that want to use them to map them themselves.

@ThomasFrans
Copy link
Contributor

Also quickly wanted to say you forgot to format I think 😉 The workflow would catch this anyway but thought I'd quickly let you know. In Rust, you can use cargo fmt to format all the code automatically, and cargo clippy to test for common errors. These are both tested by the CI. (I couldn't wait to see this PR in action so I cloned it, that's how I noticed 😝 )

@BlakeJC94
Copy link
Contributor Author

I know I suggested changing the key in the feature request as well but in hindsight, I'm not a big fan of that. I think programs should keep working the same for long time users of the software. Of course if hrkfdn thinks it's best to change the keymap, this issue of d and ctrl+d could be easily solved, but personally I'm not a big fan of changing the behavior, just like some other people that commented/reacted on the feature request. There are other ways to solve this problem, like a configuration option, or providing the extra vim commands as optional by forcing users that want to use them to map them themselves.

True, yeah that sounds like a diplomatic solution. If @hrkfdn would prefer to change the map in a future major release, the groundwork and some good suggestions are here. When I get a bit of time, I'll remove the default keymaps this adds and propose an addition to the readme to make this feature a bit more discoverable 👍

Also quickly wanted to say you forgot to format I think 😉

Haha yep! I had forgotten to enable autoformatting on write for rs files. I didn't know about cargo clippy though, thanks for the tip!

@BlakeJC94
Copy link
Contributor Author
BlakeJC94 commented Mar 4, 2023 •
edited
Loading

Alrighty, new default keybinds removed and added move variants to the parser. I'll think about the readme change for a bit more and update this soon

@BlakeJC94 BlakeJC94 marked this pull request as draft March 5, 2023 21:25
@BlakeJC94
Copy link
Contributor Author
BlakeJC94 commented Mar 5, 2023 •
edited
Loading

When updating the readme, I thought of a slightly better interface for these commands, I think I'll try implement a MoveAmount::Float variant instead that accepts a float, modelled on MoveAmount::Integer. This seems a bit cleaner than using "halfpage" and "page" literals in the command interface. I'll try it out and see what it's like to use

@BlakeJC94 BlakeJC94 marked this pull request as ready for review March 7, 2023 06:53
@hrkfdn
Copy link
Owner
hrkfdn commented Mar 12, 2023

Hey there, sorry for the delay. Thanks for the PR and the valuable discussion. FYI: I agree w/ both of you: while the existing binding isn't perfect we probably shouldn't break it.

Merging the PR in a second :)

@hrkfdn hrkfdn merged commit 1a0258f into hrkfdn:main Mar 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@BlakeJC94 @ThomasFrans @hrkfdn
0