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.

Sign up for GitHub

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

Replaced deprecated keyCode functionality and docs with KeyboardEvent.code & KeyboardEvent.key also updates the keyIsDown function to accept alphanumerics as parameters #7472

Open
wants to merge 6 commits into
base: dev-2.0
Choose a base branch
from
Open

Replaced deprecated keyCode functionality and docs with KeyboardEvent.code & KeyboardEvent.key also updates the keyIsDown function to accept alphanumerics as parameters #7472

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
dd458c0
changed keyboard.js and added the updated objects in costants.js file
Vaivaswat2244 Jan 17, 2025
bcbed0f
fixed the repeat key issue in onkeyup
Vaivaswat2244 Jan 17, 2025
c42bb7e
updated keyIsDown() to work with characters as arguments
Vaivaswat2244 Jan 18, 2025
5709aac
fixed dillema of code/key and added isCode()
Vaivaswat2244 Jan 23, 2025
0ce4e89
added tests for isCode and small changes
Vaivaswat2244 Jan 26, 2025
9cfef5a
Merge branch 'dev-2.0' into dev-2.0
Vaivaswat2244 Jan 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 42 additions & 30 deletions src/core/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -879,92 +879,104 @@ export const MITER = 'miter';
* @final
*/
export const AUTO = 'auto';

// INPUT
/**
* @typedef {18} ALT
* @typedef {'AltLeft' | 'AltRight'} ALT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why the typedef is both of these while the constant is just one?

* @property {ALT} ALT
* @final
*/
// INPUT
export const ALT = 18;
export const ALT = 'AltLeft';

/**
* @typedef {8} BACKSPACE
* @typedef {'Backspace'} BACKSPACE
* @property {BACKSPACE} BACKSPACE
* @final
*/
export const BACKSPACE = 8;
export const BACKSPACE = 'Backspace';

/**
* @typedef {17} CONTROL
* @typedef {'ControlLeft' | 'ControlRight'} CONTROL
* @property {CONTROL} CONTROL
* @final
*/
export const CONTROL = 17;
export const CONTROL = 'ControlLeft';

/**
* @typedef {46} DELETE
* @typedef {'Delete'} DELETE
* @property {DELETE} DELETE
* @final
*/
export const DELETE = 46;
export const DELETE = 'Delete';

/**
* @typedef {40} DOWN_ARROW
* @typedef {'ArrowDown'} DOWN_ARROW
* @property {DOWN_ARROW} DOWN_ARROW
* @final
*/
export const DOWN_ARROW = 40;
export const DOWN_ARROW = 'ArrowDown';

/**
* @typedef {13} ENTER
* @typedef {'Enter'} ENTER
* @property {ENTER} ENTER
* @final
*/
export const ENTER = 13;
export const ENTER = 'Enter';

/**
* @typedef {27} ESCAPE
* @typedef {'Escape'} ESCAPE
* @property {ESCAPE} ESCAPE
* @final
*/
export const ESCAPE = 27;
export const ESCAPE = 'Escape';

/**
* @typedef {37} LEFT_ARROW
* @typedef {'ArrowLeft'} LEFT_ARROW
* @property {LEFT_ARROW} LEFT_ARROW
* @final
*/
export const LEFT_ARROW = 37;
export const LEFT_ARROW = 'ArrowLeft';

/**
* @typedef {18} OPTION
* @typedef {'AltLeft' | 'AltRight'} OPTION
* @property {OPTION} OPTION
* @final
*/
export const OPTION = 18;
export const OPTION = 'AltLeft';

/**
* @typedef {13} RETURN
* @typedef {'Enter'} RETURN
* @property {RETURN} RETURN
* @final
*/
export const RETURN = 13;
export const RETURN = 'Enter';

/**
* @typedef {39} RIGHT_ARROW
* @typedef {'ArrowRight'} RIGHT_ARROW
* @property {RIGHT_ARROW} RIGHT_ARROW
* @final
*/
export const RIGHT_ARROW = 39;
export const RIGHT_ARROW = 'ArrowRight';

/**
* @typedef {16} SHIFT
* @typedef {'ShiftLeft' | 'ShiftRight'} SHIFT
* @property {SHIFT} SHIFT
* @final
*/
export const SHIFT = 16;
export const SHIFT = 'ShiftLeft';

/**
* @typedef {9} TAB
* @typedef {'Tab'} TAB
* @property {TAB} TAB
* @final
*/
export const TAB = 9;
export const TAB = 'Tab';

/**
* @typedef {38} UP_ARROW
* @typedef {'ArrowUp'} UP_ARROW
* @property {UP_ARROW} UP_ARROW
* @final
*/
export const UP_ARROW = 38;
export const UP_ARROW = 'ArrowUp';

// RENDERING
/**
Expand Down
55 changes: 40 additions & 15 deletions src/events/keyboard.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,8 @@ function keyboard(p5, fn){
*/
fn.isKeyPressed = false;
fn.keyIsPressed = false; // khan
fn._code = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is only used in this module and not used by other modules through the instance, it should be a local variable and not attached to fn if possible.

Copy link
Author

@Vaivaswat2244 Vaivaswat2244 Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, currently user is able to do
text(key, 50, 50) giving the key of the key pressed.
Maybe the same can be done for code with added documentation.

fn.key = '';

/**
* A `String` system variable that contains the value of the last key typed.
Expand Down Expand Up @@ -440,15 +442,16 @@ function keyboard(p5, fn){
* </div>
*/
fn._onkeydown = function(e) {
if (this._downKeys[e.which]) {
// prevent multiple firings
this._code = e.code;
// Check for repeat key events
if (this._downKeys[e.code]) {
return;
}
this.isKeyPressed = true;
this.keyIsPressed = true;
this.keyCode = e.which;
this._downKeys[e.which] = true;
this.key = e.key || String.fromCharCode(e.which) || e.which;
this.key = e.key;
this._downKeys[e.code] = true;
const context = this._isGlobal ? window : this;
if (typeof context.keyPressed === 'function' && !e.charCode) {
const executeDefault = context.keyPressed(e);
Expand Down Expand Up @@ -614,18 +617,19 @@ function keyboard(p5, fn){
* </div>
*/
fn._onkeyup = function(e) {
this._downKeys[e.which] = false;
delete this._downKeys[e.code];

if (!this._areDownKeys()) {
if (Object.keys(this._downKeys).length === 0) {
this.isKeyPressed = false;
this.keyIsPressed = false;
this.key = '';
this._code = null;
} else {
// If other keys are still pressed, update code to the last pressed key
const lastPressedKey = Object.keys(this._downKeys).pop();
this._code = lastPressedKey;
}

this._lastKeyCodeTyped = null;

this.key = e.key || String.fromCharCode(e.which) || e.which;
this.keyCode = e.which;

const context = this._isGlobal ? window : this;
if (typeof context.keyReleased === 'function') {
const executeDefault = context.keyReleased(e);
Expand Down Expand Up @@ -902,11 +906,32 @@ function keyboard(p5, fn){
* </code>
* </div>
*/
fn.keyIsDown = function(code) {
// p5._validateParameters('keyIsDown', arguments);
return this._downKeys[code] || false;
p5.prototype.keyIsDown = function(code) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should still be fn.keyIsDown and the indentation fixed.

console.log('Current _downKeys:', this._downKeys);
console.log('Current key:', this.key);

// For backward compatibility - if code is a number
if (typeof code === 'number') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work if our key event handlers only set _downkeys[e.code]?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also @limzykenneth do you think we need backwards compatibility? We might need to put a lookup table like this https://stackoverflow.com/a/66180965 into our code to map old key codes to keys, although it looks like it's hard to get complete backwards compatibility. That's probably enough though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the numerical code has been deprecated as a standard, we can remove it since its the point of this implementation to use the easier code property instead.

return this._downKeys[code] || false;
}

// For string inputs (new functionality)
if (typeof code === 'string') {
// Handle single character inputs
if (code.length === 1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should also add an else if branch to handle the multi-character possibilities for key, for things like Enter and the arrow keys: https://developer.mozilla.org/en-US/docs/Web/API/UI_Events/Keyboard_event_key_values If it's one of those, we would need to check against down keys as opposed to down key codes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some overlap between key and code, we'll need to decide which to prefer in case of overlap I think.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here, we are using key only for the single characters(because of same code for characters i.e. keyA for 'a' and 'A') and for rest of the inputs we are using code

if (/[A-Za-z]/.test(code)) {
// For letters, we need to check the actual key value
return this.key === code;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this only checks if the last pressed key was the one passed in, rather than if that key is currently down. We might need to make two objects, _downKeyCodes and _downKeys, and update both on keydown/keyup.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, got it

} else if (/[0-9]/.test(code)) {
return this._downKeys[`Digit${code}`] || false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably be consistent and check for keys if code.length === 1 and check for key codes otherwise.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, we are currently checking with key if code.length === 1. and checking for code otherwise. I'm assuming that we have to completely replace the usage the keyCode ig

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fully replacing event.keyCode is correct! I think ideally we could tell users that if they pass in a code (e.g. 'Digits3'), then it will check if that physical key is pressed, and if the user passes in a character (e.g. '3'), it checks whether whatever key produces that character is pressed. I think that would mean we have to detect whether they passed in an event.key or an event.code, and then handling each branch separately.

The slight inconsistency right now is if the user passes in the string '3', it is checking for the physical key via the code still.

Copy link
Author

@Vaivaswat2244 Vaivaswat2244 Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, got it.
I think this logic might work, for allowing the user to enter both key and code and seperate classification of them

fn.keyIsDown = function(input){
    if (typeof input ==='string'){
      if (input.length === 1) {                 
        return this._downKeys[input] ||
               (/[0-9]/.test(input) && this._downKeyCodes[`Digit${input}`])   
                || false; 
      }
      return this._downKeyCodes[input] || this._downKeys[input] || false;
    }
  
    return false;
  };

here _downKeyCodes is storing e.code and _downKeys is storing e.key.
now if user passes 'a' or '3', this will be checked by key and returned true,
and if passes 'Digit3', this will be checked by code and returned true. and for special keys such as Enter, ' ', Shift, ShiftLeft, this will give true as they are getting checked for key or code.
Hence, user can use either key or code for the keyisDown function,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! It might makes sense to make a isCode() function that we can call, so you can do:

fn.keyIsDown = function(input) {
  if (isCode(input)) {
    return this._downCodes[input];
  } else {
    return this._downKeys[input];
  }
}

...and possibly also unit test isCode separately too.

}
}
// Handle direct code inputs (e.g., 'KeyA', 'ArrowLeft', etc.)
return this._downKeys[code] || false;
}

return false;
};

/**
* The _areDownKeys function returns a boolean true if any keys pressed
* and a false if no keys are currently pressed.
Expand Down