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

Sort entries by primary reading #1497

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

khaitruong922
Copy link

@khaitruong922 khaitruong922 commented Oct 17, 2024

  • Add primary_reading param.
  • When click on gloss link, set furigana as primary_reading if exists.
  • Entries with reading matches primary_reading will be sorted to top.

Works correctly for Jitendex gloss link format. Don't know if there are any kind of dictionary format which will break this feature, since the reading is depended on the classes naming.

@khaitruong922 khaitruong922 changed the title Search term by exact reading when clicking on gloss link Search term by exact reading when clicking on gloss link with furigana Oct 17, 2024
@khaitruong922 khaitruong922 marked this pull request as ready for review October 17, 2024 14:13
@khaitruong922 khaitruong922 requested review from a team as code owners October 17, 2024 14:13
@khaitruong922 khaitruong922 changed the title Search term by exact reading when clicking on gloss link with furigana Sort term by exact reading when clicking on gloss link with furigana Oct 17, 2024
Copy link

codspeed-hq bot commented Oct 17, 2024

CodSpeed Performance Report

Merging #1497 will not alter performance

Comparing khaitruong922:search-reading (521cb1e) with master (9c5f824)

Summary

✅ 2 untouched benchmarks

🆕 1 new benchmarks
⁉️ 1 dropped benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark master khaitruong922:search-reading Change
⁉️ Translator.prototype.findTerms - (n=43) 154.8 ms N/A N/A
🆕 Translator.prototype.findTerms - (n=45) N/A 157.8 ms N/A

@khaitruong922
Copy link
Author

khaitruong922 commented Oct 17, 2024

Ready for review

  • Extract furigana from gloss link and add primary_reading search param

image
image

  • Sort entries which match primary_reading to top

image

image

@khaitruong922 khaitruong922 changed the title Sort term by exact reading when clicking on gloss link with furigana Sort term by primary reading when clicking on gloss link with furigana Oct 17, 2024
@khaitruong922 khaitruong922 changed the title Sort term by primary reading when clicking on gloss link with furigana Sort entries by primary reading when clicking on gloss link with furigana Oct 17, 2024
@stephenmk
Copy link

Thank you for working on this. It looks good.

Works correctly for Jitendex gloss link format. Don't know if there are any kind of dictionary format which will break this feature, since the reading is depended on the classes naming.

I don't like this approach. I think it would be better to rely on the dictionary data to specify the ?primary_reading= parameter. I don't think it's a good idea to try to parse the primary reading from the furigana. This increases the code complexity and could lead to unexpected behavior. It's possible that someone may want to manually specify a ?primary_reading= parameter that is not equivalent to the furigana readings within the link.

In other words, I would remove this block of code:

if (internal) {
let query = '';
if (href.length > 1) {
let hasFurigana = false;
let reading = '';
for (const childNode of text.childNodes) {
if (childNode instanceof HTMLElement) {
const furigana = childNode.querySelector('.gloss-sc-rt')?.textContent;
if (furigana && furigana.length > 0) {
reading += furigana;
hasFurigana = true;
}
} else {
reading += childNode.textContent ?? '';
}
}
query = href;
if (reading.length > 0 && hasFurigana) {
query += `&primary_reading=${reading}`;
}
}
href = `${location.protocol}//${location.host}/search.html${query}`;
}

@Kuuuube
Copy link
Member

Kuuuube commented Oct 18, 2024

I think it would be better to rely on the dictionary data to specify the ?primary_reading= parameter.

Agreed.

@khaitruong922
Copy link
Author

The only place that user could specify the primary_reading is the search page, by modifying the url. I don't think this is the main use case, and no user will be aware of it.

The main purpose of this PR is to extract the reading from furigana, then sort the terms with that reading on top. For example, suppose that my dictionary has 縁 (えん) above 縁 (ふち). When I click on a word link 縁 (ふち), the entry 縁 (ふち) should be above 縁 (えん).

As far as I know, only Jitendex has furigana for word link. I agree that the implementation is quite fragile, we can find better ways to handle this.

@khaitruong922
Copy link
Author

I also agree with having the primary_reading specified inside the href field of Jitendex, then we can remove the extract furigana code.

@khaitruong922 khaitruong922 changed the title Sort entries by primary reading when clicking on gloss link with furigana Sort entries by primary reading Oct 18, 2024
@stephenmk
Copy link

I just published a new version of Jitendex that includes the primary_reading parameter in hyperlinks.

@khaitruong922
Copy link
Author

Tested with new version of Jitendex. Works great!

My.Video.mp4

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