-
-
Notifications
You must be signed in to change notification settings - Fork 144
feat(dictionary): cache tokens in IndexedDB #844
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
base: main
Are you sure you want to change the base?
Conversation
842fba2 to
78bad00
Compare
78bad00 to
ef0283a
Compare
killergerbah
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey really appreciate the patience. I'm still trying to read and understand everything, so I've left some high level feedback for now. Let me know what you think.
372ab82 to
c07bd96
Compare
c07bd96 to
fae3e2c
Compare
killergerbah
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey thanks again for the patience. I did another pass and mostly made suggestions to improve readability. I know some of this is probably just stylistic difference, but there's a lot of code with complex logic which I think can be made more readable with low cost. Mostly worried about needing to understand this code when making changes months from now.
| .map((track) => `#${track + 1}`) | ||
| .join(', '), | ||
| })}`; | ||
| msg += ` | ${t('settings.dictionaryBuildModifiedCards', { numCards: numUpdatedCards.toLocaleString('en-US') })}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can localization of the progress strings be confined to the dictionary settings form instead? This would require you to model the progress/errors via a data structure but I think this is probably not so difficult as there's a limited number of types of status updates.
While the refactor might be kind of annoying, keeping the front-end and logical concerns separate as a matter of principle will will give us more flexibility to render the progress in different ways on the settings form, without a dependency on this code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That very flexibility was what I was trying to avoid. Some messages only makes sense to be displayed in certain states and constructing that seems like it would be complicated. Also some messages don't have translations if it's a random error. We also would need to handle translations in SubtitleColoring as it can log errors whenever tokens updates.
I still think it's worth doing though. I'm thinking of returning an object instead where the keys correspond to the translation key and values to the translation params. As well as a separate field for unspecified errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you provide both English string + the translation key, you could use the English string for logging in SubtitleColoring. The translation key could be used for display on the settings form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But instead of translation key, maybe provide an an enum? That will give us some decoupling of the progress/error type from the loc file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't get a design that I'm happy with so I think it's best if you make the changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed, let me know if anything looks weird to you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good, the only issue I noticed is that the elapsed time changes when a build is done and the button is pressed. I think this lies in how the UI is calculating it but it's not a big deal.
I also re-review the rest of the PR as well. Earlier your comment about dictionaryProvider was accurate, we could use DictionaryStorage directly. I'm not sure if it's worth updating though. I could see it being useful for statistics.
This eliminates the runtime dependency of Anki (though if it's available, it will be used to automatically keep cache in sync). This gives a massive speedup since we no longer need to query Anki as well as Yomitan for the Anki cards (especially sentence cards). Support for local tokens is not exposed to the user in this PR.
DictionaryProvider(works the same asSettingsProvider).meta: Use for tracking the current and previous builds to know if settings changed or build is in processtokens: Stores each word with their lemmas status, etcankiCard: Anki tokens derive their status from this store as we need to track other things like suspended statuscards > modifiedAt. This means we will only trigger a cache build once for a given card during a playback session (though subsequent card triggers will update everything). This covers the main case of collecting a token but tracking more is too inefficient.buildIdso concurrent builds are prevented even in case of shutdowns with no cleanup. Build will expire when completed or if more than 5 minutes has passed since the last update (every few seconds).SubtitlePlayer, it triggers a cache build immediately. This already existed if it was initiated from the extension (such as keybind).AlwaysandNeverwon't trigger cache builds for that track (unless coloring is enabled).SubtitleColoringwill now only reset the cache on settings that affect it, rather than all. It will now also clear the richText that's currently being displayed if the user turns off the features as we rely on it being enabled to update it.Local > Anki word > Anki sentence