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

First steps in Jetpack Compose (reimplement parts of the user screen) #5607

Merged
merged 35 commits into from May 5, 2024

Conversation

westnordost
Copy link
Member

@westnordost westnordost commented Apr 29, 2024

I re-implemented the UI of parts of the user screen in compose and set up compose in general (define the default theme etc.). When this is merged, others can contribute re-implementations of parts of the UI in compose.

I initially planned to re-implement the whole user screen, but I came upon two issues which made me postpone this. (Click for more info)
  1. physics-ballpit needs to be replaced with something more boring. On the slim chance that someone with know-how pops up or when I return back to this, I have become a Compose-wizard, I'll leave this open for now (Replace ballpit quest-view in user profile with something else #5595)

  2. Login currently uses the WebView, for which there is no official Compose replacement. There is a library by a third party, but it misses at least one crucial part of the API to be used for OAuth authorization - shouldOverrideUrlLoading - used for retrieving the authorization code from the callback URL. So, maybe it is better to use the browser for OAuth authorization instead of a webview (which is also recommended in OAuth 2 anyway) but in any case I will postpone this because of that.

@ Contributors and people thinking about contributing in the future: Feel free to look through this and learn from it.

To be honest, I have to admit that it's quite fun to use Compose, the API, at least to do the standard things, is super straightforward. The previewing tools are excellent. But since the design is being done in code, it leaves more opportunities to turn the layout code into unreadable blurb, which is why more care is necessary splitting up components in the UI into own reusable composables for readability (and reusability). After all, this wasn't really done at all before, as all design was locked away in the layout XMLs (with lots of duplication inside). One reason for the almost -1000 lines of code on this PR.

@ People who know Jetpack Compose: Feel free to look through this and suggest improvements.


Since having viewmodels is almost something like a requirement to effectively convert the UI to compose and sometimes it is a bit more complex, I will continue with that after this PR is merged.

@Jean-BaptisteC
Copy link
Contributor

Is it possible to have an APK to test these changes?

@westnordost
Copy link
Member Author

I also changed some behavior (slightly) because it was easier to do in Compose while others were mere complicated to do.

@Jean-BaptisteC Why would you want to test the changes? It should work as before, except the some small things I changed. And it is just three screens.

import org.koin.androidx.viewmodel.ext.android.viewModel

/** activity only used in debug, to show all achievement links */
class ShowLinksActivity : BaseActivity() {
Copy link
Member Author

Choose a reason for hiding this comment

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

@FloEdelmann I removed this because it is more convenient to preview the links directly in the IDE, see LazyLinksColumn.kt

@@ -75,37 +68,6 @@ class ProfileViewModelImpl(
override val biggestSolvedCountCountryStatistics = MutableStateFlow<CountryStatistics?>(null)
override val biggestSolvedCountCurrentWeekCountryStatistics = MutableStateFlow<CountryStatistics?>(null)

override var lastShownGlobalUserRank: Int?
Copy link
Member Author

Choose a reason for hiding this comment

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

@matkoniecz I removed this because I found it to be a bit complicated or at least not worth the amount of code to keep it in Compose. Now, the badges just always animate, which is nicer anyway, IMO.

Sure, the sense of progression is lost, but that feature was too well hidden anyway, i.e. it was barely noticeable what changed exactly because the animation was played automatically on entering the screen, so one would have to look really quick anyway to see what the value before was.

And making it more prominent would on the other hand usually just look too disruptive. (Don't need a "New achievement unlocked"-type of dialog every time my rank is changed towards the better)

import de.westnordost.streetcomplete.ui.theme.LeafGreen

@Composable
fun LaurelWreath(
Copy link
Member Author

Choose a reason for hiding this comment

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

@matkoniecz I re-did the laurel wreath in compose. Feel free to look through the code to understand what I changed.

@westnordost
Copy link
Member Author

westnordost commented Apr 29, 2024

Changes to current behavior:

  • badges have double the size, nice and smooth animation always when entering the screen

  • dates active table is bigger, i.e. fills screen width, so especially on tablets etc. it is bigger. Also, added support for right-to-left layouts.

  • there is no appear-animation on opening the links screen or the achievement screen. (Nothing conveniently built-in in the API for that yet, and I didn't want to add cumbersome code just to preserve this somehow)

  • the achievement info dialog should work much better on a variety of different screen sizes and orientations now

  • the achievement info dialog is much more plain than before: No fancy pop-in animation that animates from the clicked view (no idea at all how to implement that in compose) and before I add some custom appear-animations to some (i.e. the first I re-implement) dialog, I think I better look into how maybe all appear-animations of all dialogs could be made to look consistent.

This was referenced Apr 29, 2024
@westnordost
Copy link
Member Author

Nobody interested to look through this? Well, I suppose if interested, you can also still look through it when it has been merged.

Maybe before or after merging this, I will look into if I can replicate something like described here - https://fvilarino.medium.com/shared-element-transitions-in-jetpack-compose-8f553078101e - in order to re-enable this:

fancy pop-in animation that animates from the clicked view (no idea at all how to implement that in compose)

# Conflicts:
#	app/src/main/java/de/westnordost/streetcomplete/screens/user/UserActivity.kt
#	app/src/main/res/layout/fragment_quest_answer.xml
@westnordost westnordost merged commit dbf4d5d into master May 5, 2024
@FloEdelmann FloEdelmann deleted the compose1 branch May 6, 2024 09:41
@Helium314
Copy link
Collaborator

Did you try the screen in dark mode?
In SCEE I have dark text on dark background (didn't test SC yet, though).

@westnordost
Copy link
Member Author

Hmm... I thought I did. I will check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants