Implement new Wikimedia mobile end-point support/render #1903

VadimKovalenkoSNF · 2023-09-11T14:09:12Z

Fixes #1664
Fixes: #1916
Blocked by #1925

codecov · 2023-09-13T16:58:15Z

Codecov Report

Attention: 22 lines in your changes are missing coverage. Please review.

Comparison is base (6b51e74) 74.60% compared to head (d412ee4) 76.14%.
Report is 18 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1903      +/-   ##
==========================================
+ Coverage   74.60%   76.14%   +1.53%     
==========================================
  Files          33       37       +4     
  Lines        2812     2985     +173     
  Branches      626      653      +27     
==========================================
+ Hits         2098     2273     +175     
+ Misses        618      617       -1     
+ Partials       96       95       -1

Files	Coverage Δ
src/MediaWiki.ts	`86.56% <100.00%> (+0.70%)`	⬆️
src/Templates.ts	`100.00% <100.00%> (ø)`
src/config.ts	`100.00% <ø> (ø)`
src/mwoffliner.lib.ts	`74.81% <100.00%> (ø)`
src/renderers/abstract.renderer.ts	`86.17% <100.00%> (-1.10%)`	⬇️
src/renderers/visual-editor.renderer.ts	`56.75% <100.00%> (+1.20%)`	⬆️
src/util/builders/url/desktop.director.ts	`100.00% <ø> (ø)`
src/util/builders/url/mobile.director.ts	`100.00% <100.00%> (ø)`
src/util/const.ts	`100.00% <100.00%> (ø)`
src/util/dump.ts	`74.19% <100.00%> (+1.77%)`	⬆️
... and 9 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

VadimKovalenkoSNF · 2023-09-15T14:15:03Z

This patch is not ready for the review yet. I found that article treatments are not working for mobile as I expected first. I pushed preliminary commit b7f6e55 to pass output from page/mobile-html directly to zim creator, but this could not be considered as a final solution - Kiwix app doesn't render it correctly though in local Kiwix server I can see exact same response as I get from page/mobile-html endpoint. There is another approach which might be better - instead of page/mobile-html, use page/html and process sections using scripts from deprecated MCS but inside mwoffliner. This will allow to preserve functionality that has been working before but without mobile-sections endpoint. On the other hand, it might require additional effort to apply it inside mwoffliner.

VadimKovalenkoSNF · 2023-09-19T14:06:57Z

Current implementation of mobile-html render has additional treatments:

Connect mobile js and css that are required for mobile-html.
Remove Edit icon near each section.
Restore lazy load images. Current mobile-html output returns span with a lazy-load placeholder that is not transforming into the image on scroll in Kiwix app.
Restore sections' appearance.
Disable event listeners for sup elements. This can be achieved only on the client side, so I added an additional script tag with this functionality to the output.
Added additional styles to override redundant or invalid CSS rules to make final output more robust.

kelson42

Architecture looks good but a few details to fix to keep things lean.
Important now, is to get all the tests passing so we can make the final review.

src/Downloader.ts

src/MediaWiki.ts

src/renderers/renderer.builder.ts

src/util/saveArticles.ts

VadimKovalenkoSNF · 2023-09-22T08:48:24Z

Set this to the review. To secure tests I explicitly set forceRender: 'WikimediaDesktop' where it was necessary. Once #1898 is approved, we can start to test mobile treatments along with other functionality without duplicating test files, etc.

kelson42 · 2023-09-22T08:56:55Z

@VadimKovalenkoSNF Great, do we have anything know not working fine specificily with this new end-point and which would need further work (and an other ticket)?

VadimKovalenkoSNF · 2023-09-22T09:32:01Z

@kelson42 collapsible sections not working (don't mismatch this with collapsible infoboxes) + I want you to look at the HTML output and give me feedback about it ( mostly form a user perspective ). Note that page/mobile-html is different than page/html but I've tried to apply transformations to elements that are not valid for the output.

VadimKovalenkoSNF · 2023-09-22T11:29:03Z

I noticed that images in gallery box don't render and nopic parameter doesn't work as expected.

kelson42 · 2023-09-22T11:47:35Z

This is essential that both bugs are fixed

VadimKovalenkoSNF · 2023-09-26T14:26:25Z

@kelson42 , I created a separate test file test/e2e/mobileRenderIntegrity.test.ts to check zim integrity for mobile renderer. It looks like I still got error about external pcs script there. Steps to reproduce:

Download this patch and run:
npm run test:pattern 'test/e2e/mobileRenderIntegrity.test.ts'

Output that I received:

[ERROR] Invalid external links found:
https://meta.wikimedia.org/api/rest_v1/data/javascript/mobile/pcs is an external dependence in article A/Canada

And the error about invalid internal links:

[ERROR] Invalid internal links found:
The following links:
- ../-/File:"O_Canada",_performed_by_the_United_States_Third_Marine_Aircraft_Wing_Band.oga-bg.vtt
(-/File:"O_Canada",_performed_by_the_United_States_Third_Marine_Aircraft_Wing_Band.oga-bg.vtt) were not found in article A/Canada

I would appreciate your input about how to solve this. Is there any effort required for zimcheck itself to fix this. Or is this only a mwoffliner issue? Link rels seems to work as expected. List of external resources that needed for mobile renderer can be found here - https://en.wikipedia.org/api/rest_v1/page/mobile-html-offline-resources/Canada *
(* title 'Canada' or any other title is required for this API)

kelson42 · 2023-09-26T14:46:23Z

@VadimKovalenkoSNF All internal links pointing to articles which are not mirrored should be removed. Wonder why this one is not!.

External links are OK but external resources obviously not. They ahve to be locally mirrored.

VadimKovalenkoSNF · 2023-09-27T18:08:59Z

[ERROR] Invalid external links found:

This one has been fixed, I mirrored mobile modules on the right way now.

[ERROR] Invalid internal links found

And this one doesn't seem to be related to the mobile renderer. It works depending on the article title, I left a comment here, maybe this should be highlighted in a separate ticket.

kelson42

I have finally run with thie renderer and we still have serious problems:

Many editing artifact which should be removed (to add description under the title, to edit wikidata entries, ....) See
None of the internal links work because somehow the onclick event is still caught

VadimKovalenkoSNF · 2023-09-28T09:57:23Z

Many editing artifact which should be removed (to add description under the title, to edit wikidata entries, ....) See

I removed them. If there is anything else for removal, I prepared a separate res/pcs/pcs_override_style.css where we can apply CSS overrides

None of the internal links work because somehow the onclick event is still caught

I fixed this, pls check.

VadimKovalenkoSNF · 2023-09-29T17:42:33Z

Tested mobile renderer output for bm.wikipedia.org:
--format=nopic - works as expected, 2 Mb
--format=nodet - works OK, 60 Mb
--format=nodet, nopic - works OK, 1.6 Mb
bm.wikipedia.org as is - works OK, but the size is 101,5 Mb, twice bigger than WikimediaDesktop render, this needs to be fixed. I suspect the problem is with image optimization. Here is a screenshot with comparison between same images that come from desktop and mobile renders.

kelson42

Tested the ZIM and it looks good in general, but the code needs IMO to be factored properly and partly simplified.

src/renderers/abstract.renderer.ts

src/mwoffliner.lib.ts

kelson42 · 2023-09-30T13:48:04Z

src/mwoffliner.lib.ts

@@ -401,6 +404,10 @@ async function execute(argv: any) {
    logger.info('Copying Static Resource Files')
    await saveStaticFiles(config, zimCreator)

+    // TODO: refactor sequence, this only needed for mobile renderer


this should be indeed part of the mobilerenderer

Static files handle only in src/mwoffliner.lib.ts, not in renders.

This is always the same kind of problem (like the CSS): code has to be factored properly. You have one place to save static files and only one, why not here. But the place to specify what are the static files to save that should be in the renderer. In addition it seems you save these static mobile files... all the time, even if they are not needed. which is not clean.

I replaced modules handling to renderers themself. Now abstract desktop and abstract mobile have the ability to filter out necessary modules that come from Downloader. In the execution process now should be the exact modules we need.

src/util/builders/url/base.director.ts

test/e2e/mobileRenderIntegrity.test.ts

src/util/saveArticles.ts

src/util/misc.ts

kelson42 · 2023-10-04T04:40:06Z

@VadimKovalenkoSNF Can you please rebase?

kelson42

Amost good, but IMO really problematic that the modules are not handled properly in the renderers. Same for "static" files!

kelson42 · 2023-10-04T04:45:24Z

res/templates/page.html

-
+    __ARTICLE_CONFIGVARS_LIST__
+    __ARTICLE_JS_LIST__
+    __PCS_JS_OVERRIDE__


If you need to override Mediawiki JS, then make a generic solution/naming to do that, but don't make it specific in particular with naming which is very Mediawiki specific (not MWoffliner

Ok, I replaced it with MW_MOBILE naming.

This is not what I meant. There should be only one place to override CSS and not a placeholder for each usage. A reall good name should be something like __CSS_OVERRIDE__ or __JS_OVERRIDE__ ... and inside you put whatever you want... here, for this end-point, the PCS related stuff.

Done by creating a separate template for desktop and mobile.

src/Downloader.ts

src/renderers/abstractMobile.render.ts

kelson42 · 2023-10-05T04:48:01Z

@VadimKovalenkoSNF Don't forget to add WikimediaMobile in RENDER_LIST

…m/mwoffliner into 1664-mobile-html-renderer

kelson42 · 2023-10-05T05:21:36Z

@VadimKovalenkoSNF As far as I can see, there is also a size problem. With this rendere, the ZIM file is almost and still 10x bigger than it should. Please compare the maxi versions (basically --format=novid) in https://download.kiwix.org/zim/wikipedia/

VadimKovalenkoSNF · 2023-10-06T06:35:44Z

@kelson42 , I've noticed that ab wiki doesn't work as expected with mobile render. Though it has the support of page/mobile-html (prooflink), there is an example of this article (desktop) that returns empty string in page/mobile-html (link). That causes an error and break entire scrapping process. I'll make a shallow error handling without exiting the runtime process. The side effect of this will be some missing articles.

kelson42 · 2023-10-06T07:02:15Z

@VadimKovalenkoSNF either this a bug by us and it has to be fixed or its a bug on wikimedia side and it has to be reported. If the backend is not able to deliver the content of an article, the whole scraper should stop with proper error. No tolerance to backend rendering errors if they concern a full article.

VadimKovalenkoSNF · 2023-10-06T12:24:06Z

I noticed that page/html is also not working for the article mentioned above, but mwoffliner doesn't exit the scrape process and returns the page with an empty body.

Upd: I created dedicated Phab ticket - https://phabricator.wikimedia.org/T348321

VadimKovalenkoSNF · 2023-10-09T13:52:40Z

Please compare the maxi versions (basically --format=novid)

I'm not sure that the problem is with novid format, I secured it in test/e2e/mobileRenderFormatParams.test.ts

kelson42

@VadimKovalenkoSNF If I got that right, this PR still needs to be fixed to have a better static content handling.

VadimKovalenkoSNF · 2023-10-15T12:39:38Z

@VadimKovalenkoSNF If I got that right, this PR still needs to be fixed to have a better static content handling.

Yes, static file saving is in progress.

src/mwoffliner.lib.ts

kelson42 · 2023-10-17T05:54:02Z

LGTM but the js/css handling really needs to be revamped, should be handled in the renders.

VadimKovalenkoSNF requested a review from kelson42 September 13, 2023 16:59

VadimKovalenkoSNF self-assigned this Sep 15, 2023

kelson42 requested changes Sep 20, 2023

View reviewed changes

src/Downloader.ts Outdated Show resolved Hide resolved

src/MediaWiki.ts Outdated Show resolved Hide resolved

src/renderers/renderer.builder.ts Outdated Show resolved Hide resolved

src/util/saveArticles.ts Outdated Show resolved Hide resolved

VadimKovalenkoSNF force-pushed the 1664-mobile-html-renderer branch 2 times, most recently from 6e2d33c to 251b6bb Compare September 22, 2023 05:56

VadimKovalenkoSNF requested a review from kelson42 September 22, 2023 08:44

VadimKovalenkoSNF marked this pull request as ready for review September 22, 2023 08:44

VadimKovalenkoSNF mentioned this pull request Sep 22, 2023

Make sure format option is working for WikimediaMobile renderer #1916

Closed

VadimKovalenkoSNF marked this pull request as draft September 25, 2023 11:12

VadimKovalenkoSNF marked this pull request as ready for review September 27, 2023 18:09

kelson42 requested changes Sep 28, 2023

View reviewed changes

VadimKovalenkoSNF requested a review from kelson42 September 28, 2023 09:57

kelson42 requested changes Sep 30, 2023

View reviewed changes

VadimKovalenkoSNF force-pushed the 1664-mobile-html-renderer branch from 1865aaf to a5d6edf Compare October 3, 2023 07:17

VadimKovalenkoSNF requested a review from kelson42 October 3, 2023 08:38

kelson42 requested changes Oct 4, 2023

View reviewed changes

kelson42 force-pushed the 1664-mobile-html-renderer branch from 7ffc1e2 to 113ff0f Compare October 5, 2023 04:47

Merge branch '1664-mobile-html-renderer' of https://github.com/openzi…

26ffb22

…m/mwoffliner into 1664-mobile-html-renderer

VadimKovalenkoSNF added 2 commits October 5, 2023 10:04

Refactor modules resolving logic for mobile renderer

546cb7e

Create separate page templates for mobile and desktop

d3027e1

VadimKovalenkoSNF added 2 commits October 6, 2023 10:18

Add error handling for empty data in WikimediaMobile render

b1e7737

Decrease image size in mobile render, secure tests

3b53982

VadimKovalenkoSNF added 3 commits October 6, 2023 16:25

Don't save jsConfigVars for mobile render

79d5acf

Throw an error if data no present for WikimediaDesktop render

5ea91e8

Update image optimization for mobile render

99a4eda

VadimKovalenkoSNF requested a review from kelson42 October 9, 2023 13:27

Update naming and minor refactoring of resizeMobileImages() method

17a50c0

Update attributes for lazy loaded images of mobile render

dbc3116

VadimKovalenkoSNF marked this pull request as draft October 9, 2023 16:19

VadimKovalenkoSNF mentioned this pull request Oct 10, 2023

Images from page/mobile-html endpoint are too big #1925

Open

VadimKovalenkoSNF marked this pull request as ready for review October 10, 2023 14:42

kelson42 requested changes Oct 15, 2023

View reviewed changes

Move static files list preparation into renders (partial impl)

61d7768

kelson42 reviewed Oct 16, 2023

View reviewed changes

src/mwoffliner.lib.ts Show resolved Hide resolved

VadimKovalenkoSNF added 2 commits October 16, 2023 10:12

Update en.e2e.test.ts file to check zim integrity

bdd0847

Move common static files handling to abstract render

d412ee4

kelson42 merged commit 8ebd7d0 into main Oct 17, 2023
6 checks passed

kelson42 deleted the 1664-mobile-html-renderer branch October 17, 2023 05:54

Implement new Wikimedia mobile end-point support/render #1903

Implement new Wikimedia mobile end-point support/render #1903

Conversation

VadimKovalenkoSNF commented Sep 11, 2023 • edited

codecov bot commented Sep 13, 2023 • edited

Codecov Report

VadimKovalenkoSNF commented Sep 15, 2023

VadimKovalenkoSNF commented Sep 19, 2023

kelson42 left a comment

Choose a reason for hiding this comment

VadimKovalenkoSNF commented Sep 22, 2023

kelson42 commented Sep 22, 2023

VadimKovalenkoSNF commented Sep 22, 2023

VadimKovalenkoSNF commented Sep 22, 2023

kelson42 commented Sep 22, 2023

VadimKovalenkoSNF commented Sep 26, 2023

kelson42 commented Sep 26, 2023 • edited

VadimKovalenkoSNF commented Sep 27, 2023 • edited

kelson42 left a comment

Choose a reason for hiding this comment

VadimKovalenkoSNF commented Sep 28, 2023

VadimKovalenkoSNF commented Sep 29, 2023

kelson42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelson42 commented Oct 4, 2023

kelson42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelson42 commented Oct 5, 2023

kelson42 commented Oct 5, 2023

VadimKovalenkoSNF commented Oct 6, 2023 • edited

kelson42 commented Oct 6, 2023 • edited

VadimKovalenkoSNF commented Oct 6, 2023 • edited

VadimKovalenkoSNF commented Oct 9, 2023

kelson42 left a comment

Choose a reason for hiding this comment

VadimKovalenkoSNF commented Oct 15, 2023

kelson42 commented Oct 17, 2023

VadimKovalenkoSNF commented Sep 11, 2023 •

edited

codecov bot commented Sep 13, 2023 •

edited

kelson42 commented Sep 26, 2023 •

edited

VadimKovalenkoSNF commented Sep 27, 2023 •

edited

VadimKovalenkoSNF commented Oct 6, 2023 •

edited

kelson42 commented Oct 6, 2023 •

edited

VadimKovalenkoSNF commented Oct 6, 2023 •

edited