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

Add Media Notification Controls for Android 13 #540

Merged
merged 1 commit into from
Nov 4, 2022

Conversation

ksog66
Copy link
Contributor

@ksog66 ksog66 commented Nov 3, 2022

Description

Adding Media Notification Control setting under Defaults of General setting page, which gives Android 13 users option to have at max 2 option from Archive, Mark as played, Play next, Playback speed and Star option in notification media session.

Testing Instructions

  1. Start playing a podcast ( and one in queue to check play next).
  2. Go To Profile > Settings > General > Defaults.
  3. Click on Media Notification controls under defaults setting.
  4. Now choose at most two options from the following [ Archive, Mark As Played, Play Next, Playback Speed, Star] options.
  5. Go to the Notifications where Media Controller is and click on the custom actions (chosen in setting) and that action will be performed.

Screenshots or Screencast

clip4.webm
clip1.webm
clip2.webm
clip3.webm

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md
  • I have considered whether it makes sense to add tests for my changes
  • All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml
  • Any jetpack compose components I added or changed are covered by compose previews

I have tested any UI changes...

  • with different themes
  • with a landscape orientation
  • with the device set to have a large display and font size
  • for accessibility with TalkBack

@ksog66 ksog66 marked this pull request as ready for review November 3, 2022 16:41
@ksog66 ksog66 requested a review from a team as a code owner November 3, 2022 16:41
@ksog66
Copy link
Contributor Author

ksog66 commented Nov 3, 2022

Hey @ashiagr , I have opened this new PR for the issue #499.

Copy link
Contributor

@ashiagr ashiagr left a comment

Choose a reason for hiding this comment

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

This is working really well! Thank you so much for the effort. 🙇‍♀️ I just left a few code-style comments.

Let me also ping @adamzelinski to provide any design feedback.

Meanwhile, can you add a few testing instructions in the PR description?

Also, let's add a CHANGELOG entry, something like:
Allowed customization of actions through Settings in Mediastyle Notification

@ashiagr ashiagr added this to the 7.27 milestone Nov 4, 2022
@ksog66 ksog66 force-pushed the add/media-notification-control branch 2 times, most recently from 8344921 to ce30d91 Compare November 4, 2022 08:54
@ksog66
Copy link
Contributor Author

ksog66 commented Nov 4, 2022

Hey @ashiagr, I have made the required changes and I messed up while squashing the commits by rebasing again, So had to cherry pick my changes and then force push to this branch only( didn't made new PR), this will not be a problem. right?

@ashiagr
Copy link
Contributor

ashiagr commented Nov 4, 2022

I have made the required changes and I messed up while squashing the commits by rebasing again, So had to cherry pick my changes and then force push to this branch only( didn't made new PR), this will not be a problem. right?

No worries, 😀 I deleted my local branch and re-fetched yours.

All updates look good, nice job! I found a typo though.

I was also wondering if we should hide the setting if the device OS version is less than 13. Wdyt?

@ksog66
Copy link
Contributor Author

ksog66 commented Nov 4, 2022

yes, I think we should hide the setting for version less than 13.
I will make the changes and update you once it is done

@ksog66 ksog66 force-pushed the add/media-notification-control branch from ce30d91 to 1c433fb Compare November 4, 2022 10:10
@ksog66
Copy link
Contributor Author

ksog66 commented Nov 4, 2022

Hey @ashiagr, made the required changes, please review it.

Comment on lines +140 to +147
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) {
MediaNotificationControls(
saved = settings.defaultMediaNotificationControlsFlow.collectAsState().value,
onSave = {
settings.setDefaultMediaNotificationControls(it)
}
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good!

@ashiagr ashiagr self-requested a review November 4, 2022 12:35
@ashiagr
Copy link
Contributor

ashiagr commented Nov 4, 2022

👋 @adamzelinski

I'm merging this PR, but feel free to drop a comment if you have any design feedback.

Thanks once again @ksog66, for this contribution. 🙏

@ashiagr ashiagr merged commit 358c9b5 into Automattic:main Nov 4, 2022
@ghost
Copy link

ghost commented Nov 15, 2022

I'm merging this PR, but feel free to drop a comment if you have any design feedback.

@ashiagr Sorry for the late response on this - I was AFK. These additions look really nice though. Thanks so much!

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.

2 participants