kimtore / pms

Practical Music Search is an interactive Vim-like console client for the Music Player Daemon.
https://ambientsound.github.io/pms/
MIT License
249 stars 23 forks source link

Make scroll behaviour more similar to vim #96

Closed tremby closed 7 years ago

tremby commented 7 years ago

Closes https://github.com/ambientsound/pms/issues/94

Behaviour is not identical to vim, but is much closer.

Vim has weird edge cases. Examples with scrolloff=0, a buffer with many lines, and a window 10 lines tall:

I haven't tried to recreate all of this, since I'm not convinced it's actually intentional behaviour.

The behaviour I've ended up with, instead, is this:

I've moved the page up/down and half page up/down commands to the viewport namespace, since I think they make more sense here.

codecov-io commented 7 years ago

Codecov Report

Merging #96 into master will decrease coverage by 0.36%. The diff coverage is 32.43%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #96      +/-   ##
==========================================
- Coverage   54.01%   53.65%   -0.37%     
==========================================
  Files          51       51              
  Lines        1779     1791      +12     
==========================================
  Hits          961      961              
- Misses        796      807      +11     
- Partials       22       23       +1
Impacted Files Coverage Δ
commands/cursor.go 62.5% <ø> (+1.38%) :arrow_up:
options/defaults.go 0% <ø> (ø) :arrow_up:
commands/viewport.go 56.25% <32.43%> (-23.75%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c4e1a15...4917b63. Read the comment docs.

tremby commented 7 years ago

Will do, and then will rebase the other branch.

kimtore commented 7 years ago

Thanks. There is also a bug in center mode: when using page up near the top, or page down near the bottom, the cursor is not moved to the start or end of the list.

tremby commented 7 years ago

That's not a bug in my opinion! That's how it works in vim.

kimtore commented 7 years ago

My Vim only "blocks" on page up. It scrolls down to the very bottom, marking the last line and leaving only that line visible. So this behavior is dubious. I see your point on separating viewport and cursor, but in my opinion it will be more user-friendly to have the cursor move to the edge at the same time.

tremby commented 7 years ago

That behaviour at the bottom is because vim lets you keep scrolling at the bottom, showing you ~ rows. PMS doesn't do that (though it could...)

I don't agree regarding page up moving the cursor to the top. Page up moves the viewport a page up, and it's already at the top, so it's right that it shouldn't do anything. There are plenty of other ways to get the cursor to the top: gg, H, ^U. I'm back to banging the vim drum of course, but I think staying close to what vim does is best here.

Would you be on the same page as me if we implemented the ~ rows at the bottom, resulting in ^F getting you eventually to the very last song? Vim also rings the bell if you try to ^B when already at the top. We could do that too?

kimtore commented 7 years ago

With this change, PgUp will move the viewport instead of the cursor. Semantically, it is correct not to move the cursor to the top. That said, having to switch from PgUp to gg requires moving the fingers (context switching), so the correctness might come at the expense of user-friendlyness.

I don't think implementing ~ lines is useful, as the primitive in PMS is the song and not the character.

Given as Vim-like is a key selling point for users, it's important to make PMS look and feel right for Vim users. Thus, I'm going to accept your pull request. If the cursor functionality becomes important enough, it can be re-implemented as cursor [...] commands.

tremby commented 6 years ago

That said, having to switch from PgUp to gg requires moving the fingers (context switching)

Not if using ^F and ^B, which I would have thought most vim users would rather than pgup/pgdn keys. If using pgup/pgdn, there are also the home and end keys anyway.

I don't think implementing ~ lines is useful, as the primitive in PMS is the song and not the character.

I'm not sure I fully understand your line of thinking here. Could you elaborate?

If the cursor functionality becomes important enough, it can be re-implemented as cursor [...] commands.

Yes, no reason we couldn't add :cursor pageup et al back in, with their old behaviour, but I just question how useful they'd be. The weirdness of only scrolling a single row in some cases puts me off of it.

Thanks for merging!

kimtore commented 6 years ago

I don't think implementing ~ lines is useful, as the primitive in PMS is the song and not the character.

I'm not sure I fully understand your line of thinking here. Could you elaborate?

What I mean is that ~ markers are useful in Vim because one can scroll further down than the file, enter input mode, and start adding text. This mode of playlist manipulation wouldn't really be practical in PMS.

Thanks for merging!

Thanks for the pull request!

tremby commented 6 years ago

Oh, I see now. So you mean scroll down so lots of ~ are in view and then add many lines of text at once without anything scrolling.