PedroBT03 commented 1 month ago

PROBLEM: The YouTube extension currently allows users to assign the same keybind to multiple tasks simultaneously, leading to potential conflicts and confusion among users.

SOLUTION: Introduce a feature that enables keybind exclusivity when assigning tasks.

Implement a dialog prompt for confirmation before removing a keybind from other tasks.
When a user attempts to assign an already assigned keybind to a new task, a modal dialog is shown asking whether to replace the existing assignment.
If the user confirms, the keybind is removed from the previous task and assigned to the new one. If the user cancels, the new assignment is not made.

IMPROVEMENTS:

Resolved inconsistencies in the reset button functionality to ensure it correctly handles keybind assignments.
Enhanced user experience by preventing multiple tasks from having the same keybind, aligning with common application practices.

NOTE: New shortcuts only function after refreshing the page, which is due to an existing bug. Similarly, the keybind is only effectively reassigned after a page refresh.

This resolves the issue reported in code-charity#2191.

ImprovedTube commented 1 month ago

hi! @PedroBT03

thanks! There could also remain the option to keep the current way (not impossible ever actually wants to do two things with one key)

Did you see #1565 ?

( we also made something to forbid website's defaults before https://github.com/code-charity/unlock-keyboard-and-mouse )

PedroBT03 commented 1 month ago

Thanks! We can definitely add the option to have the key assigned to different tasks and let the user decide, while also keeping the current functionality intact for those who prefer it.

I didn't see issue you mentioned initially, but I have reviewed it now.

When I refer to "default shortcuts," I mean the ones defined in the skeleton file (shortcuts.js). I believe you are referring to the website's defaults that work for regular YouTube, like the space bar for play/pause. We didn't work with those defaults, but we can make any necessary adaptations if needed.

Thanks again for the feedback!

Best regards, Pedro Tavares

raszpl commented 4 weeks ago

As I stated sometime ago satus Shortcuts code is non complete/broken. For example cant bind '>', it doesnt understand how Shift works. It also shows default YT shortcuts pretending to be able to change them :( This propagates bugs further, even to this commit :(

Commit review:

Tabs instead of spaces please.

Those should not be globals:

function  getAssignedShortcuts() {
function setAssignedShortcuts(shortcuts) {
function getDefaultShortcuts(
let assignedShortcuts
let defaultShortcuts

dont use localstorage:

    return JSON.parse(localStorage.getItem('assignedShortcuts')) || {};
    localStorage.setItem('assignedShortcuts', JSON.stringify(shortcuts));

because it makes setAssignedShortcuts(assignedShortcuts); only show clash between shortcuts assigned after this patch installed

getDefaultShortcuts() dont store data in code, read it from extension.skeleton.main.layers.section.shortcuts.on.click instead:

    function activeShortcuts() {
        let excluded = [
                'baseProvider',
                'layersProvider',
                'parentObject',
                'parentSkeleton',
                'namespaceURI',
                'svg',
                'parentElement',
                'rendered'
            ];
        let threads = 0,
            results = {};

        function parse(items) {
            for (const [key, item] of Object.entries(items)) {
                if (!excluded.includes(key)) {
                    if (item.component == 'shortcut' && item.value) {
                        results[key] = Object.assign({}, item.value);
                    }

                    if (satus.isObject(item)
                        && !satus.isArray(item)
                        && !satus.isElement(item)
                        && !satus.isFunction(item)) {
                        parse(item, items);
                    }
                }
            }
        };

        parse(extension.skeleton.main.layers.section.shortcuts.on.click);
        return results;
    };

alert('This keybind is already assigned to default shortcut');

no alerts please :) This is one of the bugs mentioned at the top, Instead we should either

display default YT shortcuts in a special way and prevent editing them to stop confusing Users.
actually intercept default keys and redirect them.

both are outside the scope of this Commit. Just do nothing here :|

showDialog

why custom popup?
should be named more descriptively, overrideShortcut?
should inform which shortcut clashes by name

text: 'This keybind is already assigned to '+ satus.locale.get(clashing_shortcut) +' shortcut. Do you want to replace it?', where clashing_shortcut is component.skeleton.text of clashing shortcut

dont iterate over all settings

for (shortcut in satus.storage.data) {
                            for (key in satus.storage.data[shortcut].keys) {
                                if (key == dataKey) {
                                    satus.storage.remove(shortcut);
                                }
                            }
                        }

its unlikely but possible for another setting to have 'keys' property, should iterate only over shortcuts https://github.com/code-charity/youtube/blob/e57bfc2ba23e97f4c6b3d8a75dccd4bd7082dc92/js%26css/web-accessible/www.youtube.com/shortcuts.js#L163-L164

It also ignores modifiers, so making 3 shortcuts '1', '1'+Shift (or rather '!'+Shift, impossible because our shortcuts code is bad as states at the top, so rather '!'+Shift), and another '1'+Shift will bring up a modal, answering Yes will delete both '1'+Shift and '1'.

                        this.parentNode.parentNode.parentNode.close();
                        showDialog(this);

dont close shortcut modal here before popping showDialog, only when user answers Yes inside showDialog.

                        window.removeEventListener('keydown', component.keydown);
                        window.removeEventListener('wheel', component.mousewheel);

copy&paste artefact? shortcut modal takes care of it on its own when closing

function isShortcutEqual(a, b) {
                                if (a.alt !== b.alt || a.ctrl !== b.ctrl || a.shift !== b.shift) return false;

fine for now. In the future Extension should store only active modifiers (no false values), then this will need to be '!!(a.alt) !== !!(b.alt)' etc

ImprovedTube commented 4 weeks ago

hi! @PedroBT03 (We didn't mean to have any defaults. Our defaults are just to show youtube's defaults )

Sorry this is more of a small project than a single issue...!...

Yet luckily we aren't alone. Following through with @raszpl's review here will be great (and afterwards #1565), since so many people already use our shortcuts inspite of issues - and we can eventually provide the final code for use in other extensions)
thanks!

PedroBT03 commented 3 weeks ago

Thank you @raszpl @ImprovedTube for your detailed review and feedback. I will address the issues you pointed out. Firstly, I will convert all spaces to tabs throughout the codebase. Additionally, I will refactor the functions and variables to avoid using global scope as requested.

Regarding the use of localStorage, I understand the concern and would appreciate some guidance on the preferred alternative method for storing and retrieving shortcut data. Can you tell us which alternatives we have to implement it? We used localstorage because the already used keybinds were lost when the page is refreshed.

I will modify getDefaultShortcuts() to dynamically read from extension.skeleton.main.layers.section.shortcuts.on.click as suggested. Furthermore, I will remove the alert for keybind conflicts and avoid implementing any changes related to default YouTube shortcuts within this commit.

I will rename showDialog to overrideShortcut and ensure it informs the user about the specific clashing shortcut. Additionally, I will adjust the iteration logic to ensure it only processes shortcuts, preventing potential conflicts with other settings. Finally, I will ensure the shortcut modal closes correctly only upon user confirmation, addressing the keydown event handling issue.

I will proceed with these changes promptly and look forward to your guidance on the best practice for managing shortcut data storage.

Thank you again for your guidance.

Best regards, Pedro

ImprovedTube commented 3 weeks ago

hi @PedroBT03! thank you!

1446 might explains more still😅

dont know why shortcuts are not really applied until reload, for other settings we used this section: https://github.com/code-charity/youtube/blob/48a579aa228d4b5adf5211d15d6ff73d056b3fce/js%26css/web-accessible/core.js#L180

defaults

https://github.com/code-charity/youtube/issues/1815

"Youtube's defaults: https://github.com/code-charity/youtube/issues/517 https://github.com/code-charity/youtube/issues/110 https://github.com/code-charity/youtube/issues/1566" (from #1565)

storage

BTW, generally, our settings storage yet is loaded into "ImprovedTube.Storage" (in core.js) and can be update from content scripts with chrome.storage.local, while web-accessible scripts need a detour to send a message first. while localstorage allows bigger data but is not available in incognito mode yet https://github.com/code-charity/youtube/issues/1408

raszpl commented 2 weeks ago

https://github.com/code-charity/youtube/issues/517 had a good idea of displaying YT defaults, but instead of being merely displayed they are currently editable, and afaik that doesnt work. You cant rebind "Seek forward 10 seconds" despite extension pretending you could. Fast option is to display all YT shortcuts either separately on the bottom or in different color and disable creation of https://github.com/code-charity/youtube/blob/6d369c11c3a621bf1bf6cbd502cff96537671024/menu/satus.js#L2325 when rendering those.

As I stated some time ago satus Shortcuts code is non complete/broken. For example cant bind '>', it doesnt understand how Shift works.

This table https://www.toptal.com/developers/keycode/table shows how js maps Key Codes. Pressing Shift + 2 returns

50
code "Digit2"
key "@"

which broken code in satus displays as Shift + @ instead of Shift + 2.

ImprovedTube commented 2 weeks ago

thank you! @raszpl

You cant rebind

strange!

( we also made something to forbid website's defaults before https://github.com/code-charity/unlock-keyboard-and-mouse )

(We could allow to deny some of YouTube's defaults)

raszpl commented 2 weeks ago

edited: forgot you need to reload for shortcuts to start working. Ok, so its not as dire as I thought.

You cant rebind

strange!

only user configured shortcuts are being interpreted by:

https://github.com/code-charity/youtube/blob/e57bfc2ba23e97f4c6b3d8a75dccd4bd7082dc92/js%26css/web-accessible/www.youtube.com/shortcuts.js#L18-L21

the "default" ones are ignored. Binding something to 'Seek forward 10 seconds' will still trigger for both new and old shortcut unless old one is overridden by another shortcut. In effect You arent rebinding 'Seek forward 10 seconds', you are adding a second 'Seek forward 10 seconds' shortcut.

Just realized we are missing an option to delete/unbind default shortcuts, a 4th button to display the shortcut empty and trigger a dummy preventDefault/stopPropagation action.

raszpl commented 2 weeks ago

read it from extension.skeleton.main.layers.section.shortcuts.on.click instead:

turns out extension.skeleton.main.layers.section.shortcuts.on.click doesnt have all default shortcuts, for example its missing M for mute. Discovered by accident by trying to use M for my shortcut :)

raszpl commented 1 week ago

Shortcuts code is non complete/broken. For example cant bind '>', it doesnt understand how Shift works.

hey there is already a bug for that https://github.com/code-charity/youtube/issues/1174 :)

ImprovedTube commented 1 week ago

@raszpl #1565 😅

raszpl commented 12 hours ago

aand another thing: when checking if shortcut collides with one of YT default ones we cant just compare 1:1. For example there is SHIFT+N Next. User might try to configure Control+Shift+N and comparison will let him leading to frustration.

EDIT: default

Increase playback speed
Decrease playback speed

shortcuts show <> but those are rewind/FF 5 seconds on YT

code-charity / youtube

feature: Implement Keybind Exclusivity in Task Assignment (#2191) #2312

1446 might explains more still😅