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

Add recommended dictionaries to welcome page #991

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

Conversation

Kuuuube
Copy link
Member

@Kuuuube Kuuuube commented May 23, 2024

Currently I've only added japanese dicts but other languages can easily be added into ext/data/recommended-dictionaries.json.

@Kuuuube Kuuuube added kind/enhancement The issue or PR is a new feature or request area/ui-ux The issue or PR is related to UI/UX/Design labels May 23, 2024
@jamesmaa
Copy link
Collaborator

Awesome work :nanayay:

Some UI/UX notes from your video demo:

  • If it's minimizing scope to keep it jp atm, let's make it clear it's japanese-only
  • Change "Import" to "Download"
  • I think the progress bar should be slightly thicker. It's actually easy to miss consider the area of interaction is a bit further away from the progress bar
  • What happens if a user clicks another import while an existing one is occurring?
  • It's unclear what these dictionaries are to the new user. What is CC100 and BCCWJ? Maybe adding (?) or tooltips would be helpful here
  • Is a toaster on the bottom here?
image

@Kuuuube
Copy link
Member Author

Kuuuube commented May 23, 2024

If it's minimizing scope to keep it jp atm, let's make it clear it's japanese-only

I don't intend to add in other language dicts in this pr unless someone else wants to compile a list to add on but I'll be making it extensible so they can just be dropped in

I think the progress bar should be slightly thicker. It's actually easy to miss consider the area of interaction is a bit further away from the progress bar

It's the same progress bar that is used for normal dictionary imports

What happens if a user clicks another import while an existing one is occurring?

The import events are queued and will execute in the order the user clicked on them

Only problem with this at the moment is there is no indicator for how many dicts are being imported since it's calling single dict imports one after another

Is a toaster on the bottom here?

It's the same footer thing we use for normal dict imports.

@Kuuuube
Copy link
Member Author

Kuuuube commented May 23, 2024

Mostly done here just needs some finishing touches.

  1. Types/schema for the recommended dicts json
  2. Make dictionary import progress show all importing dicts not just the current one
  3. Dictionary hashes in the json and hash checking before importing

Also open to any feedback ofc

@stephenmk
Copy link

I have to update my website every week to change the download links to point to the latest dictionary versions. I wouldn't mind maintaining some sort of manifest file on my website with links to the latest version as well. For example I could have something like jitendex.org/latest.json, which could be fetched by yomitan. I think that would be better than having a direct link to a specific old version hardcoded in yomitan.

Going one step further, it would be neat if dictionaries could include links to these sorts of manifest files within the index.json metadata. Yomitan could use that information to check for updates even if the dictionary isn't included in the officially distributed recommended-dictionaries.json file.

@Kuuuube
Copy link
Member Author

Kuuuube commented Jun 20, 2024

@stephenmk would it be possible for you to make a static url that always pulls the latest jitexdex? Due to your release asset names changing every release (they include a date) it isnt possible to pull latest straight off github like that.

I'd like to push this pr along even if it isn't entirely perfect. And I don't think theres too much need to pin dict versions to yomitan releases. New users aren't likely to be pulling down old versions of the extension so I don't think users downloading incompatible dicts is much of a worry.

@stephenmk
Copy link

Maybe this is outside the scope of what you wanted to do right now, but the main issue I wanted to address with my "manifest" idea was the problem of upgrading. If I make a static url that always points to the latest yomitan ZIP file of jitendex, then there's no way for users to know that the ZIP file has been updated without making yomitan redownload the whole file, extracting it, checking the index.json file, etc.

If, instead, there was a small JSON file containing jitendex metadata that yomitan can fetch, then it could alert users when new versions are available. Yomitan could then give users the option to update.

For example, it might look something like this.

jitendex.org/yomitan_latest.json

{
    "name": "Jitendex",
    "version": "24.06.19.0",
    "minimum-compatible-yomitan-version": "24.06.18.0",
    "icon": "https://jitendex.org/favicon.ico",
    "update-url": "https://github.com/stephenmk/stephenmk.github.io/releases/download/v4.7-9/jitendex-yomitan-2024-06-19.zip",
    "sha256sum": "0ea49da53e23e3cd1c2b525754f2df25a9995f1ede4077e5c3e03561b52cf950",
    "notes": "Information about updated content could go here."
}

@StefanVukovic99
Copy link
Member

StefanVukovic99 commented Jun 20, 2024

I think that's the right approach. For simplicity I'd have the metadata json file be the index.json, since it's for storing metadata. To start with, I'd add to the index.json schema a boolean like isUpdatable, and if it is set, require an index_url and a download_url, both pointing to stable links for the latest version. We could then add to the index, as needed, the checksum/yomi version etc.

@Kuuuube
Copy link
Member Author

Kuuuube commented Jul 1, 2024

@stephenmk I agree with your idea for updating but I don't plan on adding anything for updating dicts in this PR. Like stefan mentioned I think it would be best to have this info in the index.json so anybody that downloads the dict from any source gets that update information.

With a static download url users who installed that way from the recommended dicts section will be able to update the dict exactly the same.

I'd rather not hold up this pr on adding update functionality or even deciding on the correct format for providing update information.

At the moment the alternative I see is to provide JMdict which does have a static link. I would like to use jitendex here but if we need to hold off from doing that until updating support is added, that's fine. I think it's better to use JMdict over potentially outdated versions of jitendex from having to use a link that does not get updated (or only gets updated when yomitan updates). I see the accuracy of the dictionary data provided is more important than the fancy formatting of jitendex. Especially considering we're putting this right in the welcome page of yomitan as a recommendation to install.

@Kuuuube
Copy link
Member Author

Kuuuube commented Jul 2, 2024

It is also possible to provide update functionality for approved dictionaries after they have been downloaded without update information.

It cant be 100% seamless like having the update information directly in the dict (due to security concerns we will need users to verify they want to download from the sources we provide for dicts) but this will be needed for jmdict, jmnedict, kanjidic, and maybe others anyways.

@stephenmk
Copy link

I removed the date from the filename of latest version of the jitendex. It should always be 'jitendex-yomitan.zip' from now on.

@Kuuuube
Copy link
Member Author

Kuuuube commented Jul 4, 2024

Thank you!

@stephenmk
Copy link

stephenmk commented Jul 4, 2024

You'll need to modify the link a little to remove the tag:
https://github.com/stephenmk/stephenmk.github.io/releases/latest/download/jitendex-yomitan.zip

Edit: I see you beat me to it.

@Kuuuube Kuuuube marked this pull request as ready for review July 6, 2024 16:54
@Kuuuube Kuuuube requested a review from a team as a code owner July 6, 2024 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ui-ux The issue or PR is related to UI/UX/Design 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.

None yet

4 participants