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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PedroBT03
Copy link
Contributor

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 #2191.

…ty#2191)

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.

Co-authored-by: João Costa <[email protected]>
@ImprovedTube
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor

raszpl commented May 30, 2024

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

for (var name in this.storage) {
if (name.indexOf('shortcut_') === 0) {

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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:

// REACTION OR VISUAL FEEDBACK WHEN THE USER CHANGES A SETTING (already automated for our CSS features):

defaults

#1815

"Youtube's defaults: #517 #110 #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 #1408

@raszpl
Copy link
Contributor

raszpl commented Jun 10, 2024

#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

component.addEventListener('click', function() {
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
Copy link
Member

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
Copy link
Contributor

raszpl commented Jun 11, 2024

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:

function handler() {
var prevent = false;
for (var key in storage) {

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
Copy link
Contributor

raszpl commented Jun 15, 2024

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
Copy link
Contributor

raszpl commented Jun 17, 2024

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

hey there is already a bug for that #1174 :)

@ImprovedTube
Copy link
Member

@raszpl #1565 😅

@raszpl
Copy link
Contributor

raszpl commented Jun 28, 2024

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

@ImprovedTube
Copy link
Member

#2421, #2429

hi! :) @PedroBT03

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants