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

support updatable dictionaries #1174

Merged
merged 22 commits into from
Jul 19, 2024

Conversation

StefanVukovic99
Copy link
Member

early draft

@StefanVukovic99 StefanVukovic99 added kind/enhancement The issue or PR is a new feature or request area/dictionary-format The issue or PR is related to dictionary formatting labels Jul 3, 2024
@StefanVukovic99 StefanVukovic99 marked this pull request as ready for review July 6, 2024 19:22
@StefanVukovic99 StefanVukovic99 requested a review from a team as a code owner July 6, 2024 19:22
@StefanVukovic99
Copy link
Member Author

This pretty much works now. You can try loading this dict:
kty-sq-de.zip
and search for 'Afrika'. The gloss should contain the text (before update). Then, in the dictionaries modal, click Check for Updates. The button should change its text to 1 updates and a blue badge button should appear next to the dictionary in the list. Clicking the badge should prompt you to update the dictionary. Then search Afrika again and it should be updated.

@jamesmaa
Copy link
Collaborator

@stephenmk can you PTAL as a potential user of this?

@MarvNC
Copy link
Member

MarvNC commented Jul 11, 2024

Tried this out, worked great.

A few things:

  • Does it work if the dictionary name is updated? Looking at the code for _updateDictionary it seems it should be fine, but just making sure.
  • Could you consider an "update all" button after clicking "check for updates"? It seems the current way is to update after clicking the ? icon on each dictionary individually which seems a bit less intuitive imo, though it's a good way to provider the user with choice of which dictionaries to update.
  • Could the updatable status of the dictionary maybe be displayed in the dictionary's details (or maybe as some sort of icon in the dictionaries list)? Small thing but it'd provide some clarity that it's an updatable dictionary.

Copy link
Collaborator

@jamesmaa jamesmaa left a 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

ext/data/schemas/dictionary-index-schema.json Show resolved Hide resolved
ext/js/pages/settings/dictionary-controller.js Outdated Show resolved Hide resolved
ext/js/pages/settings/dictionary-controller.js Outdated Show resolved Hide resolved
const {downloadUrl} = dictionaryInfo;
if (typeof downloadUrl !== 'string') { throw new Error('Attempted to update dictionary without download URL'); }

await this._deleteDictionary(dictionaryTitle);
Copy link
Collaborator

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?

Copy link
Member Author

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.

Copy link
Member

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.

ext/settings.html Show resolved Hide resolved
@StefanVukovic99
Copy link
Member Author

  • Does it work if the dictionary name is updated?

Yes, I think it should.

  • Could you consider an "update all" button after clicking "check for updates"?

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.

  • Could the updatable status of the dictionary maybe be displayed in the dictionary's details?

Yeah, I couldn't think of any good icons to represent this, and I'm not very good at icons anyway so also backburner

@stephenmk
Copy link

@stephenmk can you PTAL as a potential user of this?

Yeah I'll try to get to this today

Copy link
Member

@MarvNC MarvNC left a 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:

🥺

@stephenmk
Copy link

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.

Video
2024-07-12.12-34-44.mp4

@StefanVukovic99
Copy link
Member Author

Yeah, it shares that issue with the general downloading from URL - #1027 (comment)
Maybe we should find a way to fix that 🤔

@StefanVukovic99 StefanVukovic99 marked this pull request as draft July 15, 2024 16:55
@djahandarie
Copy link
Collaborator

djahandarie commented Jul 17, 2024

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. 🤔

@StefanVukovic99 StefanVukovic99 marked this pull request as ready for review July 19, 2024 16:49
@StefanVukovic99
Copy link
Member Author

Everything mentioned should be resolved now.

Copy link
Collaborator

@jamesmaa jamesmaa left a 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

@StefanVukovic99 StefanVukovic99 added this pull request to the merge queue Jul 19, 2024
Merged via the queue into themoeway:master with commit e268de9 Jul 19, 2024
11 checks passed
@StefanVukovic99 StefanVukovic99 deleted the updatable branch July 19, 2024 19:14
MarvNC added a commit to MarvNC/yomichan-dict-builder that referenced this pull request Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dictionary-format The issue or PR is related to dictionary formatting kind/enhancement The issue or PR is a new feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants