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

Move m.* thumbnail_url to be inside info to match m.video #723

Merged
merged 12 commits into from Nov 7, 2016

Conversation

Projects
None yet
2 participants
@NegativeMjark
Contributor

NegativeMjark commented Nov 4, 2016

I've moved the thumbnail_{url,info} inside an "info" key for all types to be consistent with m.room.message#m.video.
I've added the thumbnail_{url,info} keys to the ImageInfo shared between m.room.mesage#image and m.room.avatar.
I've created a new ThumbnailInfo schema to avoid recursion within the ImageInfo schema.

@NegativeMjark NegativeMjark self-assigned this Nov 4, 2016

@richvdh

Shame that this is in info rather than content so that we can't just reuse ImageInfo, but I guess that boat has sailed :(

Other points:

allOf:
- $ref: thumbnail_info.yaml
description: Metadata about the image referred to in ``thumbnail_url``.
title: ThumbnailInfo

This comment has been minimized.

@richvdh

richvdh Nov 7, 2016

Member

I think this is redundant, fwiw, since it's defined in thumbnail_info.yaml

This comment has been minimized.

@NegativeMjark
@@ -0,0 +1,16 @@
$schema: http://json-schema.org/draft-04/schema#

This comment has been minimized.

@richvdh

richvdh Nov 7, 2016

Member

should probably have a type: object

This comment has been minimized.

@NegativeMjark
@@ -0,0 +1,16 @@
$schema: http://json-schema.org/draft-04/schema#
description: Metadata about a thumbnail image.
properties:

This comment has been minimized.

@richvdh

richvdh Nov 7, 2016

Member

are all of the properties optional, or should we specify that some are required?

This comment has been minimized.

@NegativeMjark

NegativeMjark Nov 7, 2016

Contributor

All optional. There are some image infos out there that omit all the fields. I suspect that the thumbnail ones should match.

@@ -20,21 +20,21 @@ properties:
size:
description: The size of the file in bytes.
type: integer
thumbnail_info:

This comment has been minimized.

@richvdh

richvdh Nov 7, 2016

Member

any reason info is before url here and not for m.image? let's be consistent.

@@ -13,4 +13,13 @@ properties:
w:
description: The width of the image in pixels.
type: integer
thumbnail_url:
desciption: The URL to a thumbnail of the image.

This comment has been minimized.

@richvdh

richvdh Nov 7, 2016

Member

"desciption"

@NegativeMjark

This comment has been minimized.

Contributor

NegativeMjark commented Nov 7, 2016

would be nice to update the example for m.image to include a thumbnail

Maybe, I'd prefer it if we could include multiple examples to make it clear that clients should cope with the thumbnail being absent.

@NegativeMjark

This comment has been minimized.

Contributor

NegativeMjark commented Nov 7, 2016

@richvdh I think I've responded to everything PTAL.

@richvdh

This comment has been minimized.

Member

richvdh commented Nov 7, 2016

lgtm

@richvdh

richvdh approved these changes Nov 7, 2016

@richvdh richvdh merged commit aecac4f into master Nov 7, 2016

2 checks passed

Docs (Commit) Build #1942 succeeded in 23 sec
Details
Docs (Merged PR) Build finished.
Details

radiocane added a commit to radiocane/matrix-doc that referenced this pull request Nov 27, 2017

radiocane added a commit to radiocane/matrix-doc that referenced this pull request Nov 27, 2017

radiocane added a commit to radiocane/matrix-doc that referenced this pull request Nov 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment