#1425: Use Open Graph to determine video orientation in HTML #1526
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mplorentz
approved these changes
Sep 23, 2024
There was a problem hiding this comment.
This looks great! I had one optional comment. Also I had two concerns that aren't directly related to this PR or ticket but I am reporting them here anyway:
- Should we use OpenGraph metada for images orientation to? At least when we are rendering an image as a preview of an html page. I don't see a ticket for rendering opengraph previews of generic links, is that work not planned?
- ok the other thing is a bug I found that happens with the old media viewer. Will report it separately.
| XCTAssertEqual(mockParser.metadataCallCount, 1) | ||
| } | ||
|
|
||
| func test_fetchMetadata_when_content_is_mp4_does_not_call_parser() async throws { |
There was a problem hiding this comment.
Would it be easy to add a test that the user agent is set correctly? That seems like something that could get lost in a refactor.
I'm open to it, but it looks like we don't have a ticket, probably because I edited others and failed to include a new one to cover this. There's a user story for it as well as a design in Figma, so I'll create a new ticket. |
|
@mplorentz #1539 covers the last part of this. |
|
Merging since this was approved and I made the requested changes. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issues covered
#1425
Description
Maybe the last of #1425. This uses Open Graph to determine video orientation in HTML. Works for things like YouTube videos where there's an HTML document at the URL. For example: https://www.youtube.com/shorts/ZwlBxvnH-So
That's a YouTube short, and it should appear in portrait orientation in Nos because its height is greater than its width, as determined by the
metatags containing the Open Graph properties.We can't do this for things like
note16rs86sh4peyqu6gtppw2y4j88z0nrcmupeefnj5flnwxp3fzdfuqsln02ybecause it contains a direct link to a mp4 video, rather than a link to an HTML document.How to test
note1awmfrttfmuqecqvxhstwpad88n3gk66f6zwy3xq88dnmaw0xqmgsga9q3rnote12vlzgjvluewmf92ylxcuqdtqv70n0u6t5jqykj8u43kysf0kds4qmrwtt2.mp4. Here's one:note14h3u9pseydfpsxzwzqtr3uc6k5th4nhqwtf8fyuc2wpgud6zsvkqmhgdmdvideo/mp4, so there's no Open Graph metadata.Screenshots/Video
Simulator.Screen.Recording.-.iPhone.SE.3rd.generation.-.2024-09-19.at.18.13.19.mp4
Notes
There's some complexity in the logic here. Open Graph is a fallback; if the Nostr event contains
imetatags, those will be used and we won't fetch the Open Graph data. This is all to say: if you want to test this well, make sure your notes don't containimetatags. Nos doesn't postimetatags (yet!), but Damus does. So post your test notes from Nos.