Kiwix Serve welcome page dynamic and subset loading (OPDS based) #530

MananJethwani · 2021-05-18T12:43:12Z

@kelson42 this resolves #511 and #514. Filters will be added in other PR.

kelson42 · 2021-05-18T13:47:13Z

@MananJethwani It does not fix #517 and #519?

MananJethwani · 2021-05-18T13:51:33Z

@kelson42 no, I will be create another PR for that soon.

kelson42 · 2021-05-18T13:53:10Z

@MananJethwani Ok, even better like this.

kelson42 · 2021-05-18T13:54:27Z

@rgaudin We should secure the frontend tech could be include in offspot. Your review is needed for this.

kelson42

From a user perspective it works like expect. We need the technical review.

rgaudin · 2021-05-20T08:10:10Z

@rgaudin We should secure the frontend tech could be include in offspot. Your review is needed for this.

I don't think I can do that at the moment. Offspot requirements are not ready. We only have UI mockups at this stage and the extent of could be include in offspot is not defined neither and that could have drastic consequences.

kelson42

There is one problem. If there is no URL associated with a ZIM file, the content is not in the OPDS and the welcome page is empty. The logic behind this is clear, but we need to have the welcome page working. Maybe we should add a parameter to the OPDS search feed to deliver as well the ZIM without URLs?

MananJethwani · 2021-05-20T17:57:12Z

@kelson42 I agree will start implementing it.

kelson42 · 2021-05-21T05:11:15Z

@MananJethwani Or maybe we just deliver everything and this is the role of the kiwix user to do the necessary if he wants to make the content downloadable? @mgautierfr @veloman-yunkan what do you think?

MananJethwani · 2021-05-22T09:29:56Z

@kelson42 I believe the second option is better we should deliver everything and we can then add a disabled download button if we don't get a URL and add a tooltip to show a message for the disabled download button.
also if we don't deliver everything then what would be the use case left for the ZIM_PATH method fr starting the server as we can't add URL in that case.

MananJethwani · 2021-05-22T09:56:41Z

src/server/internalServer.cpp

@@ -658,6 +658,8 @@ std::vector<std::string>
 InternalServer::search_catalog(const RequestContext& request,
                               kiwix::OPDSDumper& opdsDumper)
 {
+    // added for discussion in #530 using the following method we can deliver zim file irrespective of a remote URl present for it.
+    // auto filter = kiwix::Filter().valid(true).local(true);


@kelson42 I have added that method in the comments here.
comment the line below it and remove comments from this one to get your desired results.

@MananJethwani kiwix-serve --port=8082 *zim still gives me an empty welcome page.

@kelson42 are you sure you commented the correct line? because I tried it and it works for me.
you need to comment L663 and uncomment L662 ... it works for me that way.
here is the proof you can clearly see the last link in this entry is for thumbnail but if I would have added a URL there would be another link there.

veloman-yunkan

All the new global variables and functions in index.js increase the risk of interfering with variables/functions defined in other JS scripts (including those from the ZIM archives).

static/skin/index.js

MananJethwani · 2021-05-24T14:53:30Z

@veloman-yunkan I have pushed the changes, I have added the closure as I tested block-external feature and it doesn't work as constant root from index.js was interfering with constant root from block-external.js.

MananJethwani · 2021-05-24T14:55:09Z

src/server/internalServer.cpp

-    auto filter = kiwix::Filter().valid(true).local(true).remote(true);
+    // added for discussion in #530 using the following method we can deliver zim file irrespective of a remote URl present for it.
+    auto filter = kiwix::Filter().valid(true).local(true);
+    // auto filter = kiwix::Filter().valid(true).local(true).remote(true);


please let me know if this is fine so that I will remove the comments.

@veloman-yunkan what about this?

static/skin/index.js

veloman-yunkan

Looks like this is the last iteration. After addressing the few comments included in this review, please clean-up your PR history, so that we can merge without an extra cycle.

static/skin/index.js

veloman-yunkan · 2021-05-24T17:45:15Z

I have added the closure

Great! I would welcome it even in the absence of any current conflict. The cost of doing it is minimal, while it can save a lot of debugging time in the future.

kelson42

Works as expected

mgautierfr

This implementation has race condition.

You can reproduce it by using the "adaptive view" in firefox developer tools :

Set the bandwidth to regular 2G
Set the windows size to make the js NOT download all available books.
While it is loading, resize the window.
Js loads (and display) several times the same book (and some books are missing)

mgautierfr · 2021-05-25T15:08:29Z

src/server/internalServer.cpp

@@ -658,7 +658,7 @@ std::vector<std::string>
 InternalServer::search_catalog(const RequestContext& request,
                               kiwix::OPDSDumper& opdsDumper)
 {
-    auto filter = kiwix::Filter().valid(true).local(true).remote(true);
+    auto filter = kiwix::Filter().valid(true).local(true);


Why removing this ?

OPDS stream list zim file a user can download. If we remove the remote(true) filter, we will return zim file without url to download from.

@mgautierfr kelson discussed this point earlier in the PR and we agreed that we should show all zim files and show a disabled download button(and a tooltip) for those zim files that can't be downloaded.

The problem is that ODPS stream is already used (and designed for) native client which list what zim files user can download.
By definition, the user can download a zim file if they have a associated url (they are "remote").
If you remove this filter, you break the API for existing client.

@mgautierfr Can you please be specific? I'm not aware of anything broken. If OPDS should not list anymore books with URL, it should list all books. This is necessary for the welcome page and this is more logical because the URL is only one attribute beetween others of the book. If at some point we need to filter on URL them we should add a new filter, but AFAIK this is not needed to the moment.

kiwix-desktop uses OPDS stream to propose zim file to download to the user. If OPDS stream now return one zim file without "url" attribute, kiwix-desktop will display to book to the user but he will not be able to download it.
(I've checked and kiwix-desktop reapply this filter internally before displaying books to the user, but other implementation (kiwix-android, kiwix-ios)) may not do the same).
(And this is maybe hypotical anyway as most (if not all) zim files in library.kiwix.org have a url, but if someone use a local opds stream, it will be broken)

This is a OPDS API break, we cannot change the content returned by a stream like that.
We may want to change this behavior or not (we can extend the catalog/v2 to handle this feature and use catalog/v2), but if we change it we have to be careful about this.

@mgautierfr @kelson42 instead of creating a completely new endpoint wouldn't it be better if I just introduce a new search parameter that will check if we need all zim files and changes filter accordingly?

@mgautierfr Kiwix-Desktop works perfectly, so there is no problem, there is no API break, there is no documentation to update. What you are talking about seems to be a potential problem if for some reason library.kiwix.org would deliver an OPDS stream with books without URLs. An "yes" I confirm, this is the resonsability of the person running the kiwix-serve to have a library.xml with URL if he wants that these books are downloadable. What is concretly wrong here or broken?

mgautierfr · 2021-05-25T15:11:03Z

static/templates/index.html

-    {{#books}}
-    <a href="{{root}}/{{name}}"><div class='book'>
-        <div class='book__background' style="background-image: url('{{root}}/meta?content={{#urlencoded}}{{{name}}}{{/urlencoded}}&name=favicon');">
-          <div class='book__title' title='{{title}}'>{{title}}</div>
-          <div class='book__description' title='{{description}}'>{{description}}</div>
-          <div class='book__info'>{{articleCount}} articles, {{mediaCount}} medias</div>
-        </div>
-    </div></a>
-    {{/books}}


If we don't use the books variable in the template, we should not set it in the cpp side.

I agree on this point I was going to open a separate issue for this, but if you want it removed from cpp side I will create a new PR and add that part in some time.

Why the next time ? I don't "want" to remove the cpp code. I want to have the template and the cpp in sync.

by remove, I meant that only😅.... I will bring them in sync.

MananJethwani requested a review from kelson42 May 18, 2021 12:43

MananJethwani force-pushed the adding_dynamic_and_subset_loading branch from e43d664 to 0bdd3dd Compare May 18, 2021 12:46

kelson42 linked an issue May 18, 2021 that may be closed by this pull request

Load & display only a subset of the results on the welcome page #514

Closed

kelson42 requested a review from veloman-yunkan May 18, 2021 13:47

kelson42 requested a review from rgaudin May 18, 2021 13:53

kelson42 changed the title ~~Adding dynamic and subset loading~~ Kiwix Serve welcome page dynamic and subset loading May 20, 2021

kelson42 changed the title ~~Kiwix Serve welcome page dynamic and subset loading~~ Kiwix Serve welcome page dynamic and subset loading (OPDS based) May 20, 2021

kelson42 force-pushed the adding_dynamic_and_subset_loading branch from 0bdd3dd to 8ba1073 Compare May 20, 2021 06:43

kelson42 approved these changes May 20, 2021

View reviewed changes

kelson42 self-requested a review May 20, 2021 13:18

kelson42 requested changes May 20, 2021

View reviewed changes

MananJethwani force-pushed the adding_dynamic_and_subset_loading branch from 8ba1073 to 31c6d60 Compare May 22, 2021 09:54

MananJethwani commented May 22, 2021

View reviewed changes

veloman-yunkan requested changes May 23, 2021

View reviewed changes

MananJethwani force-pushed the adding_dynamic_and_subset_loading branch from 9928ae7 to caa1638 Compare May 24, 2021 11:23

MananJethwani requested a review from veloman-yunkan May 24, 2021 11:24

veloman-yunkan requested changes May 24, 2021

View reviewed changes

static/skin/index.js Outdated Show resolved Hide resolved

static/skin/index.js Show resolved Hide resolved

static/skin/index.js Outdated Show resolved Hide resolved

static/skin/index.js Outdated Show resolved Hide resolved

MananJethwani commented May 24, 2021

View reviewed changes

static/skin/index.js Outdated Show resolved Hide resolved

MananJethwani requested a review from veloman-yunkan May 24, 2021 14:56

veloman-yunkan requested changes May 24, 2021

View reviewed changes

static/skin/index.js Outdated Show resolved Hide resolved

static/skin/index.js Outdated Show resolved Hide resolved

added dynamic and subset loading of zim-files in kiwix-serve

012973d

MananJethwani force-pushed the adding_dynamic_and_subset_loading branch from 1f93bbe to 012973d Compare May 24, 2021 21:12

MananJethwani requested a review from veloman-yunkan May 24, 2021 21:13

veloman-yunkan approved these changes May 25, 2021

View reviewed changes

MananJethwani requested a review from kelson42 May 25, 2021 08:48

kelson42 approved these changes May 25, 2021

View reviewed changes

kelson42 merged commit 672b4fc into master May 25, 2021

kelson42 deleted the adding_dynamic_and_subset_loading branch May 25, 2021 14:22

mgautierfr reviewed May 25, 2021

View reviewed changes

mgautierfr mentioned this pull request May 25, 2021

Revert "Kiwix Serve welcome page dynamic and subset loading (OPDS based)" #538

Merged

MananJethwani mentioned this pull request May 25, 2021

added dynamic and subset loading of zim-files in kiwix-serve #540

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kiwix Serve welcome page dynamic and subset loading (OPDS based) #530

Kiwix Serve welcome page dynamic and subset loading (OPDS based) #530

MananJethwani commented May 18, 2021 •

edited by kelson42

Loading

kelson42 commented May 18, 2021

MananJethwani commented May 18, 2021

kelson42 commented May 18, 2021

kelson42 commented May 18, 2021

kelson42 left a comment

rgaudin commented May 20, 2021

kelson42 left a comment •

edited

Loading

MananJethwani commented May 20, 2021

kelson42 commented May 21, 2021

MananJethwani commented May 22, 2021

MananJethwani May 22, 2021

kelson42 May 22, 2021

MananJethwani May 22, 2021

veloman-yunkan left a comment

MananJethwani commented May 24, 2021 •

edited

Loading

MananJethwani May 24, 2021

MananJethwani May 24, 2021

veloman-yunkan left a comment

veloman-yunkan commented May 24, 2021

kelson42 left a comment

mgautierfr left a comment

mgautierfr May 25, 2021

MananJethwani May 25, 2021

mgautierfr May 26, 2021

kelson42 May 26, 2021 •

edited

Loading

mgautierfr May 26, 2021

MananJethwani May 26, 2021

kelson42 May 26, 2021

mgautierfr May 25, 2021

MananJethwani May 25, 2021

mgautierfr May 26, 2021 •

edited

Loading

MananJethwani May 26, 2021

Kiwix Serve welcome page dynamic and subset loading (OPDS based) #530

Kiwix Serve welcome page dynamic and subset loading (OPDS based) #530

Conversation

MananJethwani commented May 18, 2021 • edited by kelson42 Loading

kelson42 commented May 18, 2021

MananJethwani commented May 18, 2021

kelson42 commented May 18, 2021

kelson42 commented May 18, 2021

kelson42 left a comment

Choose a reason for hiding this comment

rgaudin commented May 20, 2021

kelson42 left a comment • edited Loading

Choose a reason for hiding this comment

MananJethwani commented May 20, 2021

kelson42 commented May 21, 2021

MananJethwani commented May 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veloman-yunkan left a comment

Choose a reason for hiding this comment

MananJethwani commented May 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veloman-yunkan left a comment

Choose a reason for hiding this comment

veloman-yunkan commented May 24, 2021

kelson42 left a comment

Choose a reason for hiding this comment

mgautierfr 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

kelson42 May 26, 2021 • edited Loading

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

Choose a reason for hiding this comment

mgautierfr May 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MananJethwani commented May 18, 2021 •

edited by kelson42

Loading

kelson42 left a comment •

edited

Loading

MananJethwani commented May 24, 2021 •

edited

Loading

kelson42 May 26, 2021 •

edited

Loading

mgautierfr May 26, 2021 •

edited

Loading