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

Account Settings: Use Alorma Compose Settings Components #731

Merged
merged 9 commits into from
Jun 19, 2023

Conversation

sabieber
Copy link
Contributor

@sabieber sabieber commented Jun 17, 2023

As promised in #612 I tried to replace all custom component for which Alorma Compose Settings provide a counter part.

The following components don't seem to be present in the lib:

  • text field
  • text area
  • image selector

Because of this I left the custom components for these intact.

Preview

sabieber added 5 commits June 17, 2023 22:04
This uses the checkboxes of the alorma compose settings lib instead of creating our own.
This uses the dropdowns of the alorma compose settings lib instead of creating our own.
Updating the state is handled by the checkbox component itself.
Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

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

Thank you... this is sooo much better. Fix that one conflict then I can merge.

@sabieber sabieber force-pushed the account-compose-settings branch from 929e76a to ca4d3d7 Compare June 19, 2023 15:21
# Conflicts:
#	app/src/main/java/com/jerboa/ui/components/settings/account/AccountSettings.kt
@sabieber
Copy link
Contributor Author

sabieber commented Jun 19, 2023

@dessalines I am trying to merge main into the branch but I seem to be unable to login to the new main development state of the app as a NPE is thrown (perhaps because of commit 7d9c5a6)

It seems we want to read the ordinal of a setting that is not yet set for a freshly logged in account.

java.lang.NullPointerException: Attempt to invoke virtual method 'int com.jerboa.datatypes.types.ListingType.ordinal()' on a null object reference
                                                                                                    	at com.jerboa.ui.components.login.LoginViewModel$login$1.invokeSuspend(LoginViewModel.kt:103)
                                                                                                    	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
                                                                                                    	at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
                                                                                                    	at android.os.Handler.handleCallback(Handler.java:942)
                                                                                                    	at android.os.Handler.dispatchMessage(Handler.java:99)
                                                                                                    	at android.os.Looper.loopOnce(Looper.java:201)
                                                                                                    	at android.os.Looper.loop(Looper.java:288)
                                                                                                    	at android.app.ActivityThread.main(ActivityThread.java:7872)
                                                                                                    	at java.lang.reflect.Method.invoke(Native Method)
                                                                                                    	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548)
                                                                                                    	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:936)
                                                                                                  ```

The settings for default sort and listing type were changed from ints to enums so we need to handle those with the new compose fields.
@sabieber
Copy link
Contributor Author

I think I properly merged the two changes but I would like to test it locally before the PR is merged :)

@dessalines
Copy link
Member

mmmk no problem, ping me whenever you're ready to go.

@sabieber
Copy link
Contributor Author

sabieber commented Jun 19, 2023

mmmk no problem, ping me whenever you're ready to go.

@dessalines is the reported NPE issue reproducable by you on the main branch?
What I did:

  1. Clean install of the debug build
  2. Login with my user account
  3. Crash

If not I will try to fix it in my branch. But also the failing test in the build does not seem to correlate with something I changed.

@dessalines
Copy link
Member

Use https://voyager.lemmy.ml , the current main branch has breaking API changes.

@sabieber
Copy link
Contributor Author

@dessalines Thanks for the hint. With an account on that instance my changes to the account settings view seem to work with the new enums. Did set them to "Subscribed" and "TopToday", reopening the settings it stayed the same and restarting the app launched the screen to "Subscribed" and "TopToday"
So my local tests are succesfull.

@dessalines
Copy link
Member

Perfect, thanks!

@dessalines dessalines merged commit 4872a18 into LemmyNet:main Jun 19, 2023
@sabieber
Copy link
Contributor Author

Thanks for merging 👍🏻

@sabieber sabieber deleted the account-compose-settings branch June 20, 2023 06:21
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