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

Fix media notification controls configuration to support 3 icons #641

Merged
merged 1 commit into from
Dec 9, 2022

Conversation

ksog66
Copy link
Contributor

@ksog66 ksog66 commented Dec 8, 2022

Description

Add support for media notification controls configuration to support Maximum 3 icons.

Fixes #590

Testing Instructions

  1. Setup Android Auto emulator https://developer.android.com/training/cars/testing#test-auto
  2. Connect the phone using USB
  3. Play An episode
  4. Open full screen player on auto
  5. Click on option (more vert) icon.
  6. There will be 3 action displayed
  7. Now you can prioritize this action
  8. Go To Profile > Settings > General > Defaults.
  9. Click on Media Notification controls under defaults setting.
  10. Now drag and prioritize your preferred action from the following [ Archive, Mark As Played, Play Next, Playback Speed, Star] options.
  11. There will be 2 action on Android 13 media notification player and 3 actions on android auto.

Screenshots or Screencast

media_action_light
media_action_dark
media_android_auto

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 force-pushed the bugfix/media-notification-control branch from 4f08a73 to edf7c9d Compare December 8, 2022 19:04
@ksog66 ksog66 marked this pull request as ready for review December 8, 2022 19:05
@ksog66 ksog66 requested a review from a team as a code owner December 8, 2022 19:05
@ksog66
Copy link
Contributor Author

ksog66 commented Dec 8, 2022

Hey @ashiagr,This is working fine with android auto emulator right now, but I encountered a weird bug while testing on android auto emulator where, there were random icons shown instead of the media action, even for the forward and backward icons. even tough the functionality of the action were working as intended just the icons were different.

Couldn't reproduce it now, otherwise would attached a screen capture of that.
just wanted to know, if this bug is know?

@ashiagr
Copy link
Contributor

ashiagr commented Dec 9, 2022

👋 @ksog66!

Thank you so much for working on this!

I encountered a weird bug while testing on android auto emulator where, there were random icons shown instead of the media action, even for the forward and backward icons. even tough the functionality of the action were working as intended just the icons were different.

I can't find any existing issue for it. And haven't been able to reproduce it so far. I'll let you know if I can reproduce it.

@ksog66 ksog66 force-pushed the bugfix/media-notification-control branch from edf7c9d to f3de9f7 Compare December 9, 2022 06:15
@ashiagr
Copy link
Contributor

ashiagr commented Dec 9, 2022

Look like wrong icons could be a DHU emulator thing and here's how you can get them right:
https://stackoverflow.com/a/71742689/193545

I'm still not able to reproduce wrong icons.

@ksog66
Copy link
Contributor Author

ksog66 commented Dec 9, 2022

Yes, i think it's an DHU emulator issue as I also encountered it when tested my changes for the first time on DHU emulator.
Even my problem solved only after following the same method provided in https://stackoverflow.com/a/71742689/193545 except the rebooting of my phone.

After that even i am not able to reproduce the issue.

@ashiagr
Copy link
Contributor

ashiagr commented Dec 9, 2022

Awesome work, @ksog66! 🥇

I only have minor comments:

  1. Increase spacing between title and description by 8dp:

    Screenshot 2022-12-09 at 12 28 25 PM
  2. Change texts slightly as follows:

    • Your top actions will be available in your notification and Android Auto player -> Your top actions will be available in your Android 13 notification and Android Auto player
    • Other Media Action -> Other media actions

If you can address them now, I can prioritize and include this fix in 7.28.

@ksog66 ksog66 force-pushed the bugfix/media-notification-control branch from f3de9f7 to 2867fec Compare December 9, 2022 07:33
@ksog66
Copy link
Contributor Author

ksog66 commented Dec 9, 2022

Made the changes. Please review it. also have updated the changelog and put in 7.8

@ashiagr ashiagr force-pushed the bugfix/media-notification-control branch from 2867fec to 65a07db Compare December 9, 2022 07:55
@ashiagr ashiagr enabled auto-merge December 9, 2022 08:00
@ashiagr ashiagr merged commit 0afa3be into Automattic:main Dec 9, 2022
ashiagr added a commit that referenced this pull request Dec 9, 2022
Fix media notification controls configuration to support 3 icons
@ashiagr ashiagr added this to the 7.28 milestone Dec 9, 2022
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.

Add support for new media notification controls configuration to support 3 icons (if room).
2 participants