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

"isJapanese" check move #730

Merged
merged 15 commits into from
Feb 25, 2024

Conversation

toasted-nutbread
Copy link

Resolves #717.

This change generalizes how languages are checked to see if a string "may be translatable". This now supports additional languages, although it is only explicitly set up for Japanese at this point.

There are two functions worth noting:

  • getLanguageFromText is used to determine which language a string "might" be using. This is only used for assigning a lang attribute on the generated content.
  • textMayBeTranslatable is used to determine if the text might be translatable into that language. This is used to filter out things for the clipboard monitor as well as a lesser-used edge case of determining whether or not image alt text, button text, etc. should show the query parser in the standard search popup.

Copy link

codspeed-hq bot commented Feb 25, 2024

CodSpeed Performance Report

Merging #730 will not alter performance

Comparing toasted-nutbread:is-japanese-check-move (3f3cdc3) with master (73169f0)

Summary

✅ 4 untouched benchmarks

Copy link

github-actions bot commented Feb 25, 2024

✔️ No visual differences introduced by this PR.

View Playwright Report (note: open the "playwright-report" artifact)

Copy link
Member

@StefanVukovic99 StefanVukovic99 left a comment

Choose a reason for hiding this comment

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

Amazing, thank you 🙏

@djahandarie
Copy link
Collaborator

Two naming comments:

  1. The first is kind of unrelated to the change in this PR, but I dislike the use of the word "translator"/"translatable" etc that gets used in Yomitan. I think we could rename it something like "lookup"/"lookupable", maybe.

  2. Also maybe more related to this PR, I'm not sure whether "-able" is actually the right suffix at all, as it is possible to look stuff up even if it doesn't have the target language in it or not (like TKG and other Japanese words written in the English alphabet). I think the concept is closer to "lookup-worthy" -- i.e., a somewhat subjective threshold whether it's worth doing the lookup or not.

@toasted-nutbread
Copy link
Author

1. The first is kind of unrelated to the change in this PR, but I dislike the use of the word "translator"/"translatable" etc that gets used in Yomitan. I think we could rename it something like "lookup"/"lookupable", maybe.

Agree, it's not really doing a true translation. Part of the reason why Translator has its name is because legacy, but I agree that it's a misnomer.

2. Also maybe more related to this PR, I'm not sure whether "-**able**" is actually the right suffix at all, as it is _possible_ to look stuff up even if it doesn't have the target language in it or not (like TKG and other Japanese words written in the English alphabet). I think the concept is closer to "lookup-**worthy**" -- i.e., a somewhat subjective threshold whether it's worth doing the lookup or not.

Updated this to isTextLookupWorthy.

@djahandarie djahandarie added this pull request to the merge queue Feb 25, 2024
Merged via the queue into themoeway:master with commit 2e9ea19 Feb 25, 2024
10 checks passed
@Casheeew Casheeew added the kind/meta The issue or PR is meta label Feb 27, 2024
@Casheeew Casheeew added the area/linguistics The issue or PR is related to linguistics label Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/linguistics The issue or PR is related to linguistics kind/meta The issue or PR is meta
Projects
None yet
Development

Successfully merging this pull request may close these issues.

disable isStringPartiallyJapanese on other languages
4 participants