track02 / Youtube-Player

Firefox extension for youtube video control
The Unlicense
0 stars 1 forks source link

[Feat Request] Hotkeys? #16

Closed Geobert closed 6 years ago

Geobert commented 6 years ago

Hi, thank you for this handy extension!

Is there any plan to add hotkeys? (I don't know if the webextension API allows or not)

Regards,

track02 commented 6 years ago

Hi Geobert,

I'd be happy to investigate hotkey functionality (it's not something I'd thought of) - I was planning on tidying up the extension a little this month so I'll see what I can do.

Just to clarify - Are you thinking of being able to control the extension without opening it? E.g. (Alt + Shift + >) would to skip to next video.

Geobert commented 6 years ago

Correct :) especially for play/pause, on current selected YT player from the dropbox. I'm often using YT as a jukebox so it would be extra handy :D

track02 commented 6 years ago

I'll see if I can get something working over the weekend - Web Extensions can support hotkeys and they don't look too difficult to implement.

Do you have any preferences for key combinations? I was considering the following:

Geobert commented 6 years ago

for defaults I think it's fine, I don't know if it's hard to make them customizable :) (mayconflict with Ffx hotkeys)

track02 commented 6 years ago

I was able to get hotkeys working for the commands listed above. There was some conflict with Firefox's hotkeys though so some different key bindings will be needed.

I'll experiment a little further and look to push an update to the store this week. But until then feel free to clone the repo and try them out; you can alter the bindings by editing the manifest.json (To run in Firefox - Go to Add-Ons -> Debug Add-Ons -> Load from File -> Select Manifest)

Geobert commented 6 years ago

Thank you for your work!

I've tested it, and for some reason, only Ctrl-Shift-Space is working.

The hotkeys with arrows do not work for me. What can I do to provide you some data to help the debugging? (I've never done Ffx Extension)

track02 commented 6 years ago

Apologies - I forgot to mention that the arrow keys won't work (looks like Firefox uses them already along with a few others I tried).

Ctrl + Shift + 1 / 2 / 3 / 4 is what I ended up with for testing.

If you open the manifest.json (notepad will work) and scroll to the bottom you can view and modify the hotkeys, just save and reload the extension in firefox. (E.g. "default": "Ctrl+Shift+4" could be changed to "default": "Ctrl+Shift+P")

For debugging there is a console that will display errors/messages so if you come across anything you could check there for a trace. (Debug Addons -> Click "Debug" under the extension name)

Also, if you get a chance to play with the hotkey settings, let me know if you find any good working combinations. I don't really have any preferences on hotkeys so would be happy to include them or merge a PR.

Geobert commented 6 years ago

1 / 2 /3 / 4 don't work for me because I'm on a French layout. Maybe offer the possibility to customize the keys in some kind of UI? I'm thinking for the release when we will not be able to modify the manifest.json file anymore

track02 commented 6 years ago

Okay I see what you mean, it does seem that the commands api / manifest.json approach is quite limited.

Instead of listening for commands defined in the manifest you'd instead listen for keyboard events and match them against user defined key combinations set under a separate settings page.

I'll have a read of the documentation this afternoon and see what's available.

Geobert commented 6 years ago

Another thing I've noticed: The hotkeys don't work until we press the icon at least once. Maybe before clicking, the ext has not been linked to the youtube tab yet (especially in case of only one YT tab)

track02 commented 6 years ago

Yes you're correct, good find - clicking the icon runs the code behind the popup (YCtrl.js) which stores some information in the persistent browser storage (Current Tab, Time Skip Amount, etc).

The TabMonitor.js script which listens for YT tab changes (and now key presses) requests these browser storage values in order to send requests and parameters to the correct YT tab script (PageListener.js), if those values are not available it can't send any commands when a hotkey is pressed.

So the Tab Monitor could be updated to fall back to the last tab detected if no valid tab id is found in the browser storage, a default time skip value could be also used in a similar manner for seek requests.

I'll add this to the ToDo list

track02 commented 6 years ago

I think that's worked, I've pushed the changes if you'd like to try them. I'll have a look into customisable key-events this weekend.

Geobert commented 6 years ago

The very first time I tried it does not work, but I tried to reproduced since and I can't make it fail anymore. I'll post further details if ever I find how to reproduce it.

Good work!

track02 commented 6 years ago

Can you remember if a video was already playing when you loaded the extension? That's the only way I can think of it not being able to link to a YT Tab - I'll add a check for this case.

Geobert commented 6 years ago

No video was playing as far as I can remember

track02 commented 6 years ago

Sorry for the late update I've been a bit busy - unfortunately the Firefox API doesn't yet support listening for key events at the global/browser level (only predefined manifest hotkeys) so it's not quite as simple to implement as I thought.

I'm thinking of inserting a simple listening script behind every open page which can listen for events at the document level and then message the correct YT tab. I'll see if I can push an update sometime today if I find some spare time (and if it works!).

track02 commented 6 years ago

I was able to make a good bit of progress on the hotkeys, I've pushed a new branch if you'd like to take a look hotkey definitions are much more flexible now.

See KeyListener.js to define different keys and if you want to play the with modifiers change the conditional on line 13 event.altKey / event.ctrlKey / event.shiftKey are supported by Firefox as modifier events, you can have as many or as few as desired.

Any testing / bug reports are greatly appreciated, I'm aware of an issue with trying to use hotkeys when loading the extension with a video already playing and will add an initial test to check for that.

I'm going to look into a settings page next which allows for customisation of the keys and stores the config in the persisting browser storage. The KeyListener can pick up these values rather than using the hardcoded values.

track02 commented 6 years ago

Basic settings page is in which allows for hotkey customisation (See addons page -> Options button), still under the Hotkey_Testing branch.

I've also reworked some of the asynchronous calls so there shouldn't be any 'missed' commands which I think is what your earlier problem may have been. A startup check has been added too which looks for any playing videos when the extension is first loaded.

There's some testing to do now along with tidying up the code and formatting the settings page - hopefully I'll be able to push an update to the Addons Store this weekend.

Again, any feedback is appreciated.

Geobert commented 6 years ago

Thanks! I just restarted FFx, load this version, and hotkeys are not working anymore :-/

I've popped the browser console and got some warnings:

1522322102591   addons.xpi  WARN    Please specify whether you want browser_style or not in your options_ui options.
1522322102610   addons.xpi  WARN    Addon with ID YCtrl@track02.addons.mozilla.org already installed, older version will be disabled
1522322102732   addons.webextension.YCtrl@track02.addons.mozilla.org    WARN    Please specify whether you want browser_style or not in your browser_action options.

And when hitting keys CTRL + SHIFT, I got ALT + CTRL + (there's space inside) log

track02 commented 6 years ago

Not seen those warnings before unfortunately - do you have two extensions installed at once?

I've pushed another version to the branch, try that one - the wrong defaults were set in the hotkeys options page, it was displaying Ctrl + Shift but was using Ctrl + Alt.

Alt + Ctrl in the log is my mistake, I'd left it in whilst testing (removed it now), but if it's displaying then a hotkey is being picked up.

Geobert commented 6 years ago

I had the old one as well. I've uninstalled, restart FFx, load the test version, got this: Error: Could not establish connection. Receiving end does not exist. BrowserMonitor.js:30:5

and no hotkey

track02 commented 6 years ago

Not too sure at the moment to be honest I'm struggling to recreate the same error.

Are you starting the extension with a video playing already?

On startup the BrowserMonitor looks for already open YT tabs and stores the id of any tab that is currently playing (it sends a status request to the tab which is where your error comes from)

But it only checks tabs with a url matching the pattern ://.youtube.com/* so I'm not sure how it's trying to send a request to a tab without a listener behind it.

And Hotkeys don't work at all for you? Are you using the defaults (Ctrl + Alt + Space to play/pause)?

Geobert commented 6 years ago

I tried with and without a video playing, launching a video gives me:

71 BrowserMonitor.js:61:3
Error: Could not establish connection. Receiving end does not exist. BrowserMonitor.js:64:3 

I assume it's the tab's ID and then this error. Note that I'm running Nightly :)

EDIT: tried the Ctrl+Alt+Space and Ctrl+Shift+Space with no luck

track02 commented 6 years ago

That one's okay - when a tab changes (video loads) the BrowserMonitor sends a one-off message to the popup telling it to update the dropdown and current video details. If there's no popup open to receive the message then that error is automatically thrown.

I'm using Nightly too but things seem okay for me. If you could try the following that would help:

Edit - Are you trying to use hotkeys from the addons page itself (about:debugging#addons or about:addons)? Because on those pages extension input is disabled.

Geobert commented 6 years ago

Not working :( no log, and event after opening the popup it still not working :-/

track02 commented 6 years ago

I've added some trace statements to the branch to help debugging.

Try the latest version, repeat the steps above and copy out the extension console output and the web page console output.

Geobert commented 6 years ago
Beginning Startup Check BrowserMonitor.js:15:2
Checking Tab - https://xxxxxxx BrowserMonitor.js:24:4
Checking Tab - https://yyyyyy BrowserMonitor.js:24:4
Checking Tab - https://tweetdeck.twitter.com/ BrowserMonitor.js:24:4
Checking Tab - https://github.com/track02/Youtube-Player/issues/16#issuecomment-377201425 BrowserMonitor.js:24:4
Checking Tab - https://github.com/track02/Youtube-Player/tree/Hotkey_Testing
BrowserMonitor.js:24:4
Checking Tab - http://zzzzzzz BrowserMonitor.js:24:4
Checking Tab - http://ppppppp BrowserMonitor.js:24:4
Checking Tab - https://iiiiiiiiii BrowserMonitor.js:24:4
Checking Tab - https://eeeeeee BrowserMonitor.js:24:4
Checking Tab - about:addons BrowserMonitor.js:24:4
Checking Tab - about:debugging#addons BrowserMonitor.js:24:4
No Playing video found - falling back to first found -1 BrowserMonitor.js:50:4
[Show/hide message details.] Console is not defined BrowserMonitor.js:67
[Show/hide message details.] Unchecked lastError value: Error: Frame not found, or missing host permission cosmetic-filtering.js:1444
[Show/hide message details.] Unchecked lastError value: Error: Could not establish connection. Receiving end does not exist.
webrequest.js:118
[Show/hide message details.] Unchecked lastError value: Error: Frame not found, or missing host permission cosmetic-filtering.js:1444
Loaded! Options.js:2:1
updating dropdown YCtrl.js:202:3 

I've anonymize some tab info :) the log [Show/hide message details.] Console is not defined BrowserMonitor.js:67 seems to appeared when I've launched a YT video

Geobert commented 6 years ago

last one was without à YT running, here's one with a YT tab playing when loaded the extension:

Beginning Startup Check BrowserMonitor.js:15:2
Checking Tab - about:addons BrowserMonitor.js:24:4
Checking Tab - about:debugging#addons BrowserMonitor.js:24:4
Checking Tab - https://www.youtube.com/watch?v=nIsbXJ81SK0&feature=youtu.be&t=2m15s BrowserMonitor.js:24:4
Valid Tab found https://www.youtube.com/watch?v=nIsbXJ81SK0&feature=youtu.be&t=2m15s / 144 BrowserMonitor.js:28:5
No Playing video found - falling back to first found 144 BrowserMonitor.js:50:4
Current Tab ID Updated - 144 BrowserMonitor.js:40:7

removed unrelated tabs in this log :)

track02 commented 6 years ago

That's a typo on line 67 of BrowserMonitor change Console to console (or pull latest commit)

But that looks correct, it's finding the tab of your video and saving the id What does the document console show (inspect page -> console) when pressing hotkeys?

Geobert commented 6 years ago

oh, found a first issue, I'm using a keyboard layout which has _ on the space bar and reached by AtlGr+Space

Ctrl+Alt is sending AltGr code so I get an undescore instead of space.

I tried to change this to Ctrl-Shift-Space (which works before) but no luck:

Key press detected -   KeyListener.js:69:2
Modifer ctrl Pressed - true KeyListener.js:70:2
Modifer shift Pressed - true KeyListener.js:71:2
Modifer none Pressed - true
track02 commented 6 years ago

So your spacebar sends a different keycode? ( rather than " ")? Have you tried changing it to in the play pause hotkey instead of " "

Geobert commented 6 years ago

Ctrl+Alt+Space bar = "_" in my layout

I've done some more tests and made it work: I can't use Ctrl+Alt on my layout (http://bepo.fr/wiki/Accueil) as many keys have a char linked to AltGr + key.

So I changed to Ctrl+Shift and put U (capital u) and it works :)

My layout also produce another char when hitting Shift + space bar = non breakable space, that's why it was not working. Putting a non breakable space in the field with Ctrl+Shift works as well :)

Note that lower case u did not work as Ctrl+Shift+ key u send capital u :)

track02 commented 6 years ago

Ah that's interesting, I remembered you mentioned using a French layout in an earlier post and was starting to wonder if that might have been an issue with my defaults.

I'm not too sure how to get around that or alert the user though, maybe some 'test' functionality that lets you try key combinations from the settings page?

Geobert commented 6 years ago

Can you detect key code instead of the char? It will make the config more reliable

track02 commented 6 years ago

Do you mean codes like this? https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/code

That seems a much better idea.

Geobert commented 6 years ago

Yes :D

track02 commented 6 years ago

If you try the latest commit - does Ctrl + Alt + Space work for you now?

Geobert commented 6 years ago

no :( but I think it's related to my layout Ctrl+Alt are converted to AltGr

Key press detected - Space KeyListener.js:69:2
Modifer ctrl Pressed - false KeyListener.js:70:2
Modifer alt Pressed - false KeyListener.js:71:2
Modifer none Pressed - true
track02 commented 6 years ago

Any idea if your Alt + Ctrl keys would be picked up individually using Key Codes? E.g. we see two key codes instead of just a single one for AltGr?

Might need to rewrite the listener to use some kind of key map rather than the current events modifier properties- event.ctrlKey / event.altKey / event.shiftKey

Geobert commented 6 years ago

I think key codes should work, but I'm not sure :-/

track02 commented 6 years ago

No worries - If I get chance this weekend I'll rewrite the KeyListener and do some testing with a virtual French Layout keyboard.

Geobert commented 6 years ago

Don't hesitate to ask me to test ;) Thank you!

track02 commented 6 years ago

Of course - I'll make a post here when a new build is available to test out.

track02 commented 6 years ago

I've done a little more work on the Key Listener, it now works entirely from Key Codes rather than Characters. There's some more debug logging in there too to list out the Key Mapping.

On the options page you can choose from the Left Ctrl / Alt / Shift keys as modifiers and it should automatically convert keypresses into keycodes for the hotkey selection. I'll add more modifier options once I'm happy this system is working correctly.

Please feel free to test it out and let me know what works and what doesn't. You may need to remove and reinstall to clear out the browser storage.

Geobert commented 6 years ago

Thanks!

Ctrl-Alt-Space works now, but it also scroll the page Ctrl-Shift-Space ok Alt-Shift-Space not working:

Map(5) { None → true, AltLeft → true, ControlLeft → false, ShiftLeft → false, Space → false }
KeyListener.js:20:2
Map(5) { None → true, AltLeft → true, ControlLeft → false, ShiftLeft → true, Space → false }
KeyListener.js:20:2
Map(5) { None → true, AltLeft → true, ControlLeft → false, ShiftLeft → false, Space → false }
track02 commented 6 years ago

Looks like the browser window captures Alt key presses and takes focus from the document - you can see the key is left "true" after release, only when it's pressed again does it reset to "false".

So Alt + Shift + Space does 'work' if you toggle Alt by pressing it once, then just pressing Shift + Space.

I'll see if there's a way around this otherwise we may need to limit the allowable modifier combinations. Although I'm not sure if automatically disabling browser hotkeys is the best idea, it might need to be an optional feature.

track02 commented 6 years ago

I think agreeing on a set of default hotkeys that are known to work and are not already used by Firefox would be the best option. Users are then free to configure them as they wish but will need to be aware of any hotkeys already in use on their system.

I came across this list of Firefox Shortcuts that should be helpful: https://support.mozilla.org/en-US/kb/keyboard-shortcuts-perform-firefox-tasks-quickly

If you have any good hotkey combinations that work well for you and are not used by firefox we may be able to consider them for use as defaults (I don't really have any preferences). Also if you'd like any additional key modifiers to be added to the option lists let me know, it should be possible to use any key as a a modifer.

Geobert commented 6 years ago

I'm only using Ctlr-Shift-Space for play/pause :) I don't use the other action atm.

I agree on defining some default that are not conflicting and let the user do what he wants, with a disclaimer on the option page :)

track02 commented 6 years ago

I've tried finding some defaults, let me know if they work for you.

Play/Pause: Shift (+) Enter SeekFwk: Shift (+) + SeekBack: Shift (+) - NextVideo: Shift (+) > ReplayVideo: Shift (+) <

Geobert commented 6 years ago

It works :)