Iframe-based content viewer #716
Conversation
Codecov ReportBase: 70.98% // Head: 70.51% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #716 +/- ##
==========================================
- Coverage 70.98% 70.51% -0.47%
==========================================
Files 53 53
Lines 3736 3653 -83
Branches 2078 2029 -49
==========================================
- Hits 2652 2576 -76
+ Misses 1082 1075 -7
Partials 2 2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
veloman-yunkan
force-pushed
the
iframe_based_content_viewer
branch
2 times, most recently
from
f7287a7
to
693cb82
Compare
|
@kelson42 A prototype version of the iframe-based viewer is ready. I invite you to play with it by building kiwix-serve with this PR included, running it and opening the Known issues:
|
|
@veloman-yunkan Great, I will have a look. |
|
@veloman-yunkan I have tested and it looks promising. I have not much to say beside the known issues you have listed:
@rgaudin @mgautierfr Would be good you could give a try as well but to me it looks promising and the basics seem to work fine (with Wikipedia and LowTechMagazin SW file). We should probably implement what is missing to start to check that everything works fine. |
This was done. I simply slightly messed up when publishing the viewer for public testing. The newly pushed fixup commit should make it work.
What is a SW-based ZIM file? I have never dealt with such files before.
Can you please try again with the mentioned fix? If you still have the same issue, please provide more details about it.
That was another known issue that I forgot to mention. |
|
Regarding SW based ZIM files. These ZIM files are made by openzim/warc2zim. All the HTML is played by a page using Service-Workers (SW), therefore they work slightly differently from traditional ZIM. Many of them are available at https://download.kiwix.org/zim/zimit/. SW has a lot of security contstraints and basically works only through HTTPS or with |
|
404 somehow works now but:
I think you are on the good way and I could recommend to fix the last glitches so we can start a complete review. I would also recommend to make this feature as an cmd line option (so not based on a cookie) if this is not too complex to deal legacy and new taskbars together. Otherwise, we will probably have to take the risk to just move on with this new iframe based taskbar. |
@kelson42 What is the preferred way of deploying the new taskbar? The two extreme scenarios are:
Your recommendation (of enabling the new mode via a command line option of kiwix-serve) is between these two edge cases. What are the justifications for such a compromise? |
|
@veloman-yunkan To me, if the overall overhead of #1 is not over one day, then #1 is probably preferable. Otherwise #2. |
Can you please clarify what you mean by overhead of one day? 24 man*hours? |
No, around 8 hours. |
That probably means that we will have to go with option 2. But I think that we better have @mgautierfr's input too. |
|
Here are some of my thoughts regarding the final result of switching to an iframe-based viewer: Book resources accessed through the Though that is a minor issue but solving it will give us more freedom when in need of adding new endpoints. |
|
Lot of things here. There is this PR #639 which remove jquery from the taskbar. I haven't finish it because of time and I'm not sure of some js/css feature implemented. About the plan, I'm in favor of .2, we would need better testing (and there is somehow already conflict between open PRs) but it would greatly simplify the code. We have a lot of code everywhere to serve "raw" content or not depending of the context, changing path and so. Having only one version would simplify this : never insert taskbar (remove the code), (almost) always return raw content... (And I'm speaking of simplification compared from what we already have, not what we would have if we go for .1) Another point about the search.
Totally agree with you. We should have a better "REST" API and stop serving content using the |
src/server/internalServer.cpp
Outdated
| auto entry = getEntryFromPath(*archive, itemPath); | ||
| if (entry.isRedirect() || itemPath.empty()) { | ||
| return build_redirect(bookName, entry.getItem(true), /*raw=*/true); |
There was a problem hiding this comment.
I disagree with this. /raw returns the raw content and if the requested path is not present, return nothing.
The empty path (`` or /) is a valid path in a zim file. Returning something different if we request it and it is not present break the "raw" feature.
(But it opens the room for the `/content` endpoint your speak about in your comment)
There was a problem hiding this comment.
This change can be undone, provided that the support for old taskbar is dropped.
src/server/internalServer.cpp
Outdated
| @@ -630,7 +630,9 @@ std::unique_ptr<Response> InternalServer::handle_search(const RequestContext& re | |||
| renderer.setProtocolPrefix(m_root + "/"); | |||
| renderer.setSearchProtocolPrefix(m_root + "/search?"); | |||
| renderer.setPageLength(pageLength); | |||
| auto response = ContentResponse::build(*this, renderer.getHtml(), "text/html; charset=utf-8"); | |||
| const bool rawMode = rawModeWasRequested(request); | |||
There was a problem hiding this comment.
In #731 I introduce the format=xml for opensearch results. (without being sure that format is the right name).
I wonder if there is something to define in common or not.
There was a problem hiding this comment.
If we drop the old taskbar, I would prefer to undo the ?mode=raw hacks for the /random and /search endpoints
To justify taking the approach no. 2, we must properly prioritize the work on the iframe-based taskbar. For example, in the refactoring portion of #679 (currently spun off as #732) a noticeable effort has been put into handling the taskbar. Now it is quite disappointing to acknowledge that all that effort was kind of in vain (yet it shouldn't prevent us from doing the right thing). @kelson42 @mgautierfr Can we come up with a short-term plan for our work in libkiwix? |
|
@veloman-yunkan @mgautierfr It is indeed really unfortunate if work is done and then thrown away. We don't have the resources here to allow us this kind of waste. I'm really thankful to any initiative to avoid this or improve the overall code througput. But, I have no strong opinion or plan in which orders things/tickets should be implemented within a milestone. To a large extend I trust you (the developers). I also trust you to talk to each other and find the optimal path to do things. On my side, when I see a few tickets have been achieved and the current DOING pipe is a bit empty, I use to bring your attention on this or that ticket. Usually these tickets are urgent (incident) tickets or tickets in the current milestone. @veloman-yunkan Beside this, your work since a year is strongly tight to the "Better Library" project https://github.com/orgs/kiwix/projects/8. On Friday, we had to release |
|
Indeed it is a pity that unnecessary work has been done. When you start working on internationalization, I did plan that you would change so much things. I see three solutions to avoid (or minimize) such problem:
|
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions. |
|
@veloman-yunkan If I follow things properly, it’s time now to rebase and complete this PR right? |
If we go with plan 2, we must wait till there are no concurrent big changes going on in libkiwix at the same time. The search enhancement (#729 and friends) is one such big chunk of work. Let's compile a full list of such planned changes and decide an optimal implementation order for them. |
#729 and friends are part of 10.2.0 milestone (to be released ASAP) and what we talk here is part of next 10.3.0. I can only recommend to have a look to the whole 10.3.0 milestone to make you an opinion. To me, and AFAIK, bringing the iframe is a fondamental work to do to allow a proper cache system. Therefore things should be done in this order. What runs as well, is the removal of JQuery which is almost over, you have here to agree with @mgautierfr when to bring this. All the rest of what is in 10.3.0 milestone is less prioritary and could/might be done later. #479 and #754 might need an early architecture discussion to avoid later bad suprises. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions. |
Made the viewer respect the `--blockexternal` and `--nolibrarybutton` options of `kiwix-serve`. Those options are passed to the viewer via the dynamically generated resource `/viewer_settings.js`.
`--nosearchbar` option of `kiwix-serve` (despite its misleading name) was used to disable the entire taskbar. This commit accounts for the existence of that option only partially: 1. Links to books on the welcome/library page are affected - by default books are displayed in the viewer, but in a kiwix-serve instance run with --nosearchbar books are loaded in the top window. 2. The `/viewer` endpoint is enabled unconditionally, so if anyone enters the viewer URL in the address bar they will see books in the viewer.
This reverts commit 436d890.
Auto hiding of the toolbars on narrow screens works only for the first page loaded in the viewer. Navigating to other pages interferes with autohiding as follows: - If the toolbar was hidden, it stays hidden. - If the toolbar was not hidden, it loses the ability to autohide.
If `kiwix-serve` is run with the `--nosearchbar` option the toolbar is disabled (hidden) in its viewer. Note however that certain actions performed by the viewer merely with the purpose of keeping the toolbar up-to-date are still carried out.
A rebase invalidated the cacheids in the previous commits of the iframe_based_content_viewer branch. This commit fixes only the current state leaving the history with wrong cacheids - this can be an issue for `git bisect` being executed on a commit range overlapping with the iframe_based_content_viewer branch.
veloman-yunkan
force-pushed
the
iframe_based_content_viewer
branch
from
a2b0cc0
to
06af1a6
Compare
@mgautierfr Done. I also rebased on recent master. You only have to look on the last four commits. Please tell me if the issue with out-of-date cacheids in the old commits has to be fixed. |
With taskbar no longer being injected into the responses, it doesn't make any sense testing the search on different flavours of the server.
veloman-yunkan
force-pushed
the
iframe_based_content_viewer
branch
from
06af1a6
to
0a0f52f
Compare
|
I think that before merging the code has to go through independent testing by some users. |
I think we can merge now. With master we have nightlies user can test with. And if there is bug, it can be fixed by another (smaller) PR instead of adding to this long PR. Of course we need to test master before doing the release. |
|
I am also in favor of merging now and using our regular process. It will make this larger testing a lot easier IMO. |
|
Time to merge this huge PR 🎉 |
|
@veloman-yunkan @mgautierfr The tickets corresponding to the work left, questionning, regressions have not been opne. please to that immediatly before we forget. |
|
Should be deployed on dev with nightly tomorrow ; will announce on slack once it is |
@kelson42 The only issue that I was aware about is now filed as #820. |
* [API Break] Remove wrapper around libzim (@mgautierfr #789) * Allow kiwix-serve to use custom resource files (@veloman-yunkan #779) * Properly handle searchProtocolPrefix when rendering search result (@veloman-yunkan #823) * Prevent search on multi language content (@veloman-yunkan #838) * Use new `zim::Archive::getMediaCount` from libzim (@mgautierfr #836) * Catalog: - Include tags in free text catalog search (@veloman-yunkan #802) - Illustration's url is based on book's uuid (@veloman-yunkan #804) - Cleanup of the opds-dumper (@veloman-yunkan #829) - Allow filtering of catalog content using multiple languages (@veloman-yunkan #841) - Make opds-dumper respect the namemapper (@mgautierfr #837) * Server: - Correctly handle `\` in suggestion json generation (@veloman-yunkan #843) - Better http caching (@veloman-yunkan #833) - Make `/suggest` endpoint thread-safe (@veloman-yunkan #834) - Better redirection of main page (@veloman-yunkan #827) - Remove jquery (@mgautierfr @juuz0 #796) - Better Viewer of zim content : . Introduce `/content` endpoints (@veloman-yunkan #806) . Switch to iframe based content viewer (@veloman-yunkan #716) - Optimised design of the welcome page: . Alignement (@juuz0 @kelson42 #786) . Exit download modal on pressing escape key (@Juzz0 #800) . Add favicon for different devices (@Juzz0 #805) . Fix auto hidding of the toolbar (@veloman-yunkan #821) . Allow user to filter books by tags in the front page (@juuz0 #711) * CI : - Trigger CI on pull_request (@kelson42 #791) - Drop Ubuntu Impish packaging (@legoktm #825) - Add Ubuntu Kinetic packaging (@legoktm #801) * Testing: - Test ICULanguageInfo (@veloman-yunkan #795) - Introduce fake `test` language to test i18n (@veloman-yunkan #848) * Fix documentation (@kelson42 #816) * Udpate translation (#787 #839 #847)
Will fix #394
Will fix #671
/contentendpoint. Book content is served both under/content/<book>/<path>/<to>/<entry>and (for backward compatibility) under/<book>/<path>/<to>/<entry>./viewer#<book>/<path>/<to>/<entry>displays in the iframe the content of<path>/<to>/<entry>of the book<book>./viewer#search?<search_query>displays in the iframe the search results for<search_query>. Enhanced book filtering in<search_query>is not supported (a single book must be specified via thecontentquery parameter).Questions:
/viewer#search?<search_query>an acceptable URL scheme for search URLs? A subtle problem with it is that it assumes that a book cannot be namedsearch. An alternative is/viewer/search#<search_query>(or maybe you have other suggestions).