Skip to content

MSC4230: Flag for animated images #4230

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

MSC4230: Flag for animated images #4230

wants to merge 14 commits into from

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented Nov 21, 2024 •
edited by turt2live
Loading

Proposes adding a boolean flag to images that indicates whether they're animated.

Impl: element-hq/element-web#28513

Rendered

Disclosure: This is presented wearing my, "Element Employee and Element Web Engineer" hat.

FCP tickyboxes

MSC checklist

@dbkr dbkr mentioned this pull request Nov 21, 2024
Merged
@turt2live turt2live added proposal A matrix spec change proposal client-server Client-Server API kind:feature MSC for not-core and not-maintenance stuff needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels Nov 21, 2024
@turt2live turt2live marked this pull request as draft November 21, 2024 16:24
@dbkr dbkr marked this pull request as ready for review November 21, 2024 17:07
@dbkr dbkr changed the title [Placeholder] Flag for animated images Flag for animated images Nov 21, 2024
@dbkr dbkr changed the title Flag for animated images [MSC4230]: Flag for animated images Nov 21, 2024
@dbkr dbkr changed the title [MSC4230]: Flag for animated images MSC4230: Flag for animated images Nov 21, 2024
sumnerevans
sumnerevans reviewed Nov 21, 2024
View reviewed changes
proposals/4230-animated-image-flag.md Outdated

## Proposal

We add an optional boolean flag, `ia_animated` to the `info` object of image events indicating if
Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify, this field would be allowed in the info on msgtype: m.image and type: m.sticker events?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess just images for now: clarified.

Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason not to allow it for stickers?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not really sure to be honest, which is exactly why I'm leaving it for someone else to propose if they think it's appropriate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Given that stickers are ~renamed images, I agree with Sumner that the same should apply to stickers.

We don't have extensible events yet, but MSC3552 goes in the same direction about the images and stickers relation ship.

Copy link
Member

Choose a reason for hiding this comment

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

+1, let's do it for stickers too.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm afraid I am somewhat drowning with other commitments at the moment. If anyone would like to propose an update to this that would encompass stickers then please go ahead, otherwise I will put this on the back burner for a bit.

MidhunSureshR
MidhunSureshR reviewed Nov 22, 2024
View reviewed changes
@dbkr @MidhunSureshR
Typo
4078173 
Co-authored-by: R Midhun Suresh <rmidhunsuresh@gmail.com>
HarHarLinks
HarHarLinks reviewed Dec 2, 2024
View reviewed changes
Half-Shot
Half-Shot reviewed Dec 6, 2024
View reviewed changes
@dbkr @Half-Shot
Not a string
e550201 
Co-authored-by: Will Hunt <will@half-shot.uk>
@zecakeh zecakeh mentioned this pull request Dec 28, 2024
Merged
@zecakeh
Copy link
Contributor

zecakeh commented Jan 13, 2025

This is implemented in Fractal too as of today:

@nebulade nebulade mentioned this pull request Feb 6, 2025
Open
@dbkr dbkr removed the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Feb 19, 2025
@dbkr
Copy link
Member Author

dbkr commented Feb 19, 2025

As per discussion on element-hq/element-web#28311 and since this now has 2 impls, I'm going to

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Feb 19, 2025 •
edited
Loading

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

Concerns:

  • Checklist incomplete

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 proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. disposition-merge labels Feb 19, 2025
@turt2live
Copy link
Member

turt2live commented Feb 24, 2025 •
edited
Loading

MSCs proposed for Final Comment Period (FCP) should meet the requirements outlined in the checklist prior to being accepted into the spec. This checklist is a bit long, but aims to reduce the number of follow-on MSCs after a feature lands.

SCT members: please check off things you check for, and raise a concern against FCP if the checklist is incomplete. If an item doesn't apply, prefer to check it rather than remove it. Unchecking items is encouraged where applicable.

Checklist:

  • Are appropriate implementation(s)
    specified in the MSC’s PR description?
  • Are all MSCs that this MSC depends on already accepted?
  • For each new endpoint that is introduced:
    • Have authentication requirements been specified?
    • Have rate-limiting requirements been specified?
    • Have guest access requirements been specified?
    • Are error responses specified?
      • Does each error case have a specified errcode (e.g. M_FORBIDDEN) and HTTP status code?
        • If a new errcode is introduced, is it clear that it is new?
  • Will the MSC require a new room version, and if so, has that been made clear?
    • Is the reason for a new room version clearly stated? For example,
      modifying the set of redacted fields changes how event IDs are calculated,
      thus requiring a new room version.
  • Are backwards-compatibility concerns appropriately addressed?
  • Are the endpoint conventions honoured?
    • Do HTTP endpoints use_underscores_like_this?
    • Will the endpoint return unbounded data? If so, has pagination been considered?
    • If the endpoint utilises pagination, is it consistent with
      the appendices?
  • An introduction exists and clearly outlines the problem being solved.
    Ideally, the first paragraph should be understandable by a non-technical audience.
  • All outstanding threads are resolved
    • All feedback is incorporated into the proposal text itself, either as a fix or noted as an alternative
  • While the exact sections do not need to be present,
    the details implied by the proposal template are covered. Namely:
    • Introduction
    • Proposal text
    • Potential issues
    • Alternatives
    • Dependencies
  • Stable identifiers are used throughout the proposal, except for the unstable prefix section
    • Unstable prefixes consider the awkward accepted-but-not-merged state
    • Chosen unstable prefixes do not pollute any global namespace (use “org.matrix.mscXXXX”, not “org.matrix”).
  • Changes have applicable Sign Off from all authors/editors/contributors
  • There is a dedicated "Security Considerations" section which detail
    any possible attacks/vulnerabilities this proposal may introduce, even if this is "None.".
    See RFC3552 for things to think about,
    but in particular pay attention to the OWASP Top Ten.

@turt2live
Copy link
Member

@mscbot concern Checklist incomplete

(I haven't attempted to pre-populate it this time, sorry)

@mscbot mscbot added the unresolved-concerns This proposal has at least one outstanding concern label Feb 24, 2025
Johennes
Johennes reviewed Feb 25, 2025
View reviewed changes
TheArcaneBrony
TheArcaneBrony reviewed Feb 25, 2025
View reviewed changes
richvdh
richvdh reviewed Feb 25, 2025
View reviewed changes
proposals/4230-animated-image-flag.md Outdated

## Proposal

We add an optional boolean flag, `ia_animated` to the `info` object of image events indicating if
Copy link
Member

Choose a reason for hiding this comment

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

+1, let's do it for stickers too.

@richvdh
Copy link
Member

richvdh commented Feb 25, 2025

Other than the nit-picking, this looks good to me, except that I agree with the "let's extend this to stickers" thread.

@Half-Shot Half-Shot mentioned this pull request Mar 21, 2025
Open
@dbkr
Copy link
Member Author

dbkr commented Apr 29, 2025

I think the current state of this is that it's blocked on figuring out what extending it to stickers would involve. However, I have very little knowledge on how stickers work in my head right now, so if anyone would like to chip in with this this would be very welcome. Otherwise, we can consider cancelling it and coming back with a fresh MSC considering both.

HarHarLinks
HarHarLinks suggested changes Apr 29, 2025
View reviewed changes
Copy link
Contributor

@HarHarLinks HarHarLinks left a comment

Choose a reason for hiding this comment

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

I think that is all it takes to make this work for stickers.

proposals/4230-animated-image-flag.md

## Proposal

We add an optional boolean flag, `is_animated` to the `info` object of `m.image` events indicating if
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
We add an optional boolean flag, `is_animated` to the `info` object of `m.image` events indicating if
We add an optional boolean flag, `is_animated` to the `info` object of `m.image` and `m.sticker` events indicating if

Copy link
Member Author

Choose a reason for hiding this comment

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

If we can check this to make sure, this seems fine to me.

dbkr and others added 5 commits June 25, 2025 09:16
@dbkr @richvdh
Typo
82225ea 
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
@dbkr
Hopefully clarify
44824fe
@dbkr
add that we can't tell it's animated
961f9ed
@dbkr
use formal SHOULD
0230fc8
@dbkr
Fix missing words
19d9996
Half-Shot
Half-Shot reviewed Jul 8, 2025
View reviewed changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Johennes Johennes Johennes left review comments

@richvdh richvdh richvdh left review comments

@Half-Shot Half-Shot Half-Shot left review comments

@tulir tulir tulir left review comments

@MidhunSureshR MidhunSureshR MidhunSureshR left review comments

@TheArcaneBrony TheArcaneBrony TheArcaneBrony left review comments

@sumnerevans sumnerevans sumnerevans left review comments

@HarHarLinks HarHarLinks HarHarLinks requested changes

Assignees
No one assigned
Labels
client-server Client-Server API disposition-merge kind:feature MSC for not-core and not-maintenance stuff proposal A matrix spec change proposal proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. unresolved-concerns This proposal has at least one outstanding concern
Projects
Spec Core Team Workflow
Status: Ready for FCP ticks
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.