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

#241 Group call notifications #250

Open
wants to merge 22 commits into
base: sc
Choose a base branch
from
Open

#241 Group call notifications #250

wants to merge 22 commits into from

Conversation

Turubaev
Copy link

@Turubaev Turubaev commented Sep 9, 2024

This PR add ring for group jitsi calls. Also it includes corresponding lab feature, that enable/disable this functionality.

@SpiritCroc
Copy link
Member

Thanks for your PR! I'll properly review and test when I have more time, maybe on the weekend.
I see you have added strings to both strings.xml and strings_sc.xml, you probably want to remove the duplicated ones from strings.xml.

when (intent?.getIntExtra(EXTRA_CALL_ACTION_KEY, 0)) {
CALL_ACTION_REJECT -> {
val callId = intent.getStringExtra(EXTRA_CALL_ID) ?: return
notificationUtils.cancelAllNotifications()
Copy link
Member

Choose a reason for hiding this comment

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

Should this really cancel all notifications?

Copy link
Author

Choose a reason for hiding this comment

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

Yeap, this was for testing purpose. Fixed, here we should cancel certain notification

@Turubaev
Copy link
Author

Thanks for your PR! I'll properly review and test when I have more time, maybe on the weekend. I see you have added strings to both strings.xml and strings_sc.xml, you probably want to remove the duplicated ones from strings.xml.

When I try to remove from "string.xml", I got "Unresolved reference: call_notification_open_app_action" while building the project. May be you can give example, how to correctly fix it

@@ -237,6 +237,9 @@
<!-- SC-TMP, until merged upstream: https://github.com/vector-im/element-android/pull/5781 -->
<string name="settings_call_ringtone_use_default_stun_title">Allow fallback call assist server</string>
<string name="settings_call_ringtone_use_default_stun_summary">Will use %s as assist when your homeserver does not offer one (your IP address will be seen by the stun server during a call

<string name="call_notification_open_app_action">Open App</string>
<string name="labs_enable_group_call_notifications_summary">Ring for group calls</string>
)</string>
Copy link
Member

@SpiritCroc SpiritCroc Sep 10, 2024

Choose a reason for hiding this comment

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

You added the strings within another string, that's why the build can't find them - need to properly define them where they belong

Copy link
Author

Choose a reason for hiding this comment

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

Got it, thanks !

Choose a reason for hiding this comment

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

@SpiritCroc , just in case if that got slipped away unnoticed - strings has been adjusted

Please, let us know if any other obstacles or things to be adjusted / improved!

Copy link
Member

Choose a reason for hiding this comment

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

I'm aware of that, thanks. The main thing I'm lacking right now is time to properly test and review, I assume earliest in two week. I usually don't do video calls on mobile, but until element-hq#8885 is fixed it's probably not too meaningful to get this released anyway.

Choose a reason for hiding this comment

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

Thank you for a quick response and sharing what's on the plate.

We didn't have / tested video calls in mind as well - do you believe that issue 8885 is a blocker for audio calls as well?

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.

5 participants