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 facility to extract and set ZIM metadata #1133

Merged
merged 6 commits into from
Oct 17, 2023

Conversation

Jaifroid
Copy link
Member

This backports a facility in Kiwix JS PWA.

@Jaifroid Jaifroid added this to the v4.0 milestone Oct 16, 2023
@Jaifroid Jaifroid self-assigned this Oct 16, 2023
@Jaifroid
Copy link
Member Author

Tested and working on Firefox OS, the only implementation currently using the dropdown list of archives.

@Jaifroid
Copy link
Member Author

Properly fixes #395. Also provides dev info for #886.

@Jaifroid
Copy link
Member Author

@Rishabhg71 I'd like you to check over this PR if you have a chance. It adds the most important (and actually used) metadata that are contained in the M/ namespace (see https://openzim.org/wiki/Metadata) to the ZIMArchive object.

It extracts the metadata in two phases. This is because it's a slightly costly operation at least on old, slow browsers, as it's actually extracting a series of BLOBs, albeit a very small string in each case. First are the time-critical metadata that are/may be needed on ZIM archive load. Then, in the background, I add the principal remaining items (actually used ones, not all those listed in the above spec). If a Dev needs others, they can use the extraction function manually when needed.

The reason for this PR is that there are some items that I need early access to in kiwix-js-pwa, in particular, I need to be able to detect the ZIM file Creator at load time. I also add the language here because in the future the language could well determine automatically setting the RTL or LTR of the UI, or even auto-selecting UI language (as an option).

None of the rest are likely to be time-critical, but if any are, they can be added in the appropriate background loading section.

Once the principal metadata are loaded, the object will be displayed in the console.debug log, which is useful for developers, and also for the curious.

We could think of adding a display of archive metadata in the UI, as an expandable list hidden under a small down arrow next to the loaded archive name (in Configuration) in the future, but for now, displaying it in console.debug seems sufficient.

@Rishabhg71
Copy link
Collaborator

@Jaifroid, I've read through the issues and what you've done, but I'm not quite sure what's expected from me. Can you clarify? 😅👍

@Jaifroid
Copy link
Member Author

It's just good to get a second opinion on PRs. I prefer a PR to be reviewed if possible. A second pair of eyes can sometimes spot issues or else see a better way of organizing code. If you don't see any issues, then I'd appreciate if you could approve the PR.

@Rishabhg71
Copy link
Collaborator

Rishabhg71 commented Oct 17, 2023

I've reviewed the code, and it makes sense to me (though I'm not an expert). However, it got me thinking: why aren't we using TypeScript and object-oriented programming (OOP) along with design patterns?

Regarding Rollup, wouldn't it handle browser compatibility if we use typescript? Our code seems more like a "script" than an app (the JavaScript irony 😄).

After working on this project for a month, I feel the developer experience could be improved. Would you be interested in a refactor after I complete my tasks? I'd love to work on that.

@Jaifroid
Copy link
Member Author

Thanks @Rishabhg71! I always think of review on GitHub as peer review, even if the reviewer is not an expert. It's useful to have someone else sanity check some things.

Regarding typescript, it could certainly be done, and all the work I put into introducing a modern build system was heading in that direction. So it's certainly a possibility. The only thing to consider is not making the entry level too hard for new contributors. We can talk more about this! JavaScript is a subset of TrypeScript in any case.

@Jaifroid Jaifroid merged commit 108f118 into main Oct 17, 2023
9 checks passed
@Jaifroid Jaifroid deleted the Add-facility-to-extract-and-set-ZIM-metadata branch October 17, 2023 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants