Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle non-UTF-8 endpoint_name by changing Protobuf field from string to bytes #243

Merged
merged 2 commits into from
Nov 20, 2024

Conversation

SalmonSays
Copy link
Contributor

Changing "endpoint_name" from string to bytes allows "endpoint_name" to carry non-UTF-8 encoded data. I haven't confirmed, but I suspect Windows is something like Windows-1252 encoding. Adding ".into())" wraps the string into a Vec so it gets correctly serialized as bytes to match the changed protobuf definition. This tentatively fixes the other half of #141 .

I haven't tested Linux -> Linux or Linux <-> MacOS compatibility under this new format, but I can confirm that I am now able to successfully send and receive files back and forth between Linux, Android, and Windows freely with the usual/expected behaviors.

Modify endpoint_name from string to bytes
"endpoint_name: Some(sys_metrics::host::get_hostname()?.into()),

consumes the newly formatted String and returns a Vec<u8>
@Martichou
Copy link
Owner

Thanks for looking at this!

I just wonder if the same needs to be done elsewhere (like files names, etc...).
What do you think?

For example:
FileMetadata contains a string name. But this is a File so maybe it is not needed.. I'm not sure :/

@SalmonSays
Copy link
Contributor Author

SalmonSays commented Nov 20, 2024

I definitely had that thought, but stopped shortly after getting it working generally last night.

I just tried playing around with an mp3 file which has embedded Title, Artist, Album, Genre, Year, Track # and Disc # metadata in addition to the usual info like Date Created and Date Modified. I additionally compiled my branch of rquickshare for MacOS and ran it there, alongside my existing Android/Windows/Linux setup. For every combination on my device/platform matrix (both send and receive) I was not able to get the data or metadata of my mp3 to fail in any way, suggesting to me it's not an issue.

Notably I don't have duplicate devices on-hand to test MacOS <-> MacOS, Linux <-> Linux, etc but I have no reason to believe it'd be broken in those specific transfers. If you have any specific files or cases you'd like me to test, please let me know and I'd be happy to do so!

@Martichou
Copy link
Owner

Perfect, if you tested it as you're explaining, that's enough for me :)

Lets merge this PR, and I'll make a release soon after some more tests on my side!
Once again, thanks for checking that out 💯

@Martichou Martichou merged commit 72d6c71 into Martichou:master Nov 20, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants