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 similar Kanji list by WK level #23

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

Conversation

marciska
Copy link

Addresses feature request #22.
Similar Kanji are now sorted in ascending WK level order.

Before change:
Unsorted Kanji List

After change:
Sorted Kanji List

@marciska
Copy link
Author

marciska commented Jan 13, 2023

wk_niai.user.js has to be modified as well. I am not sure why the files are fixed to only certain commits, and not based on the master branch. I'll leave it for now, but note that for the above changes to work the file must be modified by the new commit url.

TLDR - Change line 36 in wk_niai.user.js to:
https://raw.githubusercontent.com/mwil/wanikani-userscripts/805c305f04c2c9fb492ebb30bc54235f05d84c03/wanikani-similar-kanji/wk_niai.main.js
Or to:
https://raw.githubusercontent.com/mwil/wanikani-userscripts/master/wanikani-similar-kanji/wk_niai.main.js

@Sinyaven
Copy link
Collaborator

I am not sure why the files are fixed to only certain commits, and not based on the master branch.

The default setting for Tampermonkey is to never update external files, so using @require https://raw.githubusercontent.com/mwil/wanikani-userscripts/master/wanikani-similar-kanji/wk_niai.main.js would result in Tampermonkey downloading this file once the script is executed for the first time, but then it would stay on this version of the file forever.

Another advantage of @requireing a specific version: if you install an older version of the script (e.g. if a newer version introduced a bug), the script will use the appropriate older versions of the @required files.

@marciska
Copy link
Author

I made a few changes to address the concerns mentioned here:
Rather than by level, the kanjis are now sorted by their similarity score. Kanji's not yet unlocked are shown last, but also among their group they are sorted by score as well.

@lupomikti
Copy link
Contributor

lupomikti commented Dec 3, 2023

I took a look at this because I wanted to have it implemented for myself in my own fork, so I thought I'd share the issues I discovered.

First up, the sorting by score doesn't actually work. This is because the sort function assumes the object returned by getInfo from NiaiDB has a property of score but this is not the case, so this.ndb.getInfo(kanjiA).score returns undefined. The only function in NiaiDB that works with similarity scores is getSimilar. After giving it some thought I figured the easiest way to address this is to change getSimilar so that it returns an array of objects with kanji and score properties (or an array of arrays with two elements, one the kanji and one the score), pass that to the sorting function and adjust subsequent elements of the code to comply with this change. Thought about it a little more: getSimilar already does the sorting by score and has access to the locked property info, so instead of making a separate sort function in main.js just edit getSimilar to also sort by locked property.

Unfortunately this is not the only problem. A more difficult to solve issue is that the lookup database is out of date with Wanikani and thus the levels used in the isKanjiLocked function can be incorrect leading to a kanji being sorted into the locked section when it should be unlocked (and then subsequently styled as unlocked by the update_wk_cache function, which is called after the list items are joined together but without an await so it actually doesn't run until after the section and chargrid have been injected into the page).

To give a concrete example of that: I am currently level 21, and unlocked the kanji 制. One of the similar kanji that are returned in the list is 劇. In the lookup database, 劇 is listed as level 34, but it is currently level 17 on Wanikani. The sort function puts 劇 into the part of the list with all of the locked items because 34 > 21, but it does not have the styling of a locked item because its class is updated by the function that uses the Open Framework to update the locked classes of items.

My first thought to fix this was that I would need getSimilar to return not just the kanji and score, but the level from the lookup database too, and then to pass the result into a function that would update all of the levels using data fetched via Open Framework, and then all of that can be passed to the sort function. However, this would require an async call and thus marking the containing function populateNiaiSection as async and that would propagate upwards and just generally cause a big enough mess that significant portions of this would need to be rewritten to be asynchronous. While looking into this I also learned about the Schwartzian transform and thought it would be nice if there were a getLevelFromWK function that takes the kanji as input and which could be used in such an algorithm for the sorting, but this too runs into the issue that it would have to be async.

In any case, since my current situation is having my own fork of Niai to use that already has significant changes that would not really make for an appropriate PR (see: niai-revamped branch on my fork), I'll continue working on my own solution ideas even if they require major rewrites; I just thought I should share these issues so if you want you can work on them in your own way.

Update: I was able to figure out how to address the second issue and make changes such that it wasn't that major of a rewrite. I will make a PR to your fork so those changes can be brought into this PR.

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