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

Voice message support #310

Closed
wants to merge 2 commits into from
Closed

Conversation

aviraldg
Copy link

autoplay in m.audio and m.recording event (required for element-hq/element-web#1358)

Generalizing the m.typing event instead may be a better idea.

@matrixbot
Copy link
Member

Can one of the admins verify this patch?

@ara4n
Copy link
Member

ara4n commented Apr 18, 2016

sorry for the delay on vetting this :(

I like the autoplay flag - I'm less certain about a whole new typing notification event in the form of m.recording. Typing notifs are already heavily special-cased both on servers & clients, and telling everyone they have to special case another event type there too is going to cause needless pain. I'm wondering if we can mux the info intom.typing instead... @erikjohnston any thoughts on this?

@aviraldg
Copy link
Author

Right, I felt the ame.

Generalizing the m.typing event instead may be a better idea.

@aviraldg
Copy link
Author

In general, in contet of thi and alo m.audio/m.file, I think a better olution i needed for derived event.

Broken keyboard 😢

@richvdh
Copy link
Member

richvdh commented May 4, 2016

@ara4n do you have the conn on this or do you want me to take a look?

@aviraldg
Copy link
Author

aviraldg commented Jun 2, 2016

@ara4n Any updates on this?

@ara4n
Copy link
Member

ara4n commented Jun 2, 2016

can we just slap it as an optional field on m.typing? i don't think it needs fullblown derived events...

@aviraldg
Copy link
Author

aviraldg commented Jun 2, 2016

@ara4n looks good to go?

},
"autoplay": {
"type": "boolean",
"description": "Hint that the audio should start playing automatically."
Copy link
Member

Choose a reason for hiding this comment

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

Be explicit please.

If true, this indicates that the audio should start playing automatically. If false or missing, the audio should not start playing automatically.

Copy link
Author

Choose a reason for hiding this comment

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

That was intentional, not all clients may wish to respect that hint.

On Thu 23 Jun, 2016, 9:28 PM Kegsay, [email protected] wrote:

In event-schemas/schema/m.room.message#m.audio
#310 (comment):

@@ -37,6 +37,10 @@
"duration": {
"type": "integer",
"description": "The duration of the audio in milliseconds."

  •                    },
    
  •                    "autoplay": {
    
  •                        "type": "boolean",
    
  •                        "description": "Hint that the audio should start playing automatically."
    

Be explicit please.

If true, this indicates that the audio should start playing
automatically. If false or missing, the audio should not start playing
automatically.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/matrix-org/matrix-doc/pull/310/files/7660ffbc7ff76a3f4dbf7a677489c8819332ab5c#r68262297,
or mute the thread
https://github.com/notifications/unsubscribe/AAOlRKWWjbNTOvOFk37OjT_zT3a2EUG2ks5qOq0bgaJpZM4IGApw
.

Copy link
Member

Choose a reason for hiding this comment

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

I understand that not all clients may wish to (given it is a "hint"). That's why you say SHOULD rather than leave it ambiguous.

@kegsay
Copy link
Member

kegsay commented Jun 23, 2016

Do typing notifications even support arbitrary keys?

@dreamflasher
Copy link

Is there any update on this?

@matrixbot
Copy link
Member

Can one of the admins verify this patch?

@bawNg
Copy link

bawNg commented Feb 7, 2017

The lack of push to talk support is one of the biggest things holding matrix back right now compared to proprietary competitors like Discord.

@aviraldg
Copy link
Author

There is now a work in progress implementation at matrix-org/matrix-react-sdk#690, so we might want to make a decision here.

@@ -1,7 +1,7 @@
{
"type": "object",
"title": "Typing Event",
"description": "Informs the client of the list of users currently typing.",
"title": "Activity Event",
Copy link
Member

Choose a reason for hiding this comment

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

"Activity" sounds like a presence thing to me - we already have currently_active etc there on the presence API. I think it's fine to call this a "typing event" still, but have the special case for a recording notification.

@ara4n
Copy link
Member

ara4n commented Feb 12, 2017

@aviraldg i think the last question was from our side (@kegsay's review discussion from #310 (comment)) if you can address that :)

Otherwise, modulo the comment I just added, this LGTM

@aviraldg
Copy link
Author

@ara4n I believe I updated the language to address @kegsay's concerns?

@kegsay
Copy link
Member

kegsay commented Feb 14, 2017

@aviraldg : You don't handle the missing case (if there is no autoplay, should it start playing automatically), and you don't use RFC-style SHOULD or SHOULD NOT keywords to indicate playability, you just use the more casual "should".

@aviraldg
Copy link
Author

aviraldg commented Feb 14, 2017 via email

@ara4n ara4n mentioned this pull request May 13, 2018
@turt2live turt2live changed the title Push to talk support. Voice message support Apr 3, 2019
@Luraktinus
Copy link

any news on this?

@turt2live
Copy link
Member

This conflicts pretty badly with the spec as it stands today. Like other PRs, this one looks to have been before its time - an MSC for this change would be appreciated. Thanks for the PR in any case!

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.

9 participants