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

Fix multiple Reddit preview links becoming the same #80

Merged
merged 3 commits into from
Apr 7, 2024

Conversation

ButteredCats
Copy link
Contributor

This fixes #48

The original rewrite_urls function would only run format_url once and then replace all Reddit preview links with that result.

The updated code replaces each match at a time until no more are left, running format_url on each match instead so their all replaced with their respective formatted URL.

One side effect of this is that the text for the link no longer shows https://preview.redd.it/ and instead shows /preview/pre/. Whether this is a bug or a feature is up to you, but personally I like it showing /preview/pre/ better since I know I'm staying on Redlib.

I've basically never worked with Rust and this took me many tries to get a working solution so hopefully it's up to par.

@ButteredCats
Copy link
Contributor Author

After a ton of trial and error I've also managed to make preview links embed within post bodies and comments now, which would fix #25.

Only problem is it builds off the code in this request since I wasn't sure how else to do it. These commits along with that one are at https://github.com/ButteredCats/redlib/tree/fix_multiple_images_and_embedding.

I didn't want to open another pull request for it yet since it includes the same commits as here and I'm not sure if they need to be modified or if this request would get rejected. Sorry if that's the wrong way to go about it, I just didn't want to be submitting potentially redundant pull requests.

@sigaloid
Copy link
Member

sigaloid commented Apr 7, 2024

This looks great. Do you mind writing a test to confirm the new functionality? You should be able to add logging to this function's call site or simply the beginning and end, then visit an affected post and copy-paste the input and output into a new test.

@ButteredCats
Copy link
Contributor Author

Hopefully that's good!

Once this is merged and I finish finding some better sizes for the embedded images on mobile I'll open up a pull request for that.

@sigaloid
Copy link
Member

sigaloid commented Apr 7, 2024

Awesome, thanks!

@sigaloid sigaloid merged commit c86ca16 into redlib-org:main Apr 7, 2024
3 checks passed
@ButteredCats ButteredCats deleted the fix_multiple_images branch April 7, 2024 16:13
@azizLIGHT
Copy link

Thanks for doing this, you're inspiring me to try my hand as well, and I have also never touched Rust either

@ButteredCats
Copy link
Contributor Author

@azizLIGHT No problem! I've found that the effort to try and fix this was a great hands on way to start using Rust so give it a shot! At worst you learn something, at best you get your code merged. A win-win if you ask me.

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.

🐛 Bug Report: Image links in body of post all become the same link
3 participants