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

MSC3381: Polls (mk II) #3381

Merged
merged 38 commits into from
Dec 5, 2023
Merged

MSC3381: Polls (mk II) #3381

merged 38 commits into from
Dec 5, 2023

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented Sep 7, 2021

Rendered

Note: the implementations below are for an earlier draft of this proposal. The latest draft does not technically have any implementations, but is not functionally different from the prior draft. That prior draft, called "v1", is available here.

Element Web Implementation:

Element iOS Implementation:

Further work:


FCP tickyboxes

@turt2live turt2live added 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. proposal A matrix spec change proposal labels Sep 7, 2021
@turt2live turt2live changed the title [WIP] Polls (mk II) [WIP] MSC3381: Polls (mk II) Sep 7, 2021
proposals/3381-polls.md Outdated Show resolved Hide resolved
@turt2live turt2live added the client-server Client-Server API label Sep 7, 2021
proposals/3381-polls.md Outdated Show resolved Hide resolved
proposals/3381-polls.md Outdated Show resolved Hide resolved
proposals/3381-polls.md Show resolved Hide resolved
proposals/3381-polls.md Outdated Show resolved Hide resolved
proposals/3381-polls.md Outdated Show resolved Hide resolved
proposals/3381-polls.md Outdated Show resolved Hide resolved
proposals/3381-polls.md Outdated Show resolved Hide resolved
proposals/3381-polls.md Show resolved Hide resolved
proposals/3381-polls.md Outdated Show resolved Hide resolved
proposals/3381-polls.md Outdated Show resolved Hide resolved
proposals/3381-polls.md Outdated Show resolved Hide resolved
proposals/3381-polls.md Outdated Show resolved Hide resolved
proposals/3381-polls.md Outdated Show resolved Hide resolved
proposals/3381-polls.md Show resolved Hide resolved
proposals/3381-polls.md Show resolved Hide resolved
proposals/3381-polls.md Outdated Show resolved Hide resolved
and naming of fields changed. The differences are:

* For `m.poll.start` / `org.matrix.msc3381.poll.start`:
* `m.text` throughout becomes a single string, represented as `org.matrix.msc1767.text`
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that while this field is now required, it used to be optional, which should probably be clarified.

The reason I'm pointing that out is that we had exactly that issue with Ruma regarding the current implementations of Element Android and Element iOS, that don't send the field: ruma/ruma#1589 (comment).

Copy link
Member Author

Choose a reason for hiding this comment

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

* For `m.poll.end` / `org.matrix.msc3381.poll.end`:
* `m.text` has the same change as `m.poll.start`.
* `m.poll.results` is removed.
* `org.matrix.msc3381.poll.end` is added as an empty object, and is required.
Copy link
Contributor

@zecakeh zecakeh Jul 21, 2023

Choose a reason for hiding this comment

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

I don't know if it should be discarded as an implementation issue, but Element Android doesn't send this field (see ruma/ruma#1589 (comment)).

I'm guessing it could be put as optional for compatibility because there are events without it that exist in the wild.

Copy link
Member Author

Choose a reason for hiding this comment

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

Android would be "non-compliant" with this, but it sounds like Ruma is overly strict as well. The intention of Extensible Events is that if you know about the event type then the fallback is ignored. In other words, it's required on send, not receive.

@mscbot
Copy link
Collaborator

mscbot commented Jul 25, 2023

🔔 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 Jul 25, 2023
@mscbot
Copy link
Collaborator

mscbot commented Jul 30, 2023

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 Jul 30, 2023
@turt2live
Copy link
Member Author

will update the MSC text with the minor (non-blocking) changes before merge.

@uhoreg
Copy link
Member

uhoreg commented Nov 27, 2023

will update the MSC text with the minor (non-blocking) changes before merge.

@turt2live it looks like this got forgotten?

@turt2live
Copy link
Member Author

It's not forgotten, but heavily delayed 😭 I'm hoping to take a look in the short term, or at least before the year ends.

proposals/3381-polls.md Outdated Show resolved Hide resolved
@turt2live
Copy link
Member Author

2ff9870 addresses the minor review feedback which came up during FCP discussions. As it didn't block FCP and the threads appear to have conclusions, I've incorporated those conclusions into the proposal text.

Merging otherwise per #3381 (comment)

@turt2live turt2live merged commit e5eb721 into main Dec 5, 2023
1 check passed
@turt2live turt2live deleted the travis/msc/polls branch December 5, 2023 22:25
@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 blocked Something needs to be done before action can be taken on this PR/issue. and removed finished-final-comment-period labels Dec 5, 2023
@turt2live
Copy link
Member Author

Like #3930 (comment) , this is blocked on the wider system actually landing in the spec itself.

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. client-server Client-Server API kind:feature MSC for not-core and not-maintenance stuff proposal A matrix spec change proposal spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec
Projects
Status: BLOCKED, requires spec writing
Status: Done for now
Development

Successfully merging this pull request may close these issues.