Skip to content

support updatable dictionaries - #1174

Merged
StefanVukovic99 merged 22 commits into
yomidevs:masterfrom
StefanVukovic99:updatable
Jul 19, 2024
Merged

StefanVukovic99 merged 22 commits into
yomidevs:masterfrom
StefanVukovic99:updatable

Conversation

@StefanVukovic99

Copy link
Copy Markdown
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
Comment thread ext/js/pages/settings/dictionary-controller.js Fixed
Comment thread ext/js/pages/settings/dictionary-controller.js Fixed
@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
Copy Markdown
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
Copy Markdown
Collaborator

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

@MarvNC

MarvNC commented Jul 11, 2024

Copy link
Copy Markdown
Member

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.

@jamesmaa jamesmaa left a comment

Copy link
Copy Markdown
Collaborator

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

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

await this._deleteDictionary(dictionaryTitle);

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread ext/settings.html
Comment thread ext/js/pages/settings/dictionary-controller.js
Comment thread ext/js/pages/settings/dictionary-controller.js Outdated
@StefanVukovic99

Copy link
Copy Markdown
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
Copy Markdown

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

Yeah I'll try to get to this today

@MarvNC MarvNC left a comment •

Copy link
Copy Markdown
Member

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:

🥺

Comment thread ext/data/schemas/dictionary-index-schema.json
Comment thread ext/js/dictionary/dictionary-importer.js
Comment thread ext/js/pages/settings/dictionary-controller.js
@stephenmk

Copy link
Copy Markdown

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
Copy Markdown
Member Author

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

Comment thread ext/js/pages/settings/dictionary-controller.js
@StefanVukovic99
StefanVukovic99 marked this pull request as draft July 15, 2024 16:55
@djahandarie

djahandarie commented Jul 17, 2024 •

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
Member Author

Everything mentioned should be resolved now.

@jamesmaa jamesmaa left a comment

Copy link
Copy Markdown
Collaborator

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 yomidevs:master with commit e268de9 Jul 19, 2024
@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.

7 participants