meshtastic / Meshtastic-Apple

Apple iOS, iPadOS & macOS Clients For Meshtastic
https://meshtastic.org
GNU General Public License v3.0
162 stars 51 forks source link

๐Ÿž [Bug]: Channels URL for QR Codes Aren't Properly Formatted #752

Closed AddisonTustin closed 2 weeks ago

AddisonTustin commented 2 weeks ago

Firmware Version

latest

What did you do?

The syntax of a URI/URL requires that the fragment (the portion of the URI that follows a #) should be at the very end of the URI. When beginning to work on meshtastic/Meshtastic-Android#953 I noticed that since ?add=true followed the fragment, Java's native URI feature does not properly parse add=true as a query param, and instead includes it as part of the fragment.

There's some cleanup I'd like to do as a part of meshtastic/Meshtastic-Android#953 to use the URI directly rather than doing manual string processing. So this becomes a small blocker.

Expected Behavior

The query params (?add=true) should be included in the URI before the fragment.

Link to URI Generic Syntax (Library of Congress).

Current Behavior

When constructing the channelsUrl ?add=true is appended to the end of the URI.

Participation

Additional comments

I'm willing to submit a PR, but I don't currently have a dev environment setup for iOS application development. So if there are others willing to tackle this before I have an opportunity to, that would be appreciated ๐Ÿ™‚

garthvh commented 2 weeks ago

It is after the hash so it does not get sent to the server. React uses this order as well.

AddisonTustin commented 2 weeks ago

The actual channel configurations are in the fragment to reduce the chance of leaking channel keys, right? Not sure if it's as meaningful a distinction for the query string. Though that may not be a super compelling reason, so happy to close this issue and just process the QR code link appropriately in the Android client if that's the preference here.

garthvh commented 2 weeks ago

I moved it before the hash, I can handle both easily enough, probably a pain in js too

garthvh commented 2 weeks ago

This is in app review