you-apps / RecordYou

Privacy focused recorder app built with MD3
https://you-apps.net
GNU General Public License v3.0
739 stars 30 forks source link

Jetpack Compose navigation + New Homescreen UI + Amoled Theme #160

Closed SuhasDissa closed 1 year ago

SuhasDissa commented 1 year ago

Hi there,

I noticed that you're using Dialogs for the settings screen and recording screen. I understand that you may have done this for convenience, or maybe you didn't expect the app to have that many settings.

So, anyway I thought it might be a better idea to use Jetpack Compose navigation to navigate between different screens, mainly the settings screen and the recordings screen.

On the home screen, I didn't like the idea of using a tiny arrow to switch between the recording screens. I think it would be better to use a Bottom Bar, which would be more self-explanatory for users.

After all that, everything looked pretty good from a user experience standpoint. However, after moving all the icons to the top app bar, the homepage started to look a bit empty and boring. I thought to myself, "I'm gonna make this more aesthetically pleasing". So I moved some things around and added some placeholder art. I think it makes the app look less boring, and it's a technique that I've seen Google use in their apps.

I hope you will like these changes.

By the way I made these changes as smaller commits so it will be easier for you to revert any unwanted changes.

Thank you

SuhasDissa commented 1 year ago

Here are some screenshots for reference


Bnyro commented 1 year ago

Really cool changes, thanks for your work!

I'm not sure what kind of formatter/linter you use, however I'd like the code to be formatted with ktlint so that the code looks consistent everywhere.

Two thing I noticed:

  1. Instead of using a shared preferences listener, please let us continue using view models. Therefore they need to be created in the MainActivity and passed down as arguments to components that are lower in hierarchy.
  2. When using the AmoLED / black theme, it doesn't follow the Material You / wallpaper accent colors on Android 12+, so it looks a bit weird at that places in my opinion. Not sure if there's a better way to achieve an amoled theme, but it'd be nice if it followed the Android accent color if available.
SuhasDissa commented 1 year ago

I'm using the built in formatting options in android studio. (Ctrl + Alt + L)

To be honest I removed the themeModel becuse it didn't work. I wasn't sure why.

Maybe there's a way to override some specific colors in the material you dark theme. I will do some research on that later.

Bnyro commented 1 year ago

It doesn't work because Navigation Compose doesn't handle view models properly. See https://stackoverflow.com/questions/68548488/sharing-viewmodel-within-jetpack-compose-navigation.

SuhasDissa commented 1 year ago

It doesn't work because Navigation Compose doesn't handle view models properly. See https://stackoverflow.com/questions/68548488/sharing-viewmodel-within-jetpack-compose-navigation.

I use navigation in all my apps. but I never had this problem. Maybe this is a bug in the specific version I used. I use the latest beta version in my apps because it support navigation animation. I had to use an older version of navigation here because your android gradle plugin version is a bit older.

For example in my MellowMusic App I use PlayerViewModel in MainActivity.kt , FullScreenPlayer.kt and MiniPlayer

Bnyro commented 1 year ago

I use navigation in all my apps. but I never had this problem. Maybe this is a bug in the specific version I used. I use the latest beta version in my apps because it support navigation animation. I had to use an older version of navigation here because your android gradle plugin version is a bit older.

That's strange, however in all apps I used navigation compose so far, this issue persists, no matter the version of the used dependencies I tested. Doesn't matter anyways since the work around is quite straight forward, so it'd be good to just pass the view models down in hierarchy for now.

SuhasDissa commented 1 year ago

Maybe that happens because you use viewModel as a val

I asked chatGPT about this and this was the response

Using a ViewModel as a val (variable) in Jetpack Compose is not inherently bad. In fact, it is a common practice to store a ViewModel as a val when you want to retain and access its state across recompositions within the same scope.

By storing a ViewModel as a val, you ensure that the same instance of the ViewModel is used throughout the lifetime of the surrounding component or container. This can be beneficial in scenarios where you need to preserve the ViewModel's state, handle configuration changes, or share the ViewModel with multiple composables within the same scope.

Example:

@Composable
fun MyScreen() {
    val viewModel = viewModel<MyViewModel>()

    // Use the viewModel in multiple composables within the same scope
}

However, it's essential to be mindful of the lifecycle management of the ViewModel and prevent potential issues such as memory leaks or unintended state retention. You should consider scoping the ViewModel appropriately based on the lifecycle of the surrounding component or container.

You can use the viewModel or rememberViewModel functions provided by the androidx.lifecycle.viewmodel.compose package to handle ViewModel instantiation and scoping within Jetpack Compose.

It's worth noting that Jetpack Compose encourages a more declarative and unidirectional data flow approach. Therefore, when using a ViewModel as a val, it's recommended to expose its state through State or StateFlow objects, allowing Compose to automatically handle recompositions and updates based on changes in the ViewModel's state.

It says that using val for viewModel makes it keep the state same within the Composables of the same scope.

So I bet the same thing applies for this https://stackoverflow.com/questions/68548488/sharing-viewmodel-within-jetpack-compose-navigation

Bnyro commented 1 year ago

That seems like it could be the reason. However, I fix we should just stick with one of the solutions described in the Stackoverflow issue, and we're good.

SuhasDissa commented 1 year ago
2. When using the AmoLED / black theme, it doesn't follow the Material You / wallpaper accent colors on Android 12+, so it looks a bit weird at that places in my opinion. Not sure if there's a better way to achieve an amoled theme, but it'd be nice if it followed the Android accent color if available.

FIxed it here. (Ignore the mess made by the ktlint)

Bnyro commented 1 year ago

Oh, did you use ktlint --code-style=android -F or just ktlint? Looks somewhat not what I expected ktlint to make it look like :/

Bnyro commented 1 year ago

I'll fix the linting with an other commit myself, thanks for your efforts!