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

Implement viewport scrolling commands #93

Closed tremby closed 7 years ago

tremby commented 7 years ago

This is for #65.

I've become stuck. @ambientsound, any chance you can take a look and help me figure out why this isn't working?

codecov-io commented 7 years ago

Codecov Report

Merging #93 into master will increase coverage by 0.44%. The diff coverage is 80%.

@@            Coverage Diff             @@
##           master      #93      +/-   ##
==========================================
+ Coverage   53.57%   54.01%   +0.44%     
==========================================
  Files          50       51       +1     
  Lines        1749     1779      +30     
==========================================
+ Hits          937      961      +24     
- Misses        790      796       +6     
  Partials       22       22

Impacted Files	Coverage Δ
commands/command.go	`65.45% <ø> (ø)`	:arrow_up:
options/defaults.go	`0% <ø> (ø)`	:arrow_up:
commands/viewport.go	`80% <80%> (ø)`

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 fefe1b0...97701dc. Read the comment docs.

kimtore commented 7 years ago

The problem here is that the views.ViewPort object is passed by value, and so the connection to the original object is lost. By returning a pointer in GetViewport, this problem is solved.

However, I would strongly prefer that the viewport object is kept private, and that a Scroll method (or similar) is exposed instead. Please see the upstream feature/viewport-commands.

tremby commented 7 years ago

Nice! OK, I'll finish this off soon.

If you have a moment, can you show me how I would have fixed my own code?

(I understand that it's better to keep it private instead, I just want to explicitly see what I was doing wrong.)

tremby commented 7 years ago

Ready for review!

Thanks for the help.

tremby commented 7 years ago

I believe I have this working correctly both with center on and off, by the way. It works just like vim (where the equivalent of center is scrolloff=999) in all cases I've tried other than at the very bottom (vim lets you keep scrolling and gives you ~ lines).