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

MSC2765: Widget avatars #2765

Merged
merged 4 commits into from
Nov 7, 2020
Merged

MSC2765: Widget avatars #2765

merged 4 commits into from
Nov 7, 2020

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented Sep 3, 2020

@turt2live turt2live changed the title MSC0000: Widget avatars MSC2765: Widget avatars Sep 3, 2020
@turt2live turt2live added kind:feature MSC for not-core and not-maintenance stuff proposal A matrix spec change proposal proposal-in-review labels Sep 3, 2020
@@ -0,0 +1,51 @@
# MSC2765: Widget avatars
Copy link
Contributor

@Sorunome Sorunome Sep 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Widgets aren't in the spec at all yet, a ctrl+f on the unstable c-s api for "widget" or "m.custom" yields nothing. If you are building on an existing MSC, please link it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's specifically not linked due to pending spec pr: #2764

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah great, thank you

@turt2live
Copy link
Member Author

@turt2live
Copy link
Member Author

no one has vehemently opposed this so send this into its next stage of life:

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Oct 1, 2020

Team member @turt2live has proposed to merge this. The next step is review by the rest of the tagged people:

Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for information about what commands tagged team members can give me.

@mscbot mscbot added disposition-merge proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels Oct 1, 2020
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to care about speccing behaviour where somebody sets the avatar url to [] or a number? I imagine we just need to warn clients to be careful?

@turt2live
Copy link
Member Author

That sounds like it's implicitly covered as invalid data.

"type": "m.custom",
"url": "https://example.org/my/widget.html?roomId=$matrix_room_id",
"waitForIframeLoad": true,
"avatar_url": "mxc://example.org/abc123"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, why is this using underscores when the other properties are using camel case?

Copy link
Member Author

@turt2live turt2live Oct 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Historical consistency: when the widget API was introduced years ago it was done as a mix, and this is in line with the perceived standard set forth.

Thought it was located somewhere else. Ideally we'd fix this over time to be all underscored

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair enough I guess.

Co-authored-by: Hubert Chathi <hubert@uhoreg.ca>
## Alternatives

We could define a whole structured system for different thumbnail sizes, though we have a thumbnail
API which can be used to request whatever size is needed by the client.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One consideration is that you can significantly improve the quality of icons by having specific bitmaps for each size, so that extra detail can be included in larger icons but smaller ones can be simple. We don't necessarily need to do this now but since you've said above that avatar_url should look good at 20x20, it implies this avatar is intended to be displayed fairly small, so we could introduce other fields for more detailed icons in the future.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea, this is definitely something for a future MSC. I'd like to solve the general case of client-supplied thumbnails at the same time, possibly for user/room avatars too. Something like "when you display my avatar as 500x500, use this one". Abuse/lying about different avatars would be an interesting problem to solve though.

@mscbot
Copy link
Collaborator

mscbot commented Nov 2, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@mscbot mscbot added final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. and removed proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels Nov 2, 2020
@mscbot
Copy link
Collaborator

mscbot commented Nov 7, 2020

The final comment period, with a disposition to merge, as per the review above, is now complete.

@mscbot mscbot added finished-final-comment-period and removed disposition-merge final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. labels Nov 7, 2020
@turt2live turt2live added spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec and removed finished-final-comment-period labels Nov 7, 2020
@turt2live turt2live merged commit c4d01b9 into master Nov 7, 2020
@turt2live turt2live deleted the travis/msc/widget-avatars branch November 7, 2020 17:52
@turt2live turt2live self-assigned this Nov 13, 2020
@turt2live turt2live mentioned this pull request Nov 23, 2020
6 tasks
@turt2live turt2live added spec-pr-in-review A proposal which has been PR'd against the spec and is in review and removed spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec labels Nov 23, 2020
turt2live added a commit that referenced this pull request Nov 23, 2020
@turt2live turt2live added the widgets anything to do with widgets label Nov 23, 2020
@turt2live turt2live added the blocked Something needs to be done before action can be taken on this PR/issue. label Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Something needs to be done before action can be taken on this PR/issue. kind:feature MSC for not-core and not-maintenance stuff proposal A matrix spec change proposal spec-pr-in-review A proposal which has been PR'd against the spec and is in review widgets anything to do with widgets
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants