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

Implement emoji import feature #5145

Closed
wants to merge 4 commits into from
Closed

Conversation

akihikodaki
Copy link
Contributor

@akihikodaki akihikodaki commented Sep 29, 2017

shortcode as identifier of custom emojis is a barrier when implementing emojis shared by multiple instances because it is not global identifier.
This is a part of a change suggested at #5107. It is not tested well yet.

@akihikodaki akihikodaki added the activitypub Protocol-related changes, federation label Sep 29, 2017
@akihikodaki akihikodaki mentioned this pull request Sep 29, 2017
2 tasks
@akihikodaki
Copy link
Contributor Author

01a406c892247738936b6c02eab2751d0cbcd2b6, 71b65493abb59609defc74952461f8472a5c21be: irrelevant but later specs are dependent on them. A commiter may cherry-pick them earlier.
9533624bfb1d9b1d51990a5f751449e85e0141b5: spec emoji import feature

I'm requesting a review from @Gargron, who implemented custom emoji and suggested emoji copy feature.

shortcode as identifier of custom emojis is a barrier when implementing
emojis shared by multiple instances because it is not global identifier.
@beatrix-bitrot
Copy link
Contributor

:/

I think it's bit large and complex for implementing the emoji copy feature. Can we not just copy the record for a custom emoji but change the domain so it is local? That should allow it to be used by local users and achieve what we need right?

@akihikodaki
Copy link
Contributor Author

I decided to track the origin:

  • to cache
  • to avoid or to make it easy to resolve potential copyright disputes
  • to block

@akihikodaki
Copy link
Contributor Author

I have reviewed the change and I think the complexity is not excessive. Though the number of lines: 1210 addition looks quite a huge, but the large portion is spec (even including specs for existing code to make sure it does not break.) The line number of this pull request is less than 500 lines. When compared to #5231, it is increase of < 200 lines.

So it is whether the three features I mentioned worth 200 lines. I think it is, and probably those for #5239 think so too.

@Gargron
Copy link
Member

Gargron commented Oct 6, 2017

Comments:

  • I don't like the CustomEmoji <-> CustomEmojiIcon separation in the database. Too complicated!
  • Admin UI changes not needed in my opinion
  • The ActivityPub emoji serializer seems to be missing the "id" attribute

If you really insist on a more object-oriented emoji representation, I suggest this:

{
  @context: { ... },
  id: 'https://uri/of/emoji',
  name: 'shortcode here',
  updated: 'so we know if we need to re-cache',
  icon: {
    type: 'Image',
    mediaType: 'image/png',
    url: 'http://url/to/file.png'
  }
}

And honestly this would keep changes minimal, I think, because on the processing side we'd only need to change taking the "href" attribute to taking the icon->"url" attribute, and additionally checking "updated"

I can do this today, I think.

@akihikodaki
Copy link
Contributor Author

@Gargron Thank you for your suggestion. I appreciate the simplicity.
However shortcode should be changable to avoid collision when copying. That's why I did so.

Apart from that, if you think it is fine and go with the your idea, you can make even simpler by directly putting the URL to image to icon property. I hope we can do similar keeping shortcode renaming...

@akihikodaki
Copy link
Contributor Author

Now I have time to reply other comments so here are my answers:

@akihikodaki
Copy link
Contributor Author

The ActivityPub emoji serializer seems to be missing the "id" attribute

I also note that it should not have the attribute because it is a Link.

@Gargron
Copy link
Member

Gargron commented Oct 6, 2017

Well, a Link cannot have an icon, so... I just turned into an Object in my other PR.

@akihikodaki
Copy link
Contributor Author

I got it.

@akihikodaki
Copy link
Contributor Author

I wonder whether I can simply put it to href. It means there could be different Links which refers to the same object. (shortcode differs so the Link is not exactly same, but href is same.) Anyway the spec does not prohibit so it may not matter. Also, I have to reconsider ActivityPub representation if #5231 gets merged.

@akihikodaki
Copy link
Contributor Author

I'll close this issue. I'm no longer interested in the feature because the emoji feature landed with 2.0.0 and there is no way to implement but adding a different protocol.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
activitypub Protocol-related changes, federation moderation Administration and moderation tooling new user experience Features for attracting and onboarding new users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants