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

LoyaltCardEditActivity: Migrate to materialdatepicker dialog #1588

Merged
merged 3 commits into from
Nov 11, 2023

Conversation

theimpulson
Copy link
Contributor

Summary

This PR migrates Catima to use the new MaterialDatePicker. MaterialDatePicker is already based on DialogFragment, thus drop the current fragment and use the picker directly.

Related Issue

@theimpulson
Copy link
Contributor Author

Are the tests failing due to migration? I think not but not sure.

@TheLastProject TheLastProject linked an issue Oct 15, 2023 that may be closed by this pull request
@TheLastProject
Copy link
Member

Had to run the CI on GitHub a few times to get the same error as locally, but there does seem to be a test failure:

class android.app.Dialog cannot be cast to class android.app.DatePickerDialog (android.app.Dialog and android.app.DatePickerDialog are in unnamed module of loader org.robolectric.internal.AndroidSandbox$SdkSandboxClassLoader @4758820d)
java.lang.ClassCastException: class android.app.Dialog cannot be cast to class android.app.DatePickerDialog (android.app.Dialog and android.app.DatePickerDialog are in unnamed module of loader org.robolectric.internal.AndroidSandbox$SdkSandboxClassLoader @4758820d)
	at protect.card_locker.LoyaltyCardViewActivityTest.startWithLoyaltyCardNoExpirySetExpiry(LoyaltyCardViewActivityTest.java:728)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
	at org.robolectric.RobolectricTestRunner$HelperTestRunner$1.evaluate(RobolectricTestRunner.java:589)
	at org.robolectric.internal.SandboxTestRunner$2.lambda$evaluate$2(SandboxTestRunner.java:290)
	at org.robolectric.internal.bytecode.Sandbox.lambda$runOnMainThread$0(Sandbox.java:99)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
	at java.base/java.lang.Thread.run(Thread.java:833)

@obfusk
Copy link
Contributor

obfusk commented Oct 15, 2023

there does seem to be a test failure

Hard to tell if this is a bug in the code or in robolectric. I can try to debug more later if someone else doesn't figure it out first :)

@obfusk
Copy link
Contributor

obfusk commented Oct 15, 2023

Sorry for asking some general questions about the need to keep (or possibility to improve) certain existing bits of code without being explicit about that. I did want to know if those were still relevant after the migration to the new date picker, but if they aren't affected by the change, changes to that code should probably be considered out of scope for this PR and left to a later follow-up.

@obfusk obfusk mentioned this pull request Oct 15, 2023
49 tasks
@theimpulson theimpulson force-pushed the materialDatePicker branch 2 times, most recently from d38f46c to fac0d9b Compare October 25, 2023 05:52
@theimpulson theimpulson requested a review from obfusk October 25, 2023 05:53
Copy link
Member

@TheLastProject TheLastProject left a comment

Choose a reason for hiding this comment

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

Finally got some time to do basic testing. It definitely looks a lot prettier, but I am noticing several bugs:

User can set expiry before valid from date

To reproduce:

  1. Create a new card
  2. Set "Valid from" to tomorrow
  3. Click "Choose expiry date" in "Expiry date"
  4. Note that the calendar picker uses today's date as default
  5. Press OK
  6. Notice "Valid from" is tomorrow but "Expiry date" is today

On rotation change, dialog stops affecting value

To reproduce:

  1. Pick "Choose valid from date" or "Choose expiry date"
  2. Wait for the dialog to show
  3. Rotate the screen
  4. Pick a date
  5. Press OK
  6. Notice the "Valid from" or "Expiry date" field stays on the old value

(It vaguely reminds me of #1431, although that was a crash, this is just state loss.)

@theimpulson theimpulson force-pushed the materialDatePicker branch 2 times, most recently from 30f8a0f to 5f0396d Compare November 2, 2023 05:21
…ePicker

MaterialDatePicker is final and thus cannot be extended to handle loss of callback
on configuration changes. We aren't using ViewModel as well that would help us to persist
changes till lifecycle.

Fallback to how DatePicker was handling this situation with a couple of more hacks.

Signed-off-by: Aayush Gupta <[email protected]>
@theimpulson
Copy link
Contributor Author

theimpulson commented Nov 2, 2023

I'm not too fond of the last commit but cannot think of something better. When we will have ViewModel, half of this won't be required.

@TheLastProject
Copy link
Member

Seems to work great now, thanks!

A nice ViewModel structure would probably be a good improvement, but well, there's only so much time in each day so it is pretty low down my own priority list I'm afraid 😅

@TheLastProject TheLastProject merged commit 1d4e47b into CatimaLoyalty:main Nov 11, 2023
1 check passed
@theimpulson theimpulson deleted the materialDatePicker branch February 2, 2024 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate to MaterialDatePicker
3 participants