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

Narrow down enum types #431

Merged
merged 13 commits into from
Dec 25, 2023
Merged

Narrow down enum types #431

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
449dfa6
narrow down enum types
Casheeew Dec 22, 2023
a525466
add enum types
Casheeew Dec 22, 2023
c250987
change from default to case
Casheeew Dec 23, 2023
dcb9dfd
add enum types
Casheeew Dec 23, 2023
770e224
remove comments
Casheeew Dec 23, 2023
6d7500e
remove comments
Casheeew Dec 23, 2023
1fadb86
fix
Casheeew Dec 23, 2023
bb0ffb2
Move getErrorLevelValue to Logger
Casheeew Dec 23, 2023
54d0832
Add enum type for LogErrorLevelValue
Casheeew Dec 23, 2023
49f9caa
add eslint switch-exhaustiveness-check rule
Casheeew Dec 25, 2023
9cd8423
Revert "add eslint switch-exhaustiveness-check"
Casheeew Dec 25, 2023
1f04a56
move from labelled loop to helper functions
Casheeew Dec 25, 2023
047c581
move helper functions downward
Casheeew Dec 25, 2023
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
19 changes: 3 additions & 16 deletions ext/js/background/backend.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,8 +341,9 @@ export class Backend {
* @param {{level: import('log').LogLevel}} params
*/
_onLog({level}) {
const levelValue = this._getErrorLevelValue(level);
if (levelValue <= this._getErrorLevelValue(this._logErrorLevel)) { return; }
const levelValue = log.getLogErrorLevelValue(level);
const currentLogErrorLevel = this._logErrorLevel !== null ? log.getLogErrorLevelValue(this._logErrorLevel) : 0;
if (levelValue <= currentLogErrorLevel) { return; }

this._logErrorLevel = level;
this._updateBadge();
Expand Down Expand Up @@ -1452,20 +1453,6 @@ export class Backend {
return results;
}

/**
* @param {?import('log').LogLevel} errorLevel
* @returns {number}
*/
_getErrorLevelValue(errorLevel) {
switch (errorLevel) {
case 'info': return 0;
case 'debug': return 0;
case 'warn': return 1;
case 'error': return 2;
default: return 0;
}
}

/**
* @param {import('settings-modifications').OptionsScope} target
* @returns {import('settings').Options|import('settings').ProfileOptions}
Expand Down
15 changes: 15 additions & 0 deletions ext/js/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -734,6 +734,21 @@ export class Logger extends EventDispatcher {
error(error, context = null) {
this.log(error, 'error', context);
}

/**
* @param {import('log').LogLevel} errorLevel
* @returns {import('log').LogErrorLevelValue}
*/
getLogErrorLevelValue(errorLevel) {
switch (errorLevel) {
case 'log':
case 'info':
case 'debug':
return 0;
case 'warn': return 1;
case 'error': return 2;
}
}
}

/**
Expand Down
227 changes: 135 additions & 92 deletions ext/js/dom/dom-text-scanner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ export class DOMTextScanner {
const nodeValueLength = nodeValue.length;
const {preserveNewlines, preserveWhitespace} = this._getWhitespaceSettings(textNode);

let done = false;
let lineHasWhitespace = this._lineHasWhitespace;
let lineHasContent = this._lineHasContent;
let content = this._content;
Expand All @@ -181,51 +182,11 @@ export class DOMTextScanner {
const char = StringUtil.readCodePointsForward(nodeValue, offset, 1);
offset += char.length;
const charAttributes = DOMTextScanner.getCharacterAttributes(char, preserveNewlines, preserveWhitespace);
/** @type {import('dom-text-scanner').SeekTextNoteDetails} */
const seekTextNoteDetails = {done, lineHasWhitespace, lineHasContent, content, offset, remainder, newlines};

if (charAttributes === 0) {
// Character should be ignored
continue;
} else if (charAttributes === 1) {
// Character is collapsible whitespace
lineHasWhitespace = true;
} else {
// Character should be added to the content
if (newlines > 0) {
if (content.length > 0) {
const useNewlineCount = Math.min(remainder, newlines);
content += '\n'.repeat(useNewlineCount);
remainder -= useNewlineCount;
newlines -= useNewlineCount;
} else {
newlines = 0;
}
lineHasContent = false;
lineHasWhitespace = false;
if (remainder <= 0) {
offset -= char.length; // Revert character offset
break;
}
}

lineHasContent = (charAttributes === 2); // 3 = character is a newline

if (lineHasWhitespace) {
if (lineHasContent) {
content += ' ';
lineHasWhitespace = false;
if (--remainder <= 0) {
offset -= char.length; // Revert character offset
break;
}
} else {
lineHasWhitespace = false;
}
}

content += char;

if (--remainder <= 0) { break; }
}
({done, lineHasWhitespace, lineHasContent, content, offset, remainder, newlines} = this._checkCharacterForward(char, charAttributes, seekTextNoteDetails));
if (done) { break; }
}

this._lineHasWhitespace = lineHasWhitespace;
Expand Down Expand Up @@ -256,6 +217,7 @@ export class DOMTextScanner {
const nodeValueLength = nodeValue.length;
const {preserveNewlines, preserveWhitespace} = this._getWhitespaceSettings(textNode);

let done = false;
let lineHasWhitespace = this._lineHasWhitespace;
let lineHasContent = this._lineHasContent;
let content = this._content;
Expand All @@ -268,50 +230,11 @@ export class DOMTextScanner {
offset -= char.length;
const charAttributes = DOMTextScanner.getCharacterAttributes(char, preserveNewlines, preserveWhitespace);

if (charAttributes === 0) {
// Character should be ignored
continue;
} else if (charAttributes === 1) {
// Character is collapsible whitespace
lineHasWhitespace = true;
} else {
// Character should be added to the content
if (newlines > 0) {
if (content.length > 0) {
const useNewlineCount = Math.min(remainder, newlines);
content = '\n'.repeat(useNewlineCount) + content;
remainder -= useNewlineCount;
newlines -= useNewlineCount;
} else {
newlines = 0;
}
lineHasContent = false;
lineHasWhitespace = false;
if (remainder <= 0) {
offset += char.length; // Revert character offset
break;
}
}

lineHasContent = (charAttributes === 2); // 3 = character is a newline
/** @type {import('dom-text-scanner').SeekTextNoteDetails} */
const seekTextNoteDetails = {done, lineHasWhitespace, lineHasContent, content, offset, remainder, newlines};

if (lineHasWhitespace) {
if (lineHasContent) {
content = ' ' + content;
lineHasWhitespace = false;
if (--remainder <= 0) {
offset += char.length; // Revert character offset
break;
}
} else {
lineHasWhitespace = false;
}
}

content = char + content;

if (--remainder <= 0) { break; }
}
({done, lineHasWhitespace, lineHasContent, content, offset, remainder, newlines} = this._checkCharacterBackward(char, charAttributes, seekTextNoteDetails));
Copy link

Choose a reason for hiding this comment

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

My comment is a bit late on this, but these sub-functions should probably just be modifying the seekTextNoteDetails value that is passed into it. As it stands right now, it's repeatedly constructing and destructuring a large structure in a high-bandwidth code-path, which is probably not great. Obviously this project doesn't have performance tests currently, so I don't have hard data to back this up, but I'd imagine that this is likely less efficient.

One of the reasons why this code path was originally somewhat messy was because it was a highly-used function. That's why it was doing the weird thing where it would assign all of the member fields to local variables first, and then restore them after: for (presumably) faster access in a loop. However, now that that effect is no longer being used, there is really no reason to do that, and instead the member fields should be used directly.

This is what it would look like:
https://github.com/toasted-nutbread/yomichan/commits/simplify-dom-text-scanner/

But I'm also not sure if there's a strong benefit of making that change vs just leaving the loop in the function. There's probably a lot of code locality and caching stuff that goes on in JS there, so it's hard to say what is the best way to do this in the long run without performance benchmarks.

if (done) { break; }
}

this._lineHasWhitespace = lineHasWhitespace;
Expand Down Expand Up @@ -350,6 +273,130 @@ export class DOMTextScanner {
return {preserveNewlines: false, preserveWhitespace: false};
}

/**
* @param {string} char
* @param {import('dom-text-scanner').CharacterAttributesEnum} charAttributes
* @param {import('dom-text-scanner').SeekTextNoteDetails} seekTextNoteDetails
* @returns {import('dom-text-scanner').SeekTextNoteDetails}
*/
_checkCharacterForward(char, charAttributes, seekTextNoteDetails) {
let {done, lineHasWhitespace, lineHasContent, content, offset, remainder, newlines} = seekTextNoteDetails;

switch (charAttributes) {
case 0:
break;
case 1:
lineHasWhitespace = true;
break;
case 2:
case 3:
if (newlines > 0) {
if (content.length > 0) {
const useNewlineCount = Math.min(remainder, newlines);
content += '\n'.repeat(useNewlineCount);
remainder -= useNewlineCount;
newlines -= useNewlineCount;
} else {
newlines = 0;
}
lineHasContent = false;
lineHasWhitespace = false;
if (remainder <= 0) {
offset -= char.length; // Revert character offset
done = true;
break;
}
}

lineHasContent = (charAttributes === 2); // 3 = character is a newline

if (lineHasWhitespace) {
if (lineHasContent) {
content += ' ';
lineHasWhitespace = false;
if (--remainder <= 0) {
offset -= char.length; // Revert character offset
done = true;
break;
}
} else {
lineHasWhitespace = false;
}
}

content += char;

if (--remainder <= 0) {
done = true;
break;
}
}

return {done, lineHasWhitespace, lineHasContent, content, offset, remainder, newlines};
}

/**
* @param {string} char
* @param {import('dom-text-scanner').CharacterAttributesEnum} charAttributes
* @param {import('dom-text-scanner').SeekTextNoteDetails} seekTextNoteDetails
* @returns {import('dom-text-scanner').SeekTextNoteDetails}
*/
_checkCharacterBackward(char, charAttributes, seekTextNoteDetails) {
let {done, lineHasWhitespace, lineHasContent, content, offset, remainder, newlines} = seekTextNoteDetails;

switch (charAttributes) {
case 0:
break;
case 1:
lineHasWhitespace = true;
break;
case 2:
case 3:
if (newlines > 0) {
if (content.length > 0) {
const useNewlineCount = Math.min(remainder, newlines);
content = '\n'.repeat(useNewlineCount) + content;
remainder -= useNewlineCount;
newlines -= useNewlineCount;
} else {
newlines = 0;
}
lineHasContent = false;
lineHasWhitespace = false;
if (remainder <= 0) {
offset += char.length; // Revert character offset
done = true;
break;
}
}

lineHasContent = (charAttributes === 2); // 3 = character is a newline

if (lineHasWhitespace) {
if (lineHasContent) {
content = ' ' + content;
lineHasWhitespace = false;
if (--remainder <= 0) {
offset += char.length; // Revert character offset
done = true;
break;
}
} else {
lineHasWhitespace = false;
}
}

content = char + content;

if (--remainder <= 0) {
done = true;
break;
}
}

return {done, lineHasWhitespace, lineHasContent, content, offset, remainder, newlines};
}

// Static helpers

/**
Expand Down Expand Up @@ -468,11 +515,7 @@ export class DOMTextScanner {
* @param {string} character A string containing a single character.
* @param {boolean} preserveNewlines Whether or not newlines should be preserved.
* @param {boolean} preserveWhitespace Whether or not whitespace should be preserved.
* @returns {number} An integer representing the attributes of the character.
* 0: Character should be ignored.
* 1: Character is collapsible whitespace.
* 2: Character should be added to the content.
* 3: Character should be added to the content and is a newline.
* @returns {import('dom-text-scanner').CharacterAttributesEnum} An enum representing the attributes of the character.
*/
static getCharacterAttributes(character, preserveNewlines, preserveWhitespace) {
switch (character.charCodeAt(0)) {
Expand Down
12 changes: 6 additions & 6 deletions ext/js/language/text-scanner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,8 @@ export class TextScanner extends EventDispatcher {
this._preventNextClick = false;
/** @type {boolean} */
this._preventScroll = false;
/** @type {0|1|2|3} */
this._penPointerState = 0; // 0 = not active; 1 = hovering; 2 = touching; 3 = hovering after touching
/** @type {import('text-scanner').PenPointerState} */
this._penPointerState = 0;
/** @type {Map<number, string>} */
this._pointerIdTypeMap = new Map();

Expand Down Expand Up @@ -1382,13 +1382,13 @@ export class TextScanner extends EventDispatcher {
return input.scanOnPenRelease;
}
switch (this._penPointerState) {
case 1: // hovering
case 1:
return input.scanOnPenHover;
case 2: // touching
case 2:
return input.scanOnPenMove;
case 3: // hovering after touching
case 3:
return input.scanOnPenReleaseHover;
default: // not active
case 0:
Casheeew marked this conversation as resolved.
Show resolved Hide resolved
return false;
}
}
Expand Down
9 changes: 5 additions & 4 deletions ext/js/pages/settings/dictionary-import-controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ export class DictionaryImportController {
};

let statusPrefix = '';
/** @type {import('dictionary-importer.js').ImportStep} */
let stepIndex = -2;
/** @type {import('dictionary-worker').ImportProgressCallback} */
const onProgress = (data) => {
Expand All @@ -178,8 +179,8 @@ export class DictionaryImportController {
for (const label of statusLabels) { label.textContent = statusString; }

switch (stepIndex2) {
case -2: // Initialize
case 5: // Data import
case -2:
case 5:
this._triggerStorageChanged();
break;
}
Expand Down Expand Up @@ -210,19 +211,19 @@ export class DictionaryImportController {
}

/**
* @param {number} stepIndex
* @param {import('dictionary-importer').ImportStep} stepIndex
* @returns {string}
*/
_getImportLabel(stepIndex) {
switch (stepIndex) {
case -2: return '';
case -1:
case 0: return 'Loading dictionary';
case 1: return 'Loading schemas';
case 2: return 'Validating data';
case 3: return 'Formatting data';
case 4: return 'Importing media';
case 5: return 'Importing data';
default: return '';
}
}

Expand Down
Loading