-
Notifications
You must be signed in to change notification settings - Fork 421
Fix Mastodon URL previews not showing anything useful #19231
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
base: develop
Are you sure you want to change the base?
Fix Mastodon URL previews not showing anything useful #19231
Conversation
4badeb5 to
308ceff
Compare
|
I'm pretty sure this regresses another site, maybe Twitter? I forgot why we started combining them like that. |
I'll try a few more sites and see what changes, I'm also worried about regressing some site I don't know about :/ |
reivilibre
left a comment
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.
I don't personally use this feature, but your screenshots look like improvements to me.
I can see this being worse for some URLs but I don't know which ones and I'm not sure how we decide which ones are best to favour.
| # information from the HTML and overlaying any information | ||
| # from the oEmbed response. | ||
| og = {**og_from_html, **og_from_oembed} | ||
| og = og_from_oembed | og_from_html |
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.
maybe at this point it would be good to add a comment explaining why this order matters (otherwise I'm half worried someone will come and flip-flop it the other way in a couple of months when another site is better the other way around).
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.
Done, it now says why the order matters - not only the type of site but also the two examples shown off in the PR.
Fixes element-hq#18444. Inside of UrlPreviewer, we need to combine two dicts (one from oEmbed, and one from HTML) and in Mastodon's case they were very different. The one from HTML is basically useless, due to it being a Javascript application. But the oEmbed one has the actual post content, yet the information from HTML was preferred. So I flipped which dictionary overlays which, so keys from oEmbed is preferred over the extracted HTML ones. This seems to be the original intention judging by the comment. I also updated to the newer Python synax for merging dictionaries.
308ceff to
cfa64f5
Compare
|
This seems related to #17462. Do you think it will help with that issue as well, or is that an additional problem? |
It looks like it does help that issue indeed, the buggy behavior matches up with what I see now. But with this patch it looks correct (see the YT section above) |








Fixes #18444. Inside of UrlPreviewer, we need to combine two dicts (one from oEmbed, and one from HTML) and in Mastodon's case they were very different. The one from HTML is basically useless, due to it being a Javascript application. But the oEmbed one has the actual post content, yet the information from HTML was preferred.
So I flipped which dictionary overlays which, so keys from oEmbed is preferred over the extracted HTML ones. This seems to be the original intention judging by the comment. I also updated to the newer Python synax for merging dictionaries.
Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.