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 Twitter embeds #57

Merged
merged 16 commits into from
May 12, 2024
Merged

Conversation

irystocratanon
Copy link

Twitframe has been broken for a long time now thanks to elon and Twitter's own embeds are also somewhat unreliable and don't work for NSFW tweets, etc.

Replace it with FxTwitter

Currently they don't have an iframe embed but they do have a simple JSON API that can be used instead.

FixTweet/FxTwitter#774

Twitframe has been broken for a long time now thanks to elon and
Twitter's own embeds are also somewhat unreliable and don't work for
NSFW tweets, etc.

Replace it with FxTwitter

Currently they don't have an iframe embed but they do have a simple JSON
API that can be used instead.

FixTweet/FxTwitter#774
Copy link
Owner

@TuxedoTako TuxedoTako left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's nice to see a pull request instead of an issue, so I'm sorry to disappoint.

  • I personally have not noticed problems with the twitframe:

    msedge_udjnWHLbW3

    It might be because I don't usually browse boards with links to nsfw tweets. Maybe you can provide some examples that don't load?

  • This implementation lacks some things: the one tweet I tested was a quote tweet, the current twitframe shows the quoted tweet, but this doesn't, despite the API returning the data.

  • Since a lot of twitter trouble is related to rate limits, do we have permission to direct traffic to https://api.fxtwitter.com? Does it have a licence?

src/Linkification/Embedding.js Outdated Show resolved Hide resolved
src/Linkification/Embedding.js Outdated Show resolved Hide resolved
src/Linkification/Embedding.js Outdated Show resolved Hide resolved
src/Linkification/Embedding.js Outdated Show resolved Hide resolved
@irystocratanon
Copy link
Author

It's nice to see a pull request instead of an issue, so I'm sorry to disappoint.

* I personally have not noticed problems with the twitframe:
  It might be because I don't usually browse boards with links to nsfw tweets. Maybe you can provide some examples that don't load?

It happens when signed out of Twitter a lot.

* This implementation lacks some things: the one tweet I tested was a quote tweet, the current twitframe shows the quoted tweet, but this doesn't, despite the API returning the data.

I should be able to fix that. I'll look into it. Another thing I don't think is handled right now are polls but that data should be in the API too I think.

* Since a lot of twitter trouble is related to rate limits, do we have permission to direct traffic to `https://api.fxtwitter.com`? Does it have a licence?

The license is here:
https://github.com/FixTweet/FxTwitter/blob/main/LICENSE.md

I think that is only for the code but they have docs here:
https://docs.fxtwitter.com/en/latest/api/about.html

They encourage use of their API.

@irystocratanon
Copy link
Author

It handles quotes now. One thing I'm not sure about is nested quotes (a quote that quotes a quote that quotes a quote). I could handle that if the API has the data and I can find a tweet to test with.

@TuxedoTako
Copy link
Owner

It happens when signed out of Twitter a lot.

I don't even have a twitter account.

I think that is only for the code but they have docs here:
https://docs.fxtwitter.com/en/latest/api/about.html

They encourage use of their API.

That does indeed look like permission.

Escape more variables (I think I got them all this time)
Replace Emoji with SVG icons
Call Linkify to process links (In theory we can also run the embedder to
to embed thing like YouTube videos but that might be overkill and I
don't know how to do that)
Unfortunately this is still required for videos
* Add a preference to control whether or not to translate Tweets
* I'm going to restore the old code in another commit to make this new
  code optional in case somebody wants to keep using twitframe
Images/Videos is not the right place for this
I iterated the wrong way so replies were displayed incorrectly
@TuxedoTako
Copy link
Owner

I'm sensing scope creep. You're adding features after you already opened the pull request. I don't know where to test the reply resolver, which the original twitframe didn't have in the first place. But I already found an issue with the translation: if it's on a quote tweet, it's displayed as: quote tweet -> quoted tweet -> quoted translation -> quote translation.

@irystocratanon
Copy link
Author

irystocratanon commented May 8, 2024

I don't know where to test the reply resolver

It's for threads. It didn't show any replies (the history, what it's in reply to) before and arguably doesn't need to (which is why I made it configurable) but this can be useful for context.

it's displayed as: quote tweet -> quoted tweet -> quoted translation -> quote translation.

Do you mean the quote translation should be displayed first? That shouldn't be too difficult.

@TuxedoTako
Copy link
Owner

It's for threads. It didn't show any replies (the history, what it's in reply to) before and arguably doesn't need to (which is why I made it configurable) but this can be useful for context.

Do you have a thread where I can test this? I want to test before I merge the PR.

it's displayed as: quote tweet -> quoted tweet -> quoted translation -> quote translation.

Do you mean the quote translation should be displayed first? That shouldn't be too difficult.

I mean the translations should be grouped with the original text. Now the original tweet is sandwiched between the quote and its translation.

@TuxedoTako
Copy link
Owner

I haven't found a tweet for 'Resolve all Tweet Replies', but I'm impatient, so I'm merging anyway.
I will move the templating to jsx, in case we missed an escape. I fixed the translation order myself.

@TuxedoTako TuxedoTako merged commit 098c521 into TuxedoTako:project-XT May 12, 2024
@TuxedoTako TuxedoTako mentioned this pull request May 13, 2024
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.

None yet

2 participants