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

Option dialog configuration fix #1266

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

CalebKL
Copy link
Contributor

@CalebKL CalebKL commented Aug 10, 2023

Description

This PR fixes optionDialog configuration bug by saving and retrieving the data as a Parcelable object.
Fixes #
#1220

Testing Instructions

  1. Go to Profile
  2. Go to settings(Top Right)
  3. Tap Auto add to up next
  4. Click Auto add limit to trigger the option Dialog
  5. Change Screen Orientation
  6. Observe the dialog doesn't crash

Screenshots or Screencast

record.mp4

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md
  • Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
  • 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

@CalebKL CalebKL requested a review from a team as a code owner August 10, 2023 12:13
Copy link
Contributor

@mchowning mchowning left a comment

Choose a reason for hiding this comment

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

Thanks for noticing this issue and putting together this fix!

This generally looks good, I just think there are two issues that we need to address before we can merge it.

First, after rotation the device, it doesn't seem like tapping on the buttons is working for me.

Video
Screen.Recording.2023-09-22.at.4.09.59.PM.mov

Second, we need to make sure this code runs on older versions of Android (this is what I brought up in my other comment).

Comment on lines +49 to +53
@RequiresApi(Build.VERSION_CODES.TIRAMISU)
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
savedInstanceState?.let {
options.addAll(it.getParcelableArrayList(KEY_OPTIONS, OptionsDialogOption::class.java) ?: mutableListOf())
Copy link
Contributor

Choose a reason for hiding this comment

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

This works fine on phones running Android 13, but any phones running older versions of Android will crash when they try to call the getParcelableArrayList method that only exists on Android 13. We need to do a version check and call the appropriate method for both Android 13 and for earlier versions. I would suggest creating an extension method similar to what we did with getSerializableCompat.

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.

None yet

2 participants