Switch to using function provided by libkiwix #1349
Conversation
vighnesh-sawant
had a problem deploying
to
external
GitHub Actions
Failure
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the address method in KiwixHotspot to use the new getServerAccessUrls() API from the underlying Kiwix C++ library instead of manually constructing the URL from IP address and port components.
Key Changes:
- Simplified the
addressmethod to usegetServerAccessUrls()[0]from the server API - Removed the helper method
portNumberSuffixwhich is no longer needed - Eliminated manual URL construction logic
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| //return the first serverAccessUrl | ||
| return [NSString stringWithUTF8String:self.server->getServerAccessUrls()[0].c_str()]; |
There was a problem hiding this comment.
Accessing index [0] without checking if the array is empty could cause a crash. The method should verify that getServerAccessUrls() returns a non-empty array before accessing the first element, or handle the error case appropriately.
| //return the first serverAccessUrl | |
| return [NSString stringWithUTF8String:self.server->getServerAccessUrls()[0].c_str()]; | |
| // Return the first serverAccessUrl, or nil if none are available | |
| auto urls = self.server->getServerAccessUrls(); | |
| if (urls.size() > 0) { | |
| return [NSString stringWithUTF8String:urls[0].c_str()]; | |
| } else { | |
| return nil; | |
| } |
There was a problem hiding this comment.
Doesnt need to right? It will never be empty
| } else { | ||
| return [NSString stringWithFormat: @":%i", portNumber]; | ||
| } | ||
| //return the first serverAccessUrl |
There was a problem hiding this comment.
The comment should be capitalized and include proper punctuation: 'Return the first server access URL.'
| //return the first serverAccessUrl | |
| // Return the first server access URL. |
There was a problem hiding this comment.
umm i prefer the first comment
There was a problem hiding this comment.
Thank you @vighnesh-sawant
I have tested it, and it works OK.
I would prefer to play it safe, and not assume the value is always there (never empty array).
Which lead me to actually try what would happen if it really was nil, and as it turns out we cannot start the hotspot in that case, which is fine.
But the button title still needed to be fixed, as it should not change in this scenario (from: "Start Hotspot" to: "Stop Hotspot"), as the hotspot wasn't started really.
TLDR: it's always better to handle these edge cases gracefully for users. If the user experience is that the hotspot cannot be started for some reason, that's a lot better. The user can still do other things in the application vs crashing the app.
Please see my changes here:
|
This is blocked by #1350 |
|
We can close this in favor of: #1353 |
3 participants
Reference -> kiwix/libkiwix#1234
Hey since I don't have a apple device. Would need someone to test this changes.