Add functions which return displayable addresses #1234

vighnesh-sawant · 2025-10-26T18:37:22Z

Fixes kiwix/kiwix-tools#764
Reference ->kiwix/kiwix-tools#763

vighnesh-sawant · 2025-10-26T18:38:02Z

@kelson42 Please do have a look at the two pull requests

vighnesh-sawant · 2025-10-26T19:35:06Z

Is this fine? It works well on kiwix-serve and kiwix-desktop.
I'll apply this to the remaining platforms soon!

Copilot

Pull Request Overview

This PR adds functionality to return displayable HTTP addresses (URLs) from the server, supporting both IPv4 and IPv6 formats. The implementation includes proper URL formatting with port numbers and root prefixes.

Key Changes:

Added getRootPrefixOfDecodedURL() getter to InternalServer class
Implemented getDisplayableAddress() and getDisplayableAddress6() methods in Server class for formatted URL output

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File	Description
src/server/internalServer.h	Added getter method for accessing the root prefix of decoded URLs
src/server.cpp	Implemented IPv4 and IPv6 displayable address formatting methods with fallback logic
include/server.h	Added public method declarations for displayable address retrieval

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

Copilot · 2025-10-27T06:36:40Z

src/server.cpp

+std::string Server::getDisplayableAddress()
+{
+  kiwix::IpAddress addresses = Server::getAddress();   
+  if(addresses.addr.empty()) return getDisplayableAddress6(); //as a fallback return ipv6 address as one of them has to be set


These two methods can create an infinite recursion loop if both addr and addr6 are empty. Add a check to prevent mutual recursion or handle the case where both addresses are empty.

The server would not start right if both addresses are missing? Do i need to handle this?

src/server.cpp

Copilot · 2025-10-27T06:36:41Z

src/server.cpp

+std::string Server::getDisplayableAddress6()
+{
+  kiwix::IpAddress addresses = Server::getAddress();
+  if(addresses.addr6.empty()) return getDisplayableAddress(); //as a fallback return ipv4 address as one of them has to be set


This creates potential for infinite recursion with getDisplayableAddress(). If both IPv4 and IPv6 addresses are empty, the methods will call each other indefinitely.

vighnesh-sawant · 2025-10-27T07:20:47Z

@kelson42 Made the changes please ask copilot to rereview( It does not allow me to do that)

Copilot

Pull Request Overview

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

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

Copilot · 2025-10-27T07:32:01Z

src/server/internalServer.cpp

+std::string InternalServer::getSuffixOfRootUrlWithPort()
+{
+  std::string suffix;
+   if (m_port == 80)


The magic number 80 should be defined as a named constant (e.g., HTTP_DEFAULT_PORT) to improve code clarity and maintainability.

Copilot · 2025-10-27T07:32:01Z

src/server.cpp


+std::string Server::getDisplayableAddress()
+{
+  kiwix::IpAddress addresses = Server::getAddress();   


Trailing whitespace should be removed from this line.

Suggested change

kiwix::IpAddress addresses = Server::getAddress();

kiwix::IpAddress addresses = Server::getAddress();

Copilot · 2025-10-27T07:32:01Z

src/server.cpp

+std::string Server::getDisplayableAddress()
+{
+  kiwix::IpAddress addresses = Server::getAddress();   
+  if(addresses.addr.empty()) return getDisplayableAddress6(); //as a fallback return ipv6 address as one of them has to be set


These mutual fallback calls between getDisplayableAddress() and getDisplayableAddress6() create infinite recursion when both addr and addr6 are empty. Add a check to prevent this or return an error/empty string when both addresses are unavailable.

Copilot · 2025-10-27T07:32:01Z

src/server.cpp

+std::string Server::getDisplayableAddress6()
+{
+  kiwix::IpAddress addresses = Server::getAddress();
+  if(addresses.addr6.empty()) return getDisplayableAddress(); //as a fallback return ipv4 address as one of them has to be set


These mutual fallback calls between getDisplayableAddress() and getDisplayableAddress6() create infinite recursion when both addr and addr6 are empty. Add a check to prevent this or return an error/empty string when both addresses are unavailable.

Copilot · 2025-10-27T07:32:02Z

src/server.cpp

+  if(addresses.addr.empty()) return getDisplayableAddress6(); //as a fallback return ipv6 address as one of them has to be set
+
+  std::string result = "http://";
+  std::string suffix=mp_server->getSuffixOfRootUrlWithPort();


Missing space around the assignment operator. Should be std::string suffix = mp_server->getSuffixOfRootUrlWithPort(); for consistency with coding style.

Copilot · 2025-10-27T07:32:02Z

src/server.cpp

+  if(addresses.addr6.empty()) return getDisplayableAddress(); //as a fallback return ipv4 address as one of them has to be set
+
+  std::string result = "http://";
+  std::string suffix=mp_server->getSuffixOfRootUrlWithPort();


Missing space around the assignment operator. Should be std::string suffix = mp_server->getSuffixOfRootUrlWithPort(); for consistency with coding style.

Suggested change

std::string suffix=mp_server->getSuffixOfRootUrlWithPort();

std::string suffix = mp_server->getSuffixOfRootUrlWithPort();

codecov · 2025-10-27T07:33:30Z

Codecov Report

❌ Patch coverage is 0% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 42.84%. Comparing base (dd09e3c) to head (361c6ce).

Files with missing lines	Patch %	Lines
src/server.cpp	0.00%	15 Missing ⚠️
src/server/internalServer.cpp	0.00%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1234      +/-   ##
==========================================
- Coverage   43.05%   42.84%   -0.21%     
==========================================
  Files          60       60              
  Lines        4734     4757      +23     
  Branches     2492     2505      +13     
==========================================
  Hits         2038     2038              
- Misses       1075     1098      +23     
  Partials     1621     1621

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

vighnesh-sawant · 2025-10-27T12:09:14Z

@kelson42
I think the PR should be ready now

Copilot

Pull Request Overview

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

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

veloman-yunkan · 2025-10-27T11:52:02Z

include/server.h

+       std::string getDisplayableAddress();
+       std::string getDisplayableAddress6();


These methods can (and, therefore, should) be declared const

I would name these functions getServerAccessUrl() and getServerAccessIPv6Url(). BTW, maybe it's better to have a single public function std::vector<std::string> getServerAccessUrls() const? @kelson42

veloman-yunkan · 2025-10-27T12:02:56Z

src/server.cpp

+  kiwix::IpAddress addresses = Server::getAddress();   
+  if(addresses.addr.empty()) return getDisplayableAddress6(); //as a fallback return ipv6 address as one of them has to be set
+
+  std::string result = "http://";


This variable is not justified. return "http://" + addresses.addr + suffix; is quite readable.

veloman-yunkan · 2025-10-27T12:07:14Z

src/server.cpp


+std::string Server::getDisplayableAddress()
+{
+  kiwix::IpAddress addresses = Server::getAddress();


I advise you to start practising defensive programming. A powerful technique is to declare const whatever can be declared const, which protects you against carelessly modifying things that are supposed to be immutable.

In particular, both addresses and suffix in this function can be declared const.

veloman-yunkan · 2025-10-27T12:20:28Z

src/server/internalServer.cpp

+std::string InternalServer::getSuffixOfRootUrlWithPort()
+{
+  std::string suffix;
+   if (m_port == HTTP_DEFAULT_PORT)
+  {
+    suffix = m_rootPrefixOfDecodedURL;
+  } else
+  {
+    suffix = ":" + std::to_string(m_port) + m_rootPrefixOfDecodedURL;
+  }
+  return suffix;
+}


Please replace this member function of InternalServer with a free function in an unnamed namespace in server.cpp std::string makeServerUrl(std::string host, int port, std::string root).

vighnesh-sawant · 2025-10-27T14:34:17Z

Incorporated all changes.
i have kept the httpPrefix variable since the other alternative will be to convert "[" to std::string
what is better?
httpPrefix + "[" + ....
"http://" + std::string("[") .....

veloman-yunkan · 2025-10-27T14:26:08Z

src/server.cpp

+  std::vector<std::string> serverAccessUrls;
+  const kiwix::IpAddress addresses = Server::getAddress();
+  const int port = Server::getPort();
+  const std::string root = mp_server->getRootPrefixOfDecodedURL();


This is available in Server as m_root. So there is no need for the getRootPrefixOfDecodedURL() accessor in InternalServer.

m_root does not get normalized completely
m_rootPrefixOfDecodedURL of internal server is normalized and decoded
m_root of internalserver gets encoded

I see. I was lost in the multitude of root and m_root variables.

Regarding normalization of root location - Server::setRoot() performs some steps in that direction but since it contains a loophole InternalServer has to correct. So I think that the code will be simpler if normalization is fully performed in Server::setRoot() and InternalServer receives a fully normalized value. Please make this change in a separate refactoring commit.

veloman-yunkan · 2025-10-27T14:28:00Z

src/server.cpp

+  const kiwix::IpAddress addresses = Server::getAddress();
+  const int port = Server::getPort();


Why don't you use m_addr and m_port directly?

m_addr of server class may be left empty so it wont work correctly
m_addr of internal server gets initialized correctly thats why
m_port works but i think its better they both are same

You are right regarding m_addr. This is a pitfall (I tell you so after emerging from it with your help 😃 ). It would be better if Server::m_addr and InternalServer::m_addr don't diverge. Please update Server::m_addr in Server::start() and make Server::getAddress() a simple accessor. Let this be another refactoring commit. Then you can use m_addr and m_port directly in this function.

src/server.cpp

vighnesh-sawant · 2025-10-27T19:13:40Z

Please look at the replies to the comments!
Thank you @veloman-yunkan for guiding me, I learnt a lot. The code looks so much better.
After this I'll go and improve this #1229 also

veloman-yunkan

Please improve the PR by adding the two refactoring changes proposed in my replies to your clarifications in the previous review. There should be three commits:

Server::m_addr stays in sync with InternalServer::m_addr
Server::m_root is fully normalized
Server::getServerAccessUrls()

vighnesh-sawant · 2025-10-30T18:08:53Z

Made the three commits as you suggested!

veloman-yunkan

We are almost there. Please remove the commit that sneaked into this PR from #1229 and address the last two minor issues.

veloman-yunkan · 2025-10-31T07:12:25Z

src/server.cpp

+  const kiwix::IpAddress addresses = m_addr;
+  const int port = m_port;
+  const std::string root = m_root;


Now these local variables are not justified. Use m_addr, m_port and m_root below directly.

veloman-yunkan · 2025-10-31T07:23:03Z

src/server.cpp

+  return result;
 }
+
+}


I am bringing to your attention that GitHub's diff visualization makes it conspicuous that the last line here doesn't end with a new-line symbol. Let's pay our tribute to all the man-hours invested in implementing that feature by addressing this otherwise invisible formatting issue.

kelson42 · 2025-10-31T11:57:21Z

@vighnesh-sawant If fixed quickly, glad to ship it in next libkiwix realease. Only a few hours left.

vighnesh-sawant · 2025-10-31T12:47:49Z

@kelson42 Addressed the issues

kelson42 · 2025-10-31T12:49:23Z

@vighnesh-sawant Thx! Waiting @veloman-yunkan's approval and will be merged.

kelson42 · 2025-10-31T13:55:30Z

Merging, so I guess we can move on the PR at the reader level now.

vighnesh-sawant mentioned this pull request Oct 26, 2025

Display better message when using default port for kiwix-server kiwix/kiwix-desktop#1417

Merged

kelson42 requested review from Copilot, kelson42, rgaudin and veloman-yunkan October 27, 2025 06:35

Copilot AI reviewed Oct 27, 2025

View reviewed changes

vighnesh-sawant force-pushed the standard-port-enhancment branch from 36c5df8 to 361c6ce Compare October 27, 2025 07:17

kelson42 requested a review from Copilot October 27, 2025 07:30

Copilot AI reviewed Oct 27, 2025

View reviewed changes

rgaudin mentioned this pull request Oct 27, 2025

Clarify Copilot usage kiwix/overview#128

Open

vighnesh-sawant force-pushed the standard-port-enhancment branch from 361c6ce to 41d8d9b Compare October 27, 2025 12:05

kelson42 requested a review from Copilot October 27, 2025 12:10

Copilot AI reviewed Oct 27, 2025

View reviewed changes

veloman-yunkan requested changes Oct 27, 2025

View reviewed changes

veloman-yunkan mentioned this pull request Oct 27, 2025

Improve message when server is running on standard port kiwix/kiwix-tools#763

Merged

vighnesh-sawant force-pushed the standard-port-enhancment branch 3 times, most recently from 166db82 to 91efcdc Compare October 27, 2025 14:30

vighnesh-sawant requested a review from veloman-yunkan October 27, 2025 14:32

veloman-yunkan requested changes Oct 27, 2025

View reviewed changes

veloman-yunkan reviewed Oct 27, 2025

View reviewed changes

src/server.cpp Outdated Show resolved Hide resolved

veloman-yunkan reviewed Oct 27, 2025

View reviewed changes

src/server.cpp Outdated Show resolved Hide resolved

vighnesh-sawant force-pushed the standard-port-enhancment branch 2 times, most recently from ea9f93d to ce2c929 Compare October 27, 2025 18:11

vighnesh-sawant force-pushed the standard-port-enhancment branch 3 times, most recently from f549764 to 200bf7a Compare October 27, 2025 18:59

vighnesh-sawant requested a review from veloman-yunkan October 28, 2025 18:05

vighnesh-sawant force-pushed the standard-port-enhancment branch from 200bf7a to 9dd0853 Compare October 28, 2025 18:15

veloman-yunkan requested changes Oct 30, 2025

View reviewed changes

vighnesh-sawant force-pushed the standard-port-enhancment branch 2 times, most recently from 7aada0c to 5914ecb Compare October 30, 2025 18:07

vighnesh-sawant requested a review from veloman-yunkan October 30, 2025 18:08

veloman-yunkan requested changes Oct 31, 2025

View reviewed changes

vighnesh-sawant force-pushed the standard-port-enhancment branch 3 times, most recently from 10df8ce to c238181 Compare October 31, 2025 12:38

vighnesh-sawant added 3 commits October 31, 2025 18:11

Sync m_addr of Server and InternalServer

cffca3a

Normalize m_root in Server itself

c2df0a9

Implement a function which returns server access url

4928509

vighnesh-sawant force-pushed the standard-port-enhancment branch from c238181 to 4928509 Compare October 31, 2025 12:41

vighnesh-sawant requested a review from veloman-yunkan October 31, 2025 12:47

kelson42 added this to the 14.1.0 milestone Oct 31, 2025

veloman-yunkan approved these changes Oct 31, 2025

View reviewed changes

kelson42 merged commit 17f0ad2 into kiwix:main Oct 31, 2025
13 checks passed

vighnesh-sawant mentioned this pull request Oct 31, 2025

Switch to using function provided by libkiwix kiwix/kiwix-apple#1349

Closed

kelson42 mentioned this pull request Nov 4, 2025

Bind server->getServerAccessUrls() kiwix/java-libkiwix#125

Closed

	kiwix::IpAddress addresses = Server::getAddress();
	kiwix::IpAddress addresses = Server::getAddress();

	std::string suffix=mp_server->getSuffixOfRootUrlWithPort();
	std::string suffix = mp_server->getSuffixOfRootUrlWithPort();

		std::string getDisplayableAddress();
		std::string getDisplayableAddress6();

		const kiwix::IpAddress addresses = Server::getAddress();
		const int port = Server::getPort();

Uh oh!

Add functions which return displayable addresses #1234

Add functions which return displayable addresses #1234

Uh oh!

Conversation

vighnesh-sawant commented Oct 26, 2025

Uh oh!

vighnesh-sawant commented Oct 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vighnesh-sawant commented Oct 26, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

vighnesh-sawant commented Oct 27, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 27, 2025

Codecov Report

Uh oh!

vighnesh-sawant commented Oct 27, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

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!

Choose a reason for hiding this comment

Uh oh!

vighnesh-sawant commented Oct 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vighnesh-sawant Oct 27, 2025 • 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!

vighnesh-sawant commented Oct 26, 2025 •

edited

Loading

vighnesh-sawant Oct 27, 2025 •

edited

Loading

vighnesh-sawant Oct 27, 2025 •

edited

Loading