Added kiwix logo to OPDS feed by rajvi0106 · Pull Request #1260 · kiwix/libkiwix

rajvi0106 · 2025-12-24T15:49:54Z

Fixes #976

Problem
The OPDS feed lacks support of element and instead falls back to the generic non descriptive RSS logo.
Solution
To address this issue first extended the render template by adding logo_url, then to actually render this value OPDS XML file is extended with and element (both to ensure it is sized well for different screen sizes), and then updates the expected output in library_server to match the generated xml.

Changes Implemented

Added the logo_url in OPDS template.
Included and element to OPDS xml template.
Updates the tests in Library_server to match the new generated output.

Copilot

Pull request overview

This PR adds logo and icon elements to the OPDS feed to replace the generic RSS logo. The implementation includes adding the Kiwix logo URL to the OPDS template data and updating XML templates to render these elements.

Adds <logo> and <icon> elements to OPDS feed XML templates
Updates test expectations across multiple catalog endpoints to include the new logo elements
Uses the Kiwix logo hosted on Wikimedia Commons

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
src/opds_dumper.cpp	Adds logo_url field to template data in both OPDS feed generation functions
static/templates/catalog_entries.xml	Adds logo and icon XML elements to the catalog template
static/templates/catalog_v2_entries.xml	Adds logo and icon XML elements to the v2 catalog template
test/library_server.cpp	Updates all test expectations to include the new logo and icon elements in expected XML output

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/opds_dumper.cpp

test/library_server.cpp

src/opds_dumper.cpp

kelson42

Thank younfor your PR.

The issue is onky about the feed logo, you should not add default kiwix logog everywhere.

The logo shoukd be embedded in Kiwix code base and we should not rely on an online one (otherwise it won't work offline).

rajvi0106 · 2025-12-27T15:58:08Z

Thank younfor your PR.

The issue is onky about the feed logo, you should not add default kiwix logog everywhere.

The logo shoukd be embedded in Kiwix code base and we should not rely on an online one (otherwise it won't work offline).

hi!
thanks for the clarification i have added the kiwix logo only at opds feed level templates(dumpOPDSFeedV2 and dumpOPDSFeed) and for the logo i will add it in the code base

veloman-yunkan

I am pretty sure that the final form of this PR has not been tested via kiwix-serve.

veloman-yunkan · 2025-12-29T12:52:17Z

src/opds_dumper.cpp


 namespace
 {
+const std::string kiwix_logo_url="static/skin/kiwix_logo.png";  


This is a relative URL. I doubt that it will work in all (even the most typical) cases.

veloman-yunkan · 2025-12-29T13:01:37Z

static/skin/kiwix_logo.png

Does this logo indeed have to be this big? Can't we reuse static/skin/favicon/favicon-32x32.png?

Just adding this file under static/skin doesn't automatically make it an embedded resource of libkiwix. It also has to be registered in static/resources_list.txt, and tested in test/server.cpp (similar to how it is done for favicon-32x32.png).

Does this logo indeed have to be this big? Can't we reuse static/skin/favicon/favicon-32x32.png?

Just adding this file under static/skin doesn't automatically make it an embedded resource of libkiwix. It also has to be registered in static/resources_list.txt, and tested in test/server.cpp (similar to how it is done for favicon-32x32.png).

yes i guess we should use faivon-32X32 png, it is small and squared. I'll do the required changes.

kelson42 · 2026-01-03T13:33:18Z

@rajvi0106 What is the status here? I see comment from @veloman-yunkan which are not been clearly answered and it is not clear either if this has been fixed. Please answer each of them pointing to the solution and let us now with a general comment if you are ready for a new review.

rajvi0106 · 2026-01-04T18:36:25Z

I am pretty sure that the final form of this PR has not been tested via kiwix-serve.

sorry for the delay, you are correct even now when i changed the logo and used favicon logo when running the kiwix serve /catalog/v2/entries it is showing the url i used in first commit.

rajvi0106 · 2026-01-07T13:42:23Z

@kelson42 @veloman-yunkan sorry for the delay! I have made the changes. Please review and let me know what you think and if any further adjustments are needed.

veloman-yunkan

I reiterate my previous assessment that this PR has not been tested via kiwix-serve. Please make sure that your changes achieve the desired effect in the tools where they can have a user-visible effect.

rajvi0106 · 2026-01-10T11:26:51Z

I reiterate my previous assessment that this PR has not been tested via kiwix-serve. Please make sure that your changes achieve the desired effect in the tools where they can have a user-visible effect.

I have tested this via kiwix-serve.

Please let me know if any changes are needed.

veloman-yunkan · 2026-01-12T12:26:02Z

I reiterate my previous assessment that this PR has not been tested via kiwix-serve. Please make sure that your changes achieve the desired effect in the tools where they can have a user-visible effect.

I have tested this via kiwix-serve.

Please let me know if any changes are needed.

This is not how it should be tested. You must make sure that the logo shows up in the browser or RSS feed reader. With the current implementation it won't.

rajvi0106 · 2026-01-17T07:43:40Z

Hii! @veloman-yunkan
please check if this implementation is okay, and let me know if any changes are needed.

veloman-yunkan

Good! Now this looks like it can work, though I have yet to test it. But I will do so after my comments are addressed.

veloman-yunkan · 2026-01-19T13:15:56Z

src/opds_dumper.cpp

-     {"books", booksData }
+     {"books", booksData },
+     {"logo_url", baseUrl+kiwix_logo_url}


Does the logo/icon URL have to be a full URL (i.e. including the scheme and host components)? Can't it be just an absolute URL (referring to the same host)?

Please take into account the -r/--urlRootLocation option of kiwix-serve

using a full url was the first approach which came to my mind (to make sure that the image opens up in browser or RSS feed reader) but sure i will use the absolute url referring to the same host.

veloman-yunkan · 2026-01-19T13:19:48Z

src/opds_dumper.cpp

-     {"books", booksData }
+     {"books", booksData },
+     {"logo_url", baseUrl + kiwix_logo_url}


veloman-yunkan · 2026-01-19T13:22:20Z

test/library_server.cpp

+  try {
+    s = std::regex_replace(s, std::regex("https?://(?:localhost|127\\.0\\.0\\.1)(?::[0-9]+)?"), "");
+    s = std::regex_replace(s, std::regex("https?://\\[::1\\](?::[0-9]+)?"), "");
+  } catch (...) {}
+


I don't like this, but it may be automatically solved if an absolute URL instead of a full URL can be used for the logo/icon URL.

rajvi0106 · 2026-02-02T14:35:13Z

@veloman-yunkan sorry for the delay! please review it

veloman-yunkan

Please squash all your commits into one

veloman-yunkan · 2026-02-02T14:41:55Z

Please squash all your commits into one

... and rebase your branch on recent main

avoid hard coded url and added feed logo file favicon logo used removed a test line Use contentServerURL to build absolute OPDS logo/icon links fix logo url so it renders in browser and readers use of absolute url

veloman-yunkan

This PR now looks almost ready to be merged so I decided to test it, but no icon/logo was displayed in the Feedbro feed reader extension for Firefox which I installed just for testing this PR (I didn't use a feed reader for many years). How did you test it?

Regarding the commit message - it bears all the history of how this change came to be. We don't need it. Please clean up the commit message. Also please eliminate the accidental or left-over changes unrelated to the functionality being implemented.

veloman-yunkan · 2026-02-03T14:57:39Z

src/opds_dumper.h

 #include "library.h"
 #include "name_mapper.h"
 #include "library_dumper.h"
+#include "server/request_context.h"


Undo this change

veloman-yunkan · 2026-02-03T14:57:48Z

src/opds_dumper.cpp

 {
  const auto endpointRoot = rootLocation + "/catalog/v2";
  const auto booksData = getBooksData(library, nameMapper, bookIds, rootLocation, contentAccessUrl, partial);
-


Undo this change

veloman-yunkan · 2026-02-03T14:58:04Z

test/library_server.cpp


 TEST_F(LibraryServerTest, catalog_root_xml)
-{
+{ 


Undo this change

rajvi0106 · 2026-02-05T11:13:10Z

This PR now looks almost ready to be merged so I decided to test it, but no icon/logo was displayed in the Feedbro feed reader extension for Firefox which I installed just for testing this PR (I didn't use a feed reader for many years). How did you test it?

Regarding the commit message - it bears all the history of how this change came to be. We don't need it. Please clean up the commit message. Also please eliminate the accidental or left-over changes unrelated to the functionality being implemented.

the logo is being served correctly in browser but in feedbro and and thorium reader no logo is there

veloman-yunkan · 2026-02-05T11:17:10Z

the logo is being served correctly in browser

What do you mean by that? Accessing http://localhost:PORT/skin/favicon/favicon-32x32.png directly?

rajvi0106 · 2026-02-05T11:24:39Z

the logo is being served correctly in browser

What do you mean by that? Accessing http://localhost:PORT/skin/favicon/favicon-32x32.png directly?

yes yes when i am running it directly is showing the picture used

veloman-yunkan · 2026-02-05T11:29:27Z

Then the next question is what made you think that the logo URL in OPDS xml should be exposed via the <logo> and/or <icon> tag? Can you provide a reference to the spec?

rajvi0106 · 2026-02-05T11:55:22Z

https://validator.w3.org/feed/docs/atom.html
here it is given in option feed elements that logo or icon tag can be used (using these tags were my first thought) but now i am going to test it with link tag

veloman-yunkan · 2026-02-05T12:10:55Z

I checked that it works in thunderbird. But now that you pointed to the spec, I see that the 32x32 image doesn't comply with what is expected from a <logo> visual.

rajvi0106 · 2026-02-05T12:20:56Z

yes that is why i used both logo and icon tags because i was using a different image earlier. So should i go forward with adding link tag or remove the logo tag?

veloman-yunkan · 2026-02-05T12:33:56Z

Let's wait for the answers to my questions (1, 2) in the ticket.

kelson42 requested review from Copilot, kelson42 and veloman-yunkan December 27, 2025 14:32

kelson42 force-pushed the add-kiwix-logo branch from 4ddae1a to 9acc978 Compare December 27, 2025 14:33

Copilot started reviewing on behalf of kelson42 December 27, 2025 14:33 View session

Copilot AI reviewed Dec 27, 2025

View reviewed changes

src/opds_dumper.cpp Outdated Show resolved Hide resolved

src/opds_dumper.cpp Outdated Show resolved Hide resolved

src/opds_dumper.cpp Outdated Show resolved Hide resolved

test/library_server.cpp Outdated Show resolved Hide resolved

src/opds_dumper.cpp Outdated Show resolved Hide resolved

kelson42 requested changes Dec 27, 2025

View reviewed changes

rajvi0106 requested a review from kelson42 December 27, 2025 21:15

veloman-yunkan requested changes Dec 29, 2025

View reviewed changes

veloman-yunkan requested changes Jan 7, 2026

View reviewed changes

veloman-yunkan requested changes Jan 19, 2026

View reviewed changes

veloman-yunkan reviewed Feb 2, 2026

View reviewed changes

rajvi0106 force-pushed the add-kiwix-logo branch from fc68a76 to 0e2e3f5 Compare February 2, 2026 18:47

Added kiwix logo to OPDS feed

92ba25a

avoid hard coded url and added feed logo file favicon logo used removed a test line Use contentServerURL to build absolute OPDS logo/icon links fix logo url so it renders in browser and readers use of absolute url

rajvi0106 force-pushed the add-kiwix-logo branch from 0e2e3f5 to 92ba25a Compare February 2, 2026 18:49

veloman-yunkan requested changes Feb 3, 2026

View reviewed changes

veloman-yunkan mentioned this pull request Feb 5, 2026

Add Kiwix logo to OPDS feed #976

Open

Uh oh!

Conversation

rajvi0106 commented Dec 24, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kelson42 left a comment

Choose a reason for hiding this comment

Uh oh!

rajvi0106 commented Dec 27, 2025

Uh oh!

veloman-yunkan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kelson42 commented Jan 3, 2026

Uh oh!

rajvi0106 commented Jan 4, 2026

Uh oh!

rajvi0106 commented Jan 7, 2026

Uh oh!

veloman-yunkan left a comment

Choose a reason for hiding this comment

Uh oh!

rajvi0106 commented Jan 10, 2026

Uh oh!

veloman-yunkan commented Jan 12, 2026

Uh oh!

rajvi0106 commented Jan 17, 2026

Uh oh!

veloman-yunkan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajvi0106 Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajvi0106 commented Feb 2, 2026

Uh oh!

veloman-yunkan left a comment

Choose a reason for hiding this comment

Uh oh!

veloman-yunkan commented Feb 2, 2026

Uh oh!

veloman-yunkan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajvi0106 commented Feb 5, 2026

Uh oh!

veloman-yunkan commented Feb 5, 2026

Uh oh!

rajvi0106 commented Feb 5, 2026

Uh oh!

veloman-yunkan commented Feb 5, 2026

Uh oh!

rajvi0106 Jan 21, 2026 •

edited

Loading

rajvi0106 commented Feb 5, 2026 •

edited

Loading

veloman-yunkan commented Feb 5, 2026 •

edited

Loading