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 additional video player keyboard shortcuts #275

Merged
merged 14 commits into from
May 23, 2016
Merged

Add additional video player keyboard shortcuts #275

merged 14 commits into from
May 23, 2016

Conversation

Lurk
Copy link
Contributor

@Lurk Lurk commented Mar 31, 2016

As described at #118

Skip forward 10 seconds ((CMD OR CTRL) ALT right)
Skip back 10 seconds ((CMD OR CTRL) ALT left)
Increase video speed ((CMD OR CTRL) +)
Decrease video speed ((CMD OR CTRL) -)

Dont tested with airplay and chromecast Video speed should be working with airplay but not with chromecast (https://github.com/mafintosh/chromecasts dont support it)

Sergey Bargamon added 2 commits March 31, 2016 18:56
 Skip back 10 seconds ((CMD OR CTRL) ALT left)
 Increase video speed ((CMD OR CTRL) +)
 Decrease video speed ((CMD OR CTRL) -)
} else if ((e.ctrlKey || e.metaKey) && e.altKey && e.which === 37) {
dispatch('playbackJump', state.playing.currentTime - 10)
} else if ((e.ctrlKey || e.metaKey) && (e.which === 107 || e.which === 171)) { /* CMD || CTRL + DOM_VK_ADD || DOM_VK_PLUS */
dispatch('setPlaybackRate', state.playing.playbackRate + 10)
Copy link
Contributor

Choose a reason for hiding this comment

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

playbackRate is the relative speed to normal so:

  • 1.0 is normal speed
  • 2.0 is double speed
  • 0.5 is half speed (slow motion :D)

and the negative ones are for playing backwards so:

  • -1.0 is backwards, normal speed
  • -0.5 is backwards, half speed
  • -2.0 is backwards, double speed

Copy link
Contributor

Choose a reason for hiding this comment

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

The 'steps' should be implemented in base2, standard players use 1x, 2x, 4x, 8x, 16x, so the function to modify the rate can be

  • increase:
    dispatch('setPlaybackRate', state.playing.playbackRate * 2)
  • decrease:
    dispatch('setPlaybackRate', state.playing.playbackRate / 2)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, but html5 video can`t play backward.

…2x, 4x, 8x, 16x

fixed bug with shift + "=" which is "+"
@feross
Copy link
Member

feross commented Apr 5, 2016

Sorry for the delay in reviewing this. We'll get to this soon, I promise.

Sergey Bargamon added 3 commits April 8, 2016 13:12
# Conflicts:
#	renderer/lib/cast.js
#	renderer/state.js
#	renderer/views/player.js
# Conflicts:
#	renderer/state.js
@@ -0,0 +1,7 @@
root = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't know if we want to add this file. There are many different editors out there, and they each have their own config file. WebTorrent Desktop is built from many npm packages, each with their own git repo.

So I don't think we want to add vim/emacs/intellij/etc configs for every editor to every repo. I recommend putting those in your home folder

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.editorconfig supported by many editors (http://editorconfig.org/) by default or with plugin. So maybe we should have it in repo?

@dcposch
Copy link
Contributor

dcposch commented Apr 13, 2016

Thanks for sending this! Left some comments

@Lurk
Copy link
Contributor Author

Lurk commented Apr 13, 2016

@dcposch Agree with all your comments but one (about .editorconfig)
I think we should discuss about it more. Or move that discussion in new issue.
My point is that .editorconfig supported by many editors/IDE, so we can define code style one time for all contributors.

More about .editorconfig you can read here http://editorconfig.org/

@grunjol
Copy link
Contributor

grunjol commented Apr 13, 2016

I tend not to push IDE config files. You can always have the local excludes using
.git/info/exclude
file or globally using
git config --global core.excludesfile ~/.gitignore_global

@Lurk
Copy link
Contributor Author

Lurk commented Apr 13, 2016

@grunjol .editorconfig not exactly IDE config file. It more like tab vs spaces thing config.

And no mater what editor you use vim/emacs/intellij/atom/sublime/etc (full list: http://editorconfig.org/#download) it works.

@grunjol
Copy link
Contributor

grunjol commented Apr 13, 2016

is it http://standardjs.com/ compatible?

Sergey Bargamon added 3 commits April 20, 2016 18:23
# Conflicts:
#	renderer/index.js
make playback rate more granular
add to menu skip and speed entries
@Lurk
Copy link
Contributor Author

Lurk commented Apr 20, 2016

All done.
I remove .editorconfig from pull request
@grunjol answer to your question - yes it is :)

Sergey Bargamon added 3 commits May 16, 2016 13:18
# Conflicts:
#	main/menu.js
#	renderer/index.js
# Conflicts:
#	main/menu.js

function increasePlaybackRate () {
if (windows.main) {
windows.main.send('dispatch', 'setPlaybackRate', 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would call this changePlaybackRate, otherwise it looks like we're setting the playback rate to 1 or -1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}
state.playing.playbackRate = rate
if (lazyLoadCast().isCasting()) {
Cast.setRate(rate)
Copy link
Contributor

Choose a reason for hiding this comment

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

See below --- when Cast.setRate returns false, let's set rate = 1.

That way, the increase/decrease playback rate won't do anything on Chromecast (instead of showing a label that says "2X" or "0.5X" while the actual playback speed doesn't change)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@dcposch
Copy link
Contributor

dcposch commented May 22, 2016

LGTM otherwise. I might merge this later today, then do a few fixes. I have access to a Chromecast, Airplay, and DLNA so I can test all the cast types.

Sergey Bargamon added 2 commits May 23, 2016 09:56
# Conflicts:
#	renderer/views/player.js
setRate return boolean depending on whether this cast target supports setting the playback rate.
if setRate returns false - don`t change state
redundant else if statement in changePlaybackRate function
@dcposch
Copy link
Contributor

dcposch commented May 23, 2016

LGTM

@dcposch dcposch merged commit deaf036 into webtorrent:master May 23, 2016
mathiasvr pushed a commit to mathiasvr/webtorrent-desktop that referenced this pull request May 26, 2016
*  Skip forward 10 seconds ((CMD OR CTRL) ALT right)
 Skip back 10 seconds ((CMD OR CTRL) ALT left)
 Increase video speed ((CMD OR CTRL) +)
 Decrease video speed ((CMD OR CTRL) -)

* Codestyle fix

* The 'steps' should be implemented in base2, standard players use 1x, 2x, 4x, 8x, 16x

fixed bug with shift + "=" which is "+"

* resolve conflicts

* remove ide specific data
make playback rate more granular
add to menu skip and speed entries

* intendation fix

* conflict resolve

* rename setPlaybackRate to changePlaybackRate
setRate return boolean depending on whether this cast target supports setting the playback rate.
if setRate returns false - don`t change state
redundant else if statement in changePlaybackRate function
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants