-
-
Notifications
You must be signed in to change notification settings - Fork 889
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
Embed Peertube videos #2261
Embed Peertube videos #2261
Conversation
crates/apub/src/objects/post.rs
Outdated
// TODO: need to generate this in a better way. one option is parsing property "og:video:url" | ||
// of the peertube video page for embed url. but as this is federation code, i think | ||
// it would make sense to federate the embed code | ||
embed_html = Some(r#"<iframe src="https://peertube2.cpy.re/videos/embed/0ee82ed7-2582-4851-856d-9b151c3e243c"></iframe>"#.to_string()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to get the Peertube embed url properly. One option is to parse the site property "og:video:url", but then it will work for all websites, not only Peertube. And as this the federation code, i believe it would make sense to also send the embed url over activitypub. What do you think about this @Chocobozzz?
1d917fb
to
1ac0656
Compare
Chocobozzz (Peertube maintainer) answered this via email:
So if we want to embed Peertube videos, there are two options:
I will go with the second option, as it seems both easier and more useful. |
Updated, this works fine for both Peertube and Youtube in my testing. I changed the field post.embed_html to embed_url, which is more secure as it doesnt allow for arbitrary html, but only iframes. This is a breaking change so should maybe wait with merging. |
dc6583f
to
6c475c3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also run clippy, CI failing.
crates/api_common/src/request.rs
Outdated
.videos | ||
.first() | ||
.map(|v| Url::parse(&v.url).ok()) | ||
.flatten(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is finding the first video, maybe the column name should be embed_video_url
.
Also I didn't pull down this code to check but hopefully this fails gracefully into a None value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the column name. And just tested it with a link that has no video, works fine.
About clippy, unfortunately it doesnt work locally anymore after upgrading to Rust 1.61 because of lint
clippy::to_string_in_displayhas been renamed to
clippy::recursive_format_impl``. And changing that would break ci i believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI is currently broken tho. Maybe you could use rustup to do rustup install 1.59
, and do cargo +1.59 fmt
Fixed clippy. For now i dont think we have any breaking changes in main, so lets wait with merging until after 0.16.4 |
Hrm needs clippy again. |
6f95b83
to
cccb965
Compare
* Use og:video attribute for embeds, change Post.embed_html to embed_url * fix clippy
It seems useful to embed Peertube videos directly in Lemmy, so i gave it a try. Embeds are currently completely disabled on the backend for new posts, but still supported in lemmy-ui so can easily be brought back.
Embeds are currently hidden behind this plus button which is not really obvious. But they do work fine (at least for the current, hardcoded embed url).