HolodexNet / Holodex

Holodex frontend source code
https://holodex.net
MIT License
525 stars 90 forks source link

Feature Request: Hotkeys for MultiView's Media Control Functions #428

Open johnchen40904 opened 2 years ago

johnchen40904 commented 2 years ago

I would like to request a feature to access those media control functions in MultiView via hotkeys / keyboard shortcuts. For example, the play all / pause all, sync all, adjust all volumes functions that were currently only accessible by clicking.

LogicalContradiction commented 1 year ago

I'm interested in implementing this. Is there anyone currently working on it?

LogicalContradiction commented 1 year ago

I’m looking to get some feedback on my implementation as well as have a few questions answered.

My current solution uses a variable to keep track of an index in the list of videos that are currently opened. Whenever the user opens the media controls, this variable is set to -1, and by using a key combination, the user can increment and decrement the index. At any given point, when the user presses a supported hotkey, it will be applied to the cell with the index in that variable.
For the hotkeys to increment and decrement the index, as well as the toggle for “mute all but one source,” I came up with these two options:

Option 1:

Action Hotkey
next control (increment index) shift + n
prev control (decrement index) shift + p
mute all but one ctrl + m

Option 2:

Action Hotkey
next control (increment index) ctrl + down arrow
prev control (decrement index) ctrl + up arrow
mute all but one ctrl + m

Option 1 uses YouTube’s hotkeys for next/prev video in a playlist as the increment/decrement keys, but uses ctrl for the mute toggle, making it different. Option 2 uses ctrl for all its hotkeys which makes them uniform, but switches the next/prev keys for up/down arrow, since ctrl + n opens a new window and ctrl + p opens the print dialog box. There may be an issue since ctrl + up/down arrow moves up/down by a paragraph, but I don’t think it will apply here since we’re not working with editable text.

For the media controls that affect all of the cells at once, I came up with the following keybinds: Action Hotkey
play shift + k
pause shift + k
sync shift + s
refresh shift + r
mute shift + m
unmute shift + m
increase volume shift + right arrow
decrease volume shift + left arrow

These are all based off of the hotkeys YouTube uses, but I have a question. Mute/unmute and play/pause are separate buttons here, unlike YouTube. Should I make those pairs of hotkeys the same and write a function that toggles between them, or should I give each one its own hotkey? The arrow keys again have a possible issue that when text is highlighted, shift + arrow left/right continues highlighting the text, but again, I don’t think this will be an issue.

The media controls I chose for a single cell are again based on YouTube’s hotkeys and are these: Action Hotkey
play/pause k
sync s
refresh r
mute/unmute m
increase volume right arrow
decrease volume left arrow
remove stream d

I also wanted to discuss how to indicate to the user what video their keys will be targeted to. I came up with 3 possible options (colors for demonstration only):

Option 1 (background only):

background

Option 2 (outline only):

outline

Option 3 (background + outline):

outline+background

Which should I go with, and what color(s) should I use? Finally, is there a preferred way to implement this? I currently assign each volume control an id and use document.getElementById() to directly change each style. I’m concerned about maintainability, since doing it this way would hard-code the style changes. Looking into other options, it seems I can define a class with the style and dynamically apply it with a ternary expression like this:

:class = "isFocusCellIndex ? 'focusedCellClass' : '' "

Which method should I use, or is there another way that I should be using?

RiceCakess commented 1 year ago

I like following youtube's hot keys. I suggest just using one modifier (shift) for everything. So to select between each video, it's shift + up/down. And then to apply to all videos it's the same as single except with shift + k, left, right, s, r...

I have a question. Mute/unmute and play/pause are separate buttons here, unlike YouTube. Should I make those pairs of hotkeys the same and write a function that toggles between them, or should I give each one its own hotkey?

Make it toggle using a single key instead of assigning two keys.

I also wanted to discuss how to indicate to the user what video their keys will be targeted to. I came up with 3 possible options (colors for demonstration only): Which should I go with, and what color(s) should I use?

Probably option 2 using the primary color that is used across the site. (the pink)

Finally, is there a preferred way to implement this?

We have a vue-keypress library that is used in various places to define hotkeys. Adding styles to the active selected video should be done in the component itself (MediaControls)

LogicalContradiction commented 1 year ago

I would like to get some more feedback as well as have a few more questions answered.

My first question is what hotkey should I use for the “mute all but one” toggle? shift+ m is already being used as the mute all key.

For the mute/unmute hotkey, I have it so that if any video is unmuted, it mutes all, otherwise, it unmutes. Is this fine, or would you prefer I reversed it and had it always unmute unless all are muted? I have a similar question about pause/unpause. I’m going to make it so if any video is playing, it pauses all, and if all are paused, it unpauses. Is that fine?

In order to add pause/unpause, I’ve been trying to add either a function or a variable to both TwitchPlayer and YoutubePlayer that I can use to determine if the player is playing. I’ve implemented a computed variable in both of the players as well as in VideoCell, but every time I try to access it, it’s undefined. From my understanding, in MediaControls I should call this.cells[index].playing which should access VideoCell.playing, but this is where I get lost. I tried to make it exactly like the muted computed property by accessing this.cellContent.playing in VideoCell, but that didn’t work. I also tried accessing this.ytPlayer.playing in VideoCell, but that didn’t work either. I think I’m missing something, so any guidance would be appreciated.

How thick should the outline on the focused controls be? Here’s what it would look like with a thickness of 1 - 6 px:

border_comparison_1-6px
I personally like 4 or 5 px.

Finally, should I hide the gray border on the bottom of each control when it’s outlined? You can see the gray border on the top part of the bottom outline. Here’s a comparison:

bottom-border-color_comparison
I like it better hidden.

Thank you again for your help.

RiceCakess commented 1 year ago

My first question is what hotkey should I use for the “mute all but one” toggle? shift+ m is already being used as the mute all key.

I think it's fine to not have a toggle or just use like ctrl + shift + m

For the mute/unmute hotkey, I have it so that if any video is unmuted, it mutes all, otherwise, it unmutes. Is this fine, or would you prefer I reversed it and had it always unmute unless all are muted? I have a similar question about pause/unpause. I’m going to make it so if any video is playing, it pauses all, and if all are paused, it unpauses. Is that fine?

Yea that should be fine

I think I’m missing something, so any guidance would be appreciated.

Maybe change this here to be from c.manualCheckMuted() => c.manualRefresh()

    created() {
        // Nothing else needs to be updated in an interval, except for checking for mute changes
        // Premature optimization.... probably
        if (!this.timer) {
            this.timer = setInterval(() => {
                if (this.cells) this.cells.forEach((c) => c.manualCheckMuted());
            }, 1000);
        }
    },

How thick should the outline on the focused controls be? Here’s what it would look like with a thickness of 1 - 6 px:

2px with maybe a little bit of rounded-ness? to match the rounded ness of the modal box. Hidden line is fine

LogicalContradiction commented 1 year ago

The hotkeys are done and ready to go, I just wanted to get one final review before submitting it.

How it works: the user opens the media controls by pressing the button in the top bar. From there, any hotkey the user inputs will be applied to either a single player or all of them, depending on the hotkey used. Hotkeys will only work when the media controls pop-up is visible, and the targeted player control will reset when the window is closed.

Here is a list of all the hotkeys:
Action
Single Video Hotkey Multi-Control Hotkey
next video control - shift + down arrow
prev video control - shift + up arrow
mute others toggle - shift + o
play/pause k shift + k
sync s shift + s
refresh r shift + r
mute/unmute m shift + m
volume increase right arrow shift + right arrow
volume decrease left arrow shift + left arrow
delete video cell d -

Next, here is the indicator to show what video will be affected by the hotkey.
30-05-2023-outline_2px_border-radius_4px_cropped
It’s 2px thick and just like the search bar, search button, and filter button on the main page, the border-radius is 4px for the rounded corners.

Please let me know if there’s any issues or final changes I should make to these before submitting.

Finally, an issue I ran into. The keys work fine for the YouTube player, but do not work with the Twitch player. It seems to be a bug with the VideoCell since the regular multiview media controls also don’t work on any Twitch player (specifically, pause/play, mute/unmute, and volume slider): even on the current live app. I should submit this as a bug, right?

sphinxrave commented 1 year ago

This looks good... yeah we're considering a slight rewrite of the vue 2 -> vue 3 branch but we'll likely incorporate the design into any products. One question though, is it really that useful if this hotkey only works when the media control window is open? It's good to have keyboard navigation for that space, don't get me wrong, but the original user might've wanted access to functions in a more global context.