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 h/l as keys for rewind/forward current song #8

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@javier-lopez
Copy link

javier-lopez commented Jan 7, 2019

When discovering new music it's often useful to fast forward / rewind the current song, this commit adds such hotkeys h/l, so koel emulates vim movement j/k/h/l, probably it could be even better if koel could read from a configuration variable such keys so anyone can specify its favourite shotcuts, however I'm not really familiar with vau therefore I'm leaving such exercise for the future.

Also, while testing this functionality I found koel setups a canplay event listener which fires in playNext, playPrev, and in seek / fastforwarding operations, as Koel limits its API to 60 request per minute I found soon than leaving such event generator quickly freeze the interface, my temporal solution was to disable throttle, I'm still looking for a more general solution.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 7, 2019

Codecov Report

Merging #8 into master will decrease coverage by 0.05%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #8      +/-   ##
==========================================
- Coverage   47.59%   47.54%   -0.06%     
==========================================
  Files          90       90              
  Lines        1767     1769       +2     
  Branches      129      129              
==========================================
  Hits          841      841              
- Misses        890      892       +2     
  Partials       36       36
Impacted Files Coverage Δ
js/services/playback.js 2.22% <0%> (-0.04%) ⬇️

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 8691e2b...3d04ef0. Read the comment docs.

@phanan
Copy link
Member

phanan left a comment

Thanks for the PR. Please see my comments inline.

@@ -3,8 +3,10 @@
@keydown.space="togglePlayback"
@keydown.j = "playNext"
@keydown.k = "playPrev"
@keydown.f = "search"
@keydown.l = "toggleLike"
@keydown.h = "playRewind"

This comment has been minimized.

@phanan

phanan Jan 8, 2019

Member

I'd suggest "rewind" and "forward" as method names (this applies to playback service as well).

@keydown.f = "search"
@keydown.l = "toggleLike"
@keydown.h = "playRewind"
@keydown.l = "playForward"

This comment has been minimized.

@phanan

phanan Jan 8, 2019

Member

Not sure it's OK to remove the functionality of toggleLike completely here. Maybe use another key for the feature?

return true
}
playback.playRewind(10)

This comment has been minimized.

@phanan

phanan Jan 8, 2019

Member

Please see above re: method name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment