-
Notifications
You must be signed in to change notification settings - Fork 112
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
support updatable dictionaries #1174
Conversation
This pretty much works now. You can try loading this dict: |
@stephenmk can you PTAL as a potential user of this? |
Tried this out, worked great. A few things:
|
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.
- Add documentation to
docs/making-yomitan-dictionaries.md
Just some comments, haven't tested it yet
const {downloadUrl} = dictionaryInfo; | ||
if (typeof downloadUrl !== 'string') { throw new Error('Attempted to update dictionary without download URL'); } | ||
|
||
await this._deleteDictionary(dictionaryTitle); |
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 suppose this is a failure case if deleteDictionary
partially fails here. What are the mechanisms to maintain data consistency here?
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.
Worst case scenario for this feature is delete works, then import fails and you're left with no dict. Preventing that would be difficult.
Deleting itself hasn't been known to fail, I think.
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.
Deleting certainly can fail but for some reason it tends to be less common (maybe because you cant do it in bulk unless you delete all). One way around this would be storing dict metadata somewhere in the database and only deleting that after the import succeeds. So there could be an easy way to recover and maybe the user could press a button to start the import.
Yes, I think it should.
I've considered it, but I was thinking of adding that in a later PR. Updating all would take a looong time, might fail midway, and there won't be many updates soon anyhow, so I'd wait and see how this fares.
Yeah, I couldn't think of any good icons to represent this, and I'm not very good at icons anyway so also backburner |
Yeah I'll try to get to this today |
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.
maybe getting downloadUrl from the json on indexUrl would in fact be better :think:
🥺
During the updating process, there's an awkward part between the deletion of the old dictionary and installation of the new dictionary. My internet speed is slow, so it takes a minute to download a 25MB file. While it's downloading, all of the yomitan dictionary import buttons become re-enabled and my first impression was that the process had failed somehow. This is shown at around the 1:20 mark in the video below. Video2024-07-12.12-34-44.mp4 |
Yeah, it shares that issue with the general downloading from URL - #1027 (comment) |
Added a related comment about potentially requiring artifact attestation here though I guess it wouldn't make sense to apply that to all dictionaries as that bar might be a little too high, maybe more just for the ones we package into Yomitan. 🤔 |
Everything mentioned should be resolved now. |
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.
This LGTM. I'm sure there are going to be rough edges but they'll be easily fixable as we come across them
early draft