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

Remove required attribute from m.room.avatar url #987

Merged
merged 3 commits into from
Mar 22, 2022

Conversation

jplatte
Copy link
Contributor

@jplatte jplatte commented Mar 8, 2022

Might close #471 (that issue refers to "State events like m.room.avatar" but gives no other examples).

Preview: https://pr987--matrix-spec-previews.netlify.app

@zecakeh
Copy link
Contributor

zecakeh commented Mar 8, 2022

I agree with this clarification but it should be noted that for now Element Web (and maybe others) actually omit the field.

@jplatte jplatte changed the title Clarify that m.room.avatar url can be null Remove required attribute from m.room.avatar url Mar 8, 2022
@jplatte
Copy link
Contributor Author

jplatte commented Mar 8, 2022

Updated the PR to just remove the required attribute, to properly reflect reality.

(I initially tested the wrong thing, oops 😄)

@richvdh richvdh requested a review from a team March 8, 2022 16:44
@turt2live
Copy link
Member

This opens a bit of a pandora's box on what "required" means. We also say that body is required for room messages, but if a message is redacted then it doesn't exist. Likewise, we don't actually say what a client should do if it receives a body-less message.

I think for this change to be accepted we'd need to figure out if this is actually what we want, and adjust the associated wording too. I've left this comment as a not-review to hopefully get other SCT members to take a look and form an opinion.

@jplatte
Copy link
Contributor Author

jplatte commented Mar 9, 2022

IMHO redacted events are a red herring: They are already clearly marked as not the regular form of the event by having the redacted_because field which you can check for before looking at the event's content.

In multiple years of maintaining Ruma, this is the one and only event I've come across where the Required attribute is set but non-redacted events without the field are still (somewhat frequently) encountered by users. That's not to say there aren't any others where this could happen, but I don't see how this would be opening a pandora's box: I know of no other events I would care to apply the same kind of change to right now.

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.

I think I need more context here. If this is unrelated to redactions (which are indeed a separate case), can someone explain to me why clients are sending m.room.avatar without a url property? It seems like it wouldn't be much use.

@jplatte
Copy link
Contributor Author

jplatte commented Mar 9, 2022

That's just what happens when you "remove" the avatar in Element Web and other clients. It sends a new m.room.avatar event without a url field to have the latest avatar be 'none' without actually redacting the information which avatar was used up until that point (which setting a new avatar would also not do).

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.

Right, I see. I think there's an argument that that would be better handled by setting url to the empty string, but otoh if the ecosystem is already doing it by removing url, that horse has bolted.

Just needs some clarifying text, imho.

data/event-schemas/schema/m.room.avatar.yaml Outdated Show resolved Hide resolved
changelogs/client_server/987.clarification Outdated Show resolved Hide resolved
@jplatte jplatte force-pushed the empty-room-avatar branch 2 times, most recently from 92d1dd8 to 09e679a Compare March 9, 2022 12:00
@jplatte jplatte requested a review from richvdh March 9, 2022 12:01
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.

this lgtm but I'm going to hold off from merging it for now in case any of the rest of @matrix-org/spec-core-team want to veto it

@richvdh richvdh requested a review from a team March 9, 2022 12:05
@richvdh
Copy link
Member

richvdh commented Mar 9, 2022

(and yes, my newsfragment CI check is broken. Fix in #993)

@turt2live
Copy link
Member

(please do not rebase after review has been given - instead prefer merge commits. Those merge commits will disappear when we merge the PR at the end)

@jplatte
Copy link
Contributor Author

jplatte commented Mar 9, 2022

Okay. Just noticed I force-pushed away the previous suggestion too. Will re-apply that.

@jplatte
Copy link
Contributor Author

jplatte commented Mar 21, 2022

this lgtm but I'm going to hold off from merging it for now in case any of the rest of @matrix-org/spec-core-team want to veto it

How long is the veto timeframe? Can this be merged now?

@richvdh
Copy link
Member

richvdh commented Mar 22, 2022

let's do it

@richvdh richvdh merged commit 1744808 into matrix-org:main Mar 22, 2022
@jplatte jplatte deleted the empty-room-avatar branch March 22, 2022 13:33
jplatte added a commit to ruma/ruma that referenced this pull request Mar 22, 2022
Removes the feature gate because this has been clarified to be right
in the spec: matrix-org/matrix-spec#987
@ShadowJonathan
Copy link
Contributor

Might close #471

this did close that issue due to GitHub’s PR keywords, should that issue be closed in the end?

@richvdh
Copy link
Member

richvdh commented Mar 22, 2022

think so

@jplatte
Copy link
Contributor Author

jplatte commented Mar 22, 2022

See rest of the issue description, I think we don't need it anymore. Can always be reopened if other affected events are found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need to clarify what empty / unset state events look like
6 participants