dvlden / ultrawideo-v2

Upscale or stretch any video on the web, to make it look great on UltraWide screens.
MIT License
133 stars 20 forks source link

Improving short-keys #43

Closed Juraj-Masiar closed 3 years ago

Juraj-Masiar commented 3 years ago

Hello, I'm a Windows user and I have no idea what "meta" key is. As Windows users are majority of the web users, you really should use something more user friendly. Note that you can use getPlatformInfo API to get operating system user is using and adjust default shot-key: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/runtime/getPlatformInfo

Also, one thing that bugs me is that I have to press all keys of the shot-key again if I want to trigger it again - for example to change crop I have to press "Ctrl + Shift + z" although I'm already holding "Ctrl + Shift" and I want to just keep pressing "z". But it will work only once and then I have release Ctrl and Shift and then press them again. Thanks!

dvlden commented 3 years ago

Hello @Juraj-Masiar ,

Thanks for opening an issue. I appreciate the note of getPlatformInfo, that is quite useful. I'll make sure to implement this ASAP, after holidays.

I think that Meta key on Windows, represents the actual (Windows logo cap). So yeah, that's odd.

It is interesting that you mention such thing. I am definitely not pressing all three keystrokes, that'd be the worst UX. Either it's WIndows OS limitation or specific keystrokes aren't working properly on Windows and it asks you to let go of all keys and press again?

Can you investigate that further? Because on macOS there's no such issue and I'm unsure if I wrote tests that cover such case of trying to toggle without letting go of two main keystroke caps.

Juraj-Masiar commented 3 years ago

So regarding those pressed keys, I've checked your code and first I have to say I really like code organization :). I've never seen so much object oriented patterns in a JS codebase :D. You should switch to TypeScript, it would make things even better.

Anyway your method of recording short-keys in shortcut.js seems over-complicated. Why not register simple keydown listener and check what buttons are being held down right now? It should be all in the event object with each key press. I would guess when I press the "z" key over and over it just being added to the array (this.keys) because the release event wont fire while I hold the modifier key, but I'm just guessing...

Alternatively you could just remove whole shortcuts code and use the commands API (global short-keys). But it would be slightly harder for users to change their shot-keys because it's a bit hidden in Chrome. In Firefox you can update it at runtime. https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/commands

dvlden commented 3 years ago

Haha thanks. Yes, I plan two rewrite everything to TypeScript this year. That is the goal at least.

Hmmm, is it? I think it's the normal/simple solution. I did not write much JS lately, so I went to CodePen to check if keydown is going to record all pressed keys at once, but it just records one key. So my solution there, seems okay I guess 😕 ?

Repeated keys won't be added to the current implementation. I'd highly appreciate any further feedback and help!

P.S. Yes, I used commands api in first release, but that ended up badly.

So I gave up on it and made this short Shortcuts module.

Juraj-Masiar commented 3 years ago

If you open console here and execute:

window.addEventListener('keydown', console.log)

And then focus page and click "Ctrl + Shift + z" then it should print event with something like this:

altKey: false
​charCode: 0
​code: "KeyZ"
​ctrlKey: true
​key: "Z"
​keyCode: 90
​metaKey: false
​shiftKey: true
​type: "keydown"
which: 90

Notice the both "​ctrlKey" and "​shiftKey" are true, which is correct.

There is a nice page for testing events: https://unixpapa.com/js/testkey.html This is what it logs out when I press "Ctrl + Shift + z + z + z + z + z...".

keydown  keyCode=17        which=17        charCode=0        
keydown  keyCode=16        which=16        charCode=0        
keydown  keyCode=90  (Z)   which=90  (Z)   charCode=0        
keyup    keyCode=90  (Z)   which=90  (Z)   charCode=0        
keydown  keyCode=90  (Z)   which=90  (Z)   charCode=0        
keyup    keyCode=90  (Z)   which=90  (Z)   charCode=0        
keydown  keyCode=90  (Z)   which=90  (Z)   charCode=0        
keyup    keyCode=90  (Z)   which=90  (Z)   charCode=0        
keydown  keyCode=90  (Z)   which=90  (Z)   charCode=0        
keyup    keyCode=90  (Z)   which=90  (Z)   charCode=0        
keydown  keyCode=90  (Z)   which=90  (Z)   charCode=0        
keyup    keyCode=90  (Z)   which=90  (Z)   charCode=0        
keydown  keyCode=90  (Z)   which=90  (Z)   charCode=0        
keyup    keyCode=90  (Z)   which=90  (Z)   charCode=0        
keydown  keyCode=90  (Z)   which=90  (Z)   charCode=0        
keyup    keyCode=90  (Z)   which=90  (Z)   charCode=0        
keyup    keyCode=16        which=16        charCode=0        
keyup    keyCode=17        which=17        charCode=0     

Try what will you get on Mac.

dvlden commented 3 years ago

Oh my, oh my. That is indeed neat. Let me try one more time on my pen...

dvlden commented 3 years ago

Yeah, it is nice and I can see now that everything is within the Event record, as you said. However, nothing comes to my mind, regarding visual representation of the pressed keys.

Instead of having it automatically shown, as I have it now, I'd need to create a list of the keys and check which of the meta keys are pressed.

For example:

// I decide to press CTRL + SHIFT + Z

const pressedKeyCombo = []

if (e.ctrlKey) {
   pressedKeyCombo.push('Control')
   return
}

if (e.shiftKey) {
  pressedKeyCombo.push('Shift')
  return
}

pressedKeyCombo.push(e.key)

pressedKeyCombo.join('+') // Control + Shift + Z

That is, in order to get result as a value of the input that records the shortcut. The current approach gives me flexibility as every key binding has the name properly handled.

Am I missing something?


Aside, I agree that current Shortcut implementation is not great. I can set ALT + CONTROL + META to be the keystroke, which is highly unlikely going to do anything at all in the browser.

I must make it so that it supports at least 2, but maximum of 3 modifier keys and one letter or number. Maybe exclude special chars entirely. (unsure)

Juraj-Masiar commented 3 years ago

The visual representation is OK. This issue is only about the two problems: 1) windows users doesn't know what "Meta" key is. So just rename it to "Win key" or something like that if you detect that user is using Windows OS (using the API I've mentioned). Nothing else is needed since as I see the "e.metaKey" is there anyway. 2) the last pressed key "z" is not registered when pressed multiple times - this is 100% related to your code of "recording" key-pressing where you could just use "detecting" keys that are pressed in the current single event (without any key-state related state change - which is most probably OS specific). I'm not sure about implementation but I would really go the simplest way possible. Since there is only a handful of modifier keys, you can check them all, something like:

if (  
  userSettingCtrl === e.ctrlKey &&
  userSettingAlt === e.altKey &&
  userSettingShift === e.shiftKey &&
  userSettingKey === e.key
) {...}
// where "userSettingXXX" is boolean value based on what user shot-key setup

It's just an idea...

dvlden commented 3 years ago

Revisiting this issue. It should be fixed in latest commit. I did a partial rewrite of it and tests are still passing, but time will tell. Please wait for updated release on the stores for v2.4.0 and let me know if it is working now as expected?

I tested on Windows 10 - Google Chrome and toggling is now working without issues. I should mention two things:

  1. Meta key has been entirely disabled as it produces undesired issues for macOS users.
  2. Default keys are changed to Control + Shift + 0 and Control + Shift + 9

Basically Control is now a replacement for Meta.

Juraj-Masiar commented 3 years ago

Hello, I've tested it now in Firefox but it doesn't work well. The default shot-key Ctrl + Shift + 9 (or 0) doesn't work at all (I've tried normal and numeric keys). I think the "Shift" key will change the key code or something like that...

When remapping to: Pause: Ctrl + Shift + Z Mode: Ctrl + Shift + X The toggle mode will work great, but the toggle pause doesn't toggle, it can do the OFF but it won't turn ON anymore.

When I use the fancy page to record events, I see this for Ctrl + Shift + 9:

keydown  keyCode=16        which=16        charCode=0        
keydown  keyCode=17        which=17        charCode=0        
keydown  keyCode=57  (9)   which=57  (9)   charCode=0        
keyup    keyCode=57  (9)   which=57  (9)   charCode=0        

And this is the event:

altKey: false
​bubbles: true
​cancelBubble: false
​cancelable: true
​charCode: 0
​code: "Digit9"
​composed: true
​ctrlKey: true
​currentTarget: null
​defaultPrevented: false
​detail: 0
​eventPhase: 0
​isTrusted: true
​key: "("
​keyCode: 57
​layerX: 0
​layerY: 0
​location: 0
​metaKey: false
​rangeOffset: 0
​rangeParent: null
​repeat: false
​returnValue: true
shiftKey: true
​timeStamp: 1341133
type: "keydown"
which: 57

As you can see, the "key" is "(" because that's the "Shift + 9".

dvlden commented 3 years ago

Thank you for the feedback. I just tested the defaults and you're right. Control + Shift + 0 or ) which is the char while Shift is being held, doesn't work on Windows for some reason. It appears to be reserved to something internally.

Configuring custom shortcuts is working for you, correct?

That is intended. You can read that on the info circle of the Master section. If I keep listening for keyboard events when someone wants to pause the extension, the pause doesn't make a lot of sense. It stops listening for all events that it uses. (fullscreen and keyboard)

Screen Shot 2021-01-29 at 6 33 38 PM

Seems like I must change default shortcuts. Do you have any suggestion for sensible defaults?

Juraj-Masiar commented 3 years ago

Regarding the pause toggle - if it can only switch off, then it's not a toggle :D. You should rename it to "Stop" or "Switch OFF".

The default shot-key is always a pain to pick... there is so little to choose from. But actually why does it have to be a multi-key? It could be a single key as well, right? It's not like user can be typing inside a fullscreen video... So why not make it simple "m".

dvlden commented 3 years ago

It's good point, sort of. Pause is still a toggle switch, its just not possible to toggle it – via shortcut. I know, that's why I'm wondering what would be the best sensible default.

Do you think that Pause maybe doesn't deserve a shortcut? Sometimes I am accidentally pressing pause instead of mode toggling when I use keystrokes. Maybe removing keystroke on it is a good decision?

That's not smart to do so, as many platforms put single letter keystrokes as a shortcut dedicated to a player. For example, YouTube uses m for Mute toggling. But maybe something like Control + Shift + M or Control + Shift + Z (as it can be handled with a single hand)?

I'd really like your input here and feedback from other people that use extension too.

UPDATE: Looks like CTRL + SHIFT + M is reserved on Windows in Chrome. This will be hard to determine, because of system and browser differences.

Here's a list of reserved keystrokes:

etc...

dvlden commented 3 years ago

Investigation

Attempting to find the best possible sensible default across all platforms and browsers.

Firefox Windows, Linux

Ctrl + Shift + Z - Redo Ctrl + Shift + X - Moves the URL left or right (if cursor is in the address bar)

It does not seem that these reserved keys are preventing UltraWideo from using the same keystrokes.

Opera (all OS')

Ctrl + Shift + Z - Redo the last change you made. Ctrl + Shift + X - Quit Opera (don’t worry - you’ll get a confirmation window).

Z appears to work fine, no problems, but X warns to quit Opera and prevents the extension pausing.

If I were to remove pause keystroke, which I think is smart thing to do, then X would become irrelevant.

Juraj-Masiar commented 3 years ago

Your Pause button is a toggle, your pause short-key is not a toggle. From a Wiki about toggle switch:

In some contexts, particularly computing, a toggle switch, or the action of toggling, is understood in the different sense of a mechanical or software switch that alternates between two states each time it is activated, regardless of mechanical construction.

And yes, I would say remove it, especially if it's confusing for users. It happened to me as well - I've pressed it by accident and then my correct shot-key stopped working...

The fact that YouTube is using "M" for mute should tell you that it's completely OK to use single key shot-key to control the video. Just like desktop video players are using many of single key shot-keys (imagine full-screen toggle to be "Ctrl + Shift + F" instead of just "F"). The point of multi-key is just to avoid collisions with normal work, but in full-screen it's not a problem. So why not keep it simple?

dvlden commented 3 years ago

Yeah, it appears to be making confusion. I think I'll remove it in the next update.

No, the fact that single letter is already being used for commands of the video player, tells me that it's not ok to be used by the extension, to avoid collision between different players and platforms.

Default keys are now changed and I tested them across two OS' and all browsers. There's only collision in Opera where Pause won't work as it's a shortcut to quit the browser. But that problem will disappear with the removal of Pause keystroke.

I appreciate your input and collaboration on this issue @Juraj-Masiar ! ❤️