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

[Feature Request]: Add link embeds in ATProtoBluesky #41

Closed
MasterJ93 opened this issue Oct 25, 2024 · 26 comments
Closed

[Feature Request]: Add link embeds in ATProtoBluesky #41

MasterJ93 opened this issue Oct 25, 2024 · 26 comments
Assignees
Labels
new feature New feature or request

Comments

@MasterJ93
Copy link
Owner

Summary

Support for links should be embedded in the ATProtoBluesky class.

Pain points

At the moment, link embeds are not available to be made when using ATProtoBluesky.createPostRecord.

Considered Alternatives

No response

Is this a breaking change?

No

Library Examples

No response

Additional Context

The question at the moment is how to grab this information. The ultimate solution would probably be to make it so that the method responsible for implementing the embedded link to request this information. From what I understand so far, the following is required to be grabbed:

  • The URL of the link.
  • The title of the link.
  • The description of the link.
  • The thumbnail of the link.

There may be services that can grab this information, or the client could be responsible with getting the link. Additionally, I could pay for a VPS dedicated for ATProtoKit clients to grab the links. If the latter happens, then this program should be made open source, but the VPS would be available for anyone using ATProtoKit. The question is how to ensure that the client is being used by ATProtoKit and not by any other client that wishes to get that information. This would be a mid-term goal.

@MasterJ93 MasterJ93 added the new feature New feature or request label Oct 25, 2024
@aaronvegh
Copy link
Contributor

May I suggest a Swift Package to do the heavy lifting here?

https://github.com/pzmudzinski/OpenGraphReader

MIT licensed, and doesn't have any platform specific dependencies, so you should still be able to deploy to Linux. I think you want to avoid relying on a VPS-type situation here as much as possible!

@MasterJ93
Copy link
Owner Author

I'm curious to hear your thoughts as to why it would be best to avoid making a VPS solution. Are there any security concerns that I would be too annoying to deal with? Are there privacy concerns that I may need to worry about? Or is there something else I haven't considered?

If it's due to simply making sure non-ATProtoKit clients aren't using the VPS, then that is a valid point, but it's something I'm willing to try to solve. But if there's something I haven't considered, then I'm okay with hearing the issues surrounding this.

@MasterJ93 MasterJ93 moved this to Todo in ATProtoKit Oct 26, 2024
@aaronvegh
Copy link
Contributor

Unless I'm missing a consideration, the addition of a separate hosted server — with its attendant costs, configuration and maintenance burden — simply to provide website scanning services for one API endpoint in a framework, feels unnecessary. Anyone looking to deploy ATProtoKit who wanted to do website cards would, I feel, be put off by having to stand up a server just for that. And as for you taking on that cost on users' behalf? That feels unfair to you as well. Commercial users of your framework (people like me, though I could never do this) could take advantage of that generosity, if they could avoid the time and cost and bureaucratic headache of putting up a server. The bigger the company using ATProtoKit, the more likely they actually wouldn't be able to pass that hurdle, and the greater the damage to your own server!

Meanwhile, in OpenGraphReader, we have an existing solution that does the necessary work: loads a webpage, scrapes the needed data out of it. The "cost" of that is borne by the application developer, where it belongs.

Hope this helps!

@MasterJ93
Copy link
Owner Author

Okay, I didn't consider that. Fair enough then: I won't do it. I still feel like someone may want to set up a server and, therefore, I'm not opposed to making a small server application still. It just won't be a high or even medium priority.

As for something like OpenGraphReader, this is fine, and I'll keep it in mind.

@MasterJ93
Copy link
Owner Author

This feature has been completed and will be available in version 0.20.0. This issue will now be closed.

@github-project-automation github-project-automation bot moved this from Review to Done in ATProtoKit Oct 29, 2024
@aaronvegh
Copy link
Contributor

Wow, thanks Christopher! But I don't see a PR for this. Can you point to the code that closes this?

@MasterJ93
Copy link
Owner Author

MasterJ93 commented Oct 29, 2024

I didn't make one. It's available in the develop branch. I'll be releasing the update within the next two days.

That said, I'll have to make it more of a habit to create PRs.

@aaronvegh
Copy link
Contributor

@MasterJ93 I note that your feature requires the client to derive the title, description and thumbnail image for the card. I note that even providing empty values for these yields a link card in Bluesky. I can live with this, thank you.

Running the develop branch in my project, I'm encountering a crash when a post includes a hashtag or mention. It appears during posting and seems related to the logging infrastructure:

#3 0x000000010b44aa50 in LoggingSystem.ReplaceOnceBox.ReplaceOnce.replaceUnderlying(:validate:) at /Users/aaron/Library/Developer/Xcode/DerivedData/Croissant-ficcvyeffbcyvshgbfvqjsxfbnkn/SourcePackages/checkouts/swift-log/Sources/Logging/Logging.swift:617
#4 0x000000010b44acd0 in closure #1 in LoggingSystem.ReplaceOnceBox.replace(
:validate:) at /Users/aaron/Library/Developer/Xcode/DerivedData/Croissant-ficcvyeffbcyvshgbfvqjsxfbnkn/SourcePackages/checkouts/swift-log/Sources/Logging/Logging.swift:640
#5 0x000000010b44a81c in closure #1 in LoggingSystem.RWLockedValueBox.withWriteLock<Logging.LoggingSystem.ReplaceOnceBox<@sendable (String, Logging.Logger.MetadataProvider?) -> Logging.LogHandler>.ReplaceOnce>(:) at /Users/aaron/Library/Developer/Xcode/DerivedData/Croissant-ficcvyeffbcyvshgbfvqjsxfbnkn/SourcePackages/checkouts/swift-log/Sources/Logging/Logging.swift:603
#6 0x000000010b453d48 in partial apply for closure #1 in LoggingSystem.RWLockedValueBox.withWriteLock(
:) ()
#7 0x000000010b445524 in ReadWriteLock.withWriterLock<()>(:) at /Users/aaron/Library/Developer/Xcode/DerivedData/Croissant-ficcvyeffbcyvshgbfvqjsxfbnkn/SourcePackages/checkouts/swift-log/Sources/Logging/Locks.swift:273
#8 0x000000010b44a75c in LoggingSystem.RWLockedValueBox.withWriteLock<Logging.LoggingSystem.ReplaceOnceBox<@sendable (String, Logging.Logger.MetadataProvider?) -> Logging.LogHandler>.ReplaceOnce>(
:) at /Users/aaron/Library/Developer/Xcode/DerivedData/Croissant-ficcvyeffbcyvshgbfvqjsxfbnkn/SourcePackages/checkouts/swift-log/Sources/Logging/Logging.swift:602
#9 0x000000010b449c04 in LoggingSystem.ReplaceOnceBox.replace(:validate:) at /Users/aaron/Library/Developer/Xcode/DerivedData/Croissant-ficcvyeffbcyvshgbfvqjsxfbnkn/SourcePackages/checkouts/swift-log/Sources/Logging/Logging.swift:640
#10 0x000000010b449a68 in static LoggingSystem.bootstrap(
:) at /Users/aaron/Library/Developer/Xcode/DerivedData/Croissant-ficcvyeffbcyvshgbfvqjsxfbnkn/SourcePackages/checkouts/swift-log/Sources/Logging/Logging.swift:521
#11 0x000000010aae37a8 in ATProtocolConfiguration.setupLog(::) at /Users/aaron/Library/Developer/Xcode/DerivedData/Croissant-ficcvyeffbcyvshgbfvqjsxfbnkn/SourcePackages/checkouts/ATProtoKit/Sources/ATProtoKit/APIReference/SessionManager/ATProtocolConfiguration.swift:137
#12 0x000000010aae2e38 in ATProtocolConfiguration.init(handle:appPassword:pdsURL:configuration:logIdentifier:logCategory:logLevel:) at /Users/aaron/Library/Developer/Xcode/DerivedData/Croissant-ficcvyeffbcyvshgbfvqjsxfbnkn/SourcePackages/checkouts/ATProtoKit/Sources/ATProtoKit/APIReference/SessionManager/ATProtocolConfiguration.swift:85
#13 0x000000010aae2994 in ATProtocolConfiguration.__allocating_init(handle:appPassword:pdsURL:configuration:logIdentifier:logCategory:logLevel:) ()
#14 0x000000010b6e3f8c in BlueskyService.crossPost(from:replyingTo:) at /Users/aaron/Developer/XPostKit/Sources/XPostKit/SocialService+Bluesky.swift:132

The stack trace originates with my call to setup the configuration:

let config = ATProtocolConfiguration(handle: accountName + "." + serverName, appPassword: password)

Hope you can determine the cause?

@MasterJ93
Copy link
Owner Author

LoggingSystem is from the swift-log package, so there's not much that I can do there. I'll probably have to revert back to the initial version of the package (or at least downgrade to the version that makes the package conform to Sendable). But I'll test it out and see what's going on.

@aaronvegh
Copy link
Contributor

@MasterJ93 I've since determined that the problem here was on my end: you can't initialize multiple instances of the logger, and that's what causes the crash. I have updated my own code to ensure the ATProtocolConfiguration is only initialized once, and that resolves it. I'm quite eager (or more accurately, holy cow my users are the eager ones!) to have this released so I can make it available! 🙏

@MasterJ93
Copy link
Owner Author

MasterJ93 commented Nov 12, 2024

Yes that, true: only one logger can be initialized, but I thought I changed that. Multiple ATProtocolConfiguration instances should be allowed to be initialized, however, so I find that to be strange. Sorry you had to deal with using your own code: I'll need to fix that so you're not out of sync with the rest of the project. I don't want my project users to have to resort to that if they don't need to.

I suppose that, for now, I'll need to make it where, if it sees that a logger instance is already initialized, then it should simply ignore trying to make any form of initialization.

@aaronvegh
Copy link
Contributor

@MasterJ93 Just wanted to see how this work was going? I'm still getting regular complaints about this missing support from my users, so no pressure! 😅 I'm setup on my end to use your new API as soon as it's released...

@MasterJ93
Copy link
Owner Author

MasterJ93 commented Nov 18, 2024

I want to get that completed by either today or tomorrow. It's mostly finishing touches and writing documentation at this point (as well as testing to make sure it works).

@aaronvegh
Copy link
Contributor

@MasterJ93 I'm sorry to be that guy, but hoping for an update here?

@MasterJ93
Copy link
Owner Author

The develop branch is mostly completed and you should (in theory) be able to test and see if it's working. I still have some documentation to work on and there might be one or two things that aren't working. Please let me know if there's anything that needs to change.

@aaronvegh
Copy link
Contributor

The develop branch has other issues, it seems. In VersionNumberPluginExec/main on line 16, the error:

'init(filePath:directoryHint:relativeTo:)' is only available in iOS 16.0 or newer

and line 34, the error:

Cannot find 'Process' in scope

@MasterJ93 MasterJ93 reopened this Nov 21, 2024
@MasterJ93
Copy link
Owner Author

MasterJ93 commented Nov 21, 2024

I'm going to reopen this issue until no other errors are found in relation to link embeds.

As for the additional issue you found, I don't think it's related to link embeds, so please create another issue with respect to that so I can have that specific issue tracked.

@MasterJ93 MasterJ93 moved this from Done to In Progress in ATProtoKit Nov 21, 2024
@MasterJ93 MasterJ93 self-assigned this Nov 21, 2024
@MasterJ93
Copy link
Owner Author

The develop branch has been updated and you should be able to do the following:

  • Use ATLinkBuilder to implement your own way of making the metadata required for creating the external embeds.
  • Automatically have an external embed generated if you don't explicitly use the embed argument.

I haven't tested it yet; I've been awake for the past 36 hours or so. I'm going to bed and I'll continue fixing the warnings, test the implementation of ATLinkBuilder, and fix any bugs it may have. Please let me know if there's any confusion as I haven't written all of the documentation yet. And tell me if it works for you.

@aaronvegh
Copy link
Contributor

I've gotten much further this time! But I'm hitting that old problem with the Logger again. Because I serialize and store account information, an ATProtocolConfiguration object is created whenever I initialize a record from the account storage. When multiple accounts are instantiated, the app crashes because you can't have multiple loggers. I think you mentioned that you were hoping to ensure this type of crash didn't happen? I've done some work in my app to resolve this before, but I'm not quite sure how to best deal with this particular case.

@MasterJ93
Copy link
Owner Author

The dreaded logger... 😓

I have an idea. It's an ugly hack, but it might work: an internal logger property at the top of the main file. If you make a new ATProtocolConfiguration instance, it will be added to the property. If you make another instance, it will simply not be able to override the current logger.

I'll build that tonight and hopefully it kills that problem once and for all.

@MasterJ93
Copy link
Owner Author

MasterJ93 commented Nov 24, 2024

I pushed an update in the develop branch. I basically moved the logger logic into a separate location and ATProtocolConfiguration can make any necessary logging setup calls from there. There's also a counter which ensures that the setup can't happen more than once: if it sees that the value is 1, it will cancel the setup.

There was also a bug that caused ATProtoKit to crash if there were multiple ATProtoKit instances: it seems to be happening when they're trying to add the record lexicons to the registry. That has been fixed as well.

And with that, I am hoping that there will be no more issues on this end.

@MasterJ93 MasterJ93 moved this from In Progress to Review in ATProtoKit Nov 24, 2024
@aaronvegh
Copy link
Contributor

Fantastic! Will have to try this tomorrow; I'll let you know!

@MasterJ93
Copy link
Owner Author

I hope that, by this time tomorrow, baring any bugs, it should be out before this time tomorrow.

I hope...

@aaronvegh
Copy link
Contributor

What is hope, if not a projection out to the universe, wanting it to hold you accountable? What is this comment to you, if not the warm wishes of your number one fan, wondering how likely that release is today? 🙏 🌈

@aaronvegh
Copy link
Contributor

@MasterJ93 for what it's worth, the develop branch code is working perfectly. Incredible to finally see a social card in Bluesky! https://bsky.app/profile/did:plc:vepis2bllryitqskja5fmwiw/post/3lbsrmzxvxg2g

🎉

@MasterJ93
Copy link
Owner Author

I tested it a couple of hours ago and it's working fine on my end as well. I've also written some documentation and a DocC article on how to use it as well.

As for your question:

What is this comment to you, if not the warm wishes of your number one fan, wondering how likely that release is today? 🙏 🌈

In about five minutes: I just pushed everything to main. If GitHub Actions is able to give me a checkmark, I'll create a release and we're all set!

I assume that the other issues you had are no longer existing, so I'll close this, as well as the other ones you've opened, but let me know if that changes.

@github-project-automation github-project-automation bot moved this from Review to Done in ATProtoKit Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request
Projects
Status: Done
Development

No branches or pull requests

2 participants