-
Notifications
You must be signed in to change notification settings - Fork 82
Emoji: First pass at support in Interactions #1129
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: trunk
Are you sure you want to change the base?
Conversation
I think before allowing images in comments, we have to think about/implement a proper blocking and moderation tooling (even more if it is about side-loading images): https://www.theverge.com/2023/7/24/23806093/mastodon-csam-study-decentralized-network |
I just pushed an update that moves the replacement into a filter that runs after comment_content and the author have been sanitized. That way, only custom emoji images will be added to the content. |
Would sideloading the image onto the site with
Of course, we'd need to be okay adding potentially a lot of new media to a site once that's enabled. I'm not sure that's okay. |
With custom emoji being shared tags, is that not kind of expected? |
76f5d33
to
47a6493
Compare
This comment was marked as outdated.
This comment was marked as outdated.
@Jiwoon-Kim I appreciate your input and feedback, but comments and issues of this length are not helpful. They generally lack a specific ask or suggestion and are incredibly hard to read. Going forward, please keep comments/issues concise and actionable, like I asked for previously. |
@obenland Thank you for your feedback, and I really appreciate the attention. As someone coming from a non-developer background, I tend to approach these topics from a more experiential and design-oriented perspective. That often leads me to start from broader conceptual ideas before narrowing down to specifics—especially when things are interconnected or have potential design implications that aren't immediately obvious. I understand that this approach can result in comments that feel messy or overwhelming, and I apologize if it made the discussion harder to follow. I added the reference materials at the end to reduce the need for re-investigation, but I realize now that might have come across as overly verbose. Also, since this pull request already exists, I thought discussing the context and UX considerations here would help avoid scattering the conversation into too many isolated issues. But I see now that clarity and conciseness in individual issues is important, so I’ll aim to keep future contributions more structured and actionable. If you ever have time, I’d be grateful for any advice on how to break down larger conceptual suggestions into well-scoped issues. I’m eager to learn and collaborate more effectively with the team. Thanks again! Also, just to explain a bit about my workflow — I usually start with an idea in Korean, then use GPT to help flesh it out and translate it into English. Sometimes I’ll use Gemini or Perplexity to refine the wording too. So I’m not actually writing or editing in English directly most of the time. If this were a Korean-language project, breaking things down into smaller, well-scoped issues like you suggested would honestly be much easier for me. But when working in English, it takes quite a lot more energy for me to carefully read through, translate, and restructure everything. That’s why I sometimes rely on machine translation to double-check and just post the draft as it is. Hope you can understand that part of my process. Anyway — I actually ran your feedback and my original comment through Gemini to auto-organize it a bit, and I’ll drop that summary in a follow-up comment below. |
I do understand, maybe part of your workflow could be to instruct your LLM to use natural language and distill its response to a sentence or two? |
This comment was marked as outdated.
This comment was marked as outdated.
51eeb14
to
ffbce79
Compare
Replace multiple database queries with single batched query and fast lookup arrays. This eliminates N+1 query problem where each emoji required separate database calls. Changes: - Single get_posts() query instead of one per emoji - Fast O(1) lookup arrays instead of O(n) loops - Consolidated meta query building into single loop - Removed redundant array collections
Move temp file cleanup inside success block to prevent cleanup attempts when no file was created. This ensures temp files are properly cleaned up only when download_url() succeeds.
Adds a 10 second timeout to the download_url call when downloading emoji files to prevent long-running requests.
- Use get_comment_author filter instead of comment_author for proper timing - Improve emoji detection to check for class="emoji" instead of just "emoji" - Use proper HTML entity decoding with ENT_QUOTES | ENT_HTML5 flags - Add test coverage for emoji in comment author names
Break up large replace_custom_emoji method into focused single-responsibility methods: - extract_emoji_data(): Parse emoji from activity tags - get_emoji_attachments(): Handle database queries and build lookup arrays - get_or_create_emoji_attachment(): Decision logic for reuse vs download - download_emoji(): Handle file download and WordPress attachment creation - replace_emoji_in_text(): Handle text replacement with HTML Improvements: - Better separation of concerns following SRP - Improved documentation with detailed parameter types - Cleaner alt text (removes colons from emoji names) - Simplified fallback logic using consistent emoji URLs
Moved custom emoji processing logic from Interactions to a new Activitypub\Emoji class for better separation of concerns and maintainability. Updated Interactions and related tests to use the new Emoji class.
Should we sideload emoji images?
Fixes #970.
See https://mastodon.social/@obenland/113788023027390776
See https://obietester.blog/2025/01/06/99/#comment-31
Proposed changes:
Other information:
Testing instructions:
:yikes:
,:AngeryCat:
, etc.