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

Encode custom emojis as resolveable objects in ActivityPub #5243

Merged
merged 2 commits into from Oct 7, 2017

Conversation

@Gargron
Copy link
Member

commented Oct 6, 2017

Within master this is a breaking change. Instead of encoding custom emojis as Link entities with href pointing to the static file, they are now encoded as Object entities with an icon property that points to the static file:

{
  @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'
  }
}
@Gargron Gargron added the activitypub label Oct 6, 2017
@akihikodaki

This comment has been minimized.

Copy link
Collaborator

commented Oct 6, 2017

Sorry I was replying late, but it is not what was intended in #5145.
The form of the representation does not matter at all. The intension is:

  • to keep the sole, unique identifier to the Image object and
  • to allow to edit shortcode to avoid collision when copying for scalability

I should have clarified that also in the first comment of the pull request. My apologies.

@Gargron

This comment has been minimized.

Copy link
Member Author

commented Oct 6, 2017

@akihikodaki If you want to copy an emoji but the shortcode is taken, you can just save the image and upload it like a new emoji. The "copy" function is just a convenience thing. I don't believe that shortcodes should be changeable at all, though!

@akihikodaki

This comment has been minimized.

Copy link
Collaborator

commented Oct 6, 2017

The "copy" function is just a convenience thing.

What do you want to resolve then? If you do not care caching, copyright and domain blocking, you do not need #5145 and probably this.

I don't believe that shortcodes should be changeable at all, though!

Then how should we resolve the collision?

@Gargron

This comment has been minimized.

Copy link
Member Author

commented Oct 6, 2017

If you do not care caching, copyright and domain blocking

Sorry, but domain blocking is already implemented? Emojis don't get downloaded with reject_media. Also, admins can "disable" an individual emoji from admin UI. Regarding caching, this PR starts storing remote file URL and serializes an "updated" attribute; if the file URL is different or if the "updated" timestamp is newer than when we last touched our cache, it does get updated!!

Then how should we resolve the collision?

I am not sure collision of what you are talking about? Collision of local emojis when you want to copy a remote one to a local one?

@akihikodaki

This comment has been minimized.

Copy link
Collaborator

commented Oct 6, 2017

Sorry, but domain blocking is already implemented?

I mean to block the copied emojis depending on the source, just like boosts.

I am not sure collision of what you are talking about? Collision of local emojis when you want to copy a remote one to a local one?

Yes, that is what I am concerning. The collision which happens when there are emojis with same shortcode.

@Gargron Gargron referenced this pull request Oct 6, 2017
14 of 14 tasks complete
@Komic

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2017

neat~
any upgrading guideline in particular?

@cwebber

This comment has been minimized.

Copy link

commented Oct 6, 2017

The updated I assume will be a date? Also I assume these Emoji objects will be fetchable from their id?

Seems fine as an object structure. I'm still not sure how these are used... could you paste an example?

@Gargron

This comment has been minimized.

Copy link
Member Author

commented Oct 6, 2017

@cwebber Yes, updated is a datetime just like published is on a Note. And yes they are resolveable via id. They are used in-place of the Link ones.

@akihikodaki

This comment has been minimized.

Copy link
Collaborator

commented Oct 6, 2017

updated? I think it is irrelevant from #5145. It is misleading to say this is an alternative of the pull request.

Copy link
Collaborator

left a comment

looks good to me w/ small nits

def show
respond_to do |format|
format.json do
render json: @emoji, serializer: ActivityPub::EmojiSerializer, adapter: ActivityPub::Adapter, content_type: 'application/activity+json'

This comment has been minimized.

Copy link
@nightpool

nightpool Oct 7, 2017

Collaborator

line length?

emoji = CustomEmoji.find_by(shortcode: shortcode, domain: @account.domain)

return if !emoji.nil? || skip_download?
return if skip_download? || (!emoji.nil? && emoji.updated_at >= updated && emoji.image_remote_url == image_url)

This comment has been minimized.

Copy link
@nightpool

nightpool Oct 7, 2017

Collaborator

this would probably be clearer as two return statements

This comment has been minimized.

Copy link
@nightpool

nightpool Oct 7, 2017

Collaborator

do we need the emoji.image_remote_url == image_url check? wouldn't the save just be a no-op?

full_asset_url(object.url(:original))
end

def media_type

This comment has been minimized.

Copy link
@nightpool

nightpool Oct 7, 2017

Collaborator

is this new? is there a practical implication to this?

This comment has been minimized.

Copy link
@Gargron

Gargron Oct 7, 2017

Author Member

No, it's just nice

def name
":#{object.shortcode}:"
end
class CustomEmojiSerializer < ActivityPub::EmojiSerializer

This comment has been minimized.

Copy link
@nightpool

nightpool Oct 7, 2017

Collaborator

what's the difference between this and the Emoji serializer? do we need both?

This comment has been minimized.

Copy link
@Gargron

Gargron Oct 7, 2017

Author Member

AMS derives serializer name from class name, virtual_tags is array of various objects, some of which are instances of CustomEmoji, but what we need is called ActivityPub::EmojiSerializer. It's easier to do this than define an AMS method that changes how serializer names are derived.

This comment has been minimized.

Copy link
@nightpool

nightpool Oct 7, 2017

Collaborator

Does the def object_type; emoji; end thing mean this isn't relevant? having a hard time finding the AMS docs on this, sorry.....

@Gargron Gargron merged commit 3a34754 into master Oct 7, 2017
3 checks passed
3 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@Gargron Gargron deleted the feature-activitypub-emojis branch Oct 7, 2017
rutan added a commit to rutan/mastodon that referenced this pull request Oct 11, 2017
…#5243)

* Encode custom emojis as resolveable objects in ActivityPub

* Improve code style
takayamaki added a commit to takayamaki/mastodon that referenced this pull request Oct 12, 2017
…#5243)

* Encode custom emojis as resolveable objects in ActivityPub

* Improve code style
cobodo pushed a commit to cobodo/mastodon that referenced this pull request Oct 20, 2017
…#5243)

* Encode custom emojis as resolveable objects in ActivityPub

* Improve code style
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.