Feature/compose cleanup #1543

ThomasSession · 2024-07-12T01:42:39Z

Clean up work on our Compose classes.
Working on theming and overall standardising of UI elements.
Applying UI fixes where found.

Using the proper color for danger as opposed to one hardcoded color Reusing BlackAlpha40 Using the right delete icon in settings

Updated the typography in an composition local so it can be accessed from anyehere in compose and matching the figma declarations.

Unifying spacing values and reusing common ones Identified spacing issues and inconsistencies in design and figma

app/src/main/java/org/thoughtcrime/securesms/components/menu/ActionItem.kt

Discussed with QA to make sure the 'new message' screen should indeed behave as the other screens and use disabled state instead of disappearing

Re-using our new radio buttons in disappearing messages Tweaking UI to match designs

AL-Session · 2024-07-15T23:36:05Z

app/src/androidTest/java/network/loki/messenger/HomeActivityTests.kt

-        onView(withId(R.id.displayNameEditText)).perform(ViewActions.typeText("test-user123"))
-        onView(withId(R.id.registerButton)).perform(ViewActions.click())
+        objectFromDesc(R.string.displayNameEnter).click()
+        device.pressKeyCode(65)


Maybe add a comment like "Enter the dummy name 'ABC'" (is it sad that I remember some ASCII codes? Yes it is, haha)

This is code from the onboarding PR. I suggest looking at the commits instead to see the work from this PR.

...java/org/thoughtcrime/securesms/conversation/disappearingmessages/ui/DisappearingMessages.kt

bemusementpark · 2024-07-16T01:15:39Z

app/src/main/java/org/thoughtcrime/securesms/conversation/disappearingmessages/ui/UiState.kt

@@ -5,7 +5,7 @@ import network.loki.messenger.libsession_util.util.ExpiryMode
 import org.thoughtcrime.securesms.ui.GetString
 import org.thoughtcrime.securesms.ui.RadioOption

-typealias ExpiryOptionsCard = OptionsCard<ExpiryMode>
+typealias ExpiryOptionsCard = OptionsCardData<ExpiryMode>


cool, maybe suffix the typealias too?

bemusementpark · 2024-07-16T02:29:06Z

app/src/main/java/org/thoughtcrime/securesms/ui/theme/ThemeFromPreferences.kt

+ * Some behaviour is hardcoded to cater for legacy usage of people with themes already set
+ * But future themes will be picked and set directly from the "Appearance" screen
+ */
+@Composable


This function is only @Composable because of the call to isSystemInDarkTheme().

Perhaps extract everything else to a non-composable? or use val lightDarkColors: StateFlow<LightDarkColors> from this PR

It is always called from Compose so there is no issue with it being Composable. You do need to know what the system is currently set to isSystemInDarkTheme in addition to the preferences chosen by the user.
It can be cleaned up once we have the sharedPref code if that manages to help.

bemusementpark · 2024-07-16T02:30:35Z

app/src/main/java/org/thoughtcrime/securesms/ui/theme/Themes.kt

+val LocalColors = compositionLocalOf <ThemeColors> { ClassicDark() }
+val LocalType = compositionLocalOf { sessionTypography }
+
+var selectedTheme: ThemeColors? = null


top-level var is probably a code smell.

Yeah I don't love this but wanted something temporary while we get a root composable.
Maybe I can tweak the sharedPrefs flow once they're added in. Though I wouldn't move the compose data outside the app module where it belongs.

bemusementpark · 2024-07-16T02:32:35Z

...c/main/java/org/thoughtcrime/securesms/preferences/appearance/AppearanceSettingsViewModel.kt

    }

    fun setNewFollowSystemSettings(followSystemSettings: Boolean) {
        prefs.setFollowSystemSettings(followSystemSettings)
        _uiState.value = prefs.themeState()
+
+        // force compose to refresh its style reference
+        selectedTheme = null


This won't force currently composed layouts to refresh, it'll just refresh on a subsequent call.

So this Activity (and all open activities in the back stack) won't update.

Maybe this PR will help.

In a fully compose setup the value will come from a stateflow, directly passed to the root composable.
For now it works, even with the back stack, as going back to a previous fragment/activity calls the composable again which will check the value.
I'm a bit wary of moving all our composable theming in a module where it doesn't belong...

oh yeah... EmptyView is recomposed when you return to HomeActivity, I don't know why, I don't know if it's reliable.

Yeah, we don't have to move compose components. We can keep compose in this module, We'd just probably need to make another object... or just @Provides this StateFlow<Theme> with Dagger.

The currently open activity won't update though. It doesn't have any compose now...

Once we move to a fully composed screen the theme will be passed via a Flow so this won't be an issue anymore.
If we change this screen first before changing the full app we can make that flow available and collected at the root composable here to be applied.

alansley

All looking fair and reasonable from my perspective - have added a couple of comments / suggestions for you to address as you see fit.

alansley · 2024-07-17T01:43:20Z

libsignal/src/main/java/org/session/libsignal/crypto/MnemonicCodec.kt

-        if (mnemonic.isEmpty()) throw DecodingError.InputTooShort
-        if (words.isEmpty()) throw DecodingError.InputTooShort
+        // Check preconditions
+        if (words.size < 13) throw DecodingError.InputTooShort


Can we also add a check for if the mneumonic words is > 13 (i.e., additional superflous words)?

This is an onboarding comment > @bemusementpark

Ummm... It's technically possibly valid

alansley · 2024-07-17T01:45:10Z

app/src/main/java/org/thoughtcrime/securesms/home/HomeActivity.kt

@@ -138,6 +138,9 @@ class HomeActivity : PassphraseRequiredActionBarActivity(),
    // region Lifecycle
    override fun onCreate(savedInstanceState: Bundle?, isReady: Boolean) {
        super.onCreate(savedInstanceState, isReady)
+
+        if (!isTaskRoot) { finish(); return }


Potentially add a comment like "Prevent multiple instances of the app running at once"?

This is an onboarding comment > @bemusementpark

Yeah, it's worth a comment, as yeah, it's a lot more subtle than your suggestion ^^ :/

app/src/main/java/org/thoughtcrime/securesms/onboarding/loading/LoadingViewModel.kt

bemusementpark · 2024-07-18T03:26:09Z

...java/org/thoughtcrime/securesms/conversation/disappearingmessages/ui/DisappearingMessages.kt

+                state.cards.forEachIndexed { index, option ->
+                    OptionsCard(option, callbacks)
+
+                    // add spacing if not the last item
+                    if(index != state.cards.lastIndex){
+                        Spacer(modifier = Modifier.height(LocalDimensions.current.spacing))
+                    }


Suggested change

state.cards.forEachIndexed { index, option ->

OptionsCard(option, callbacks)

// add spacing if not the last item

if(index != state.cards.lastIndex){

Spacer(modifier = Modifier.height(LocalDimensions.current.spacing))

}

state.cards.forEachIndexed { index, option ->

if (index != 0) {

Spacer(modifier = Modifier.height(LocalDimensions.current.spacing))

}

OptionsCard(option, callbacks)

bemusementpark · 2024-07-18T03:30:08Z

app/src/main/java/org/thoughtcrime/securesms/conversation/start/home/StartConversation.kt

-    Column(modifier = Modifier.background(LocalColors.current.backgroundSecondary)) {
+    Column(modifier = Modifier.background(
+        LocalColors.current.backgroundSecondary,
+        shape = MaterialTheme.shapes.small


This isn't a small shape, it's a bottom sheet, it's large.

We shouldn't just use this because it has the same value.

We could do our own Shapes object, but probably overkill atm, so just some extra val BottomSheetShape?

It isn't large, it is the rounded corner value that we want, which in this case matches the small value that we have defined in the Theme

Yeah, I'm just saying it's a coincidence, that our theme at the moment uses the same shape for small and bottom sheets.

bemusementpark · 2024-07-18T03:36:34Z

app/src/main/java/org/thoughtcrime/securesms/ui/theme/ThemeColors.kt

+import androidx.compose.ui.tooling.preview.Preview
+import androidx.compose.ui.tooling.preview.PreviewParameter
+
+interface ThemeColors {


What's the inspiration for the name change?

We still have LocalColors, which is longer than I like 😂 and the idiom is to name that the same as the class it provides.

If it's just because of the clash in this file, we could use a typealias MatColors = material.Colors or whatever.

And then yeah, we can add those courtesies...

Colors { companion object { @Composable val primary get() = LocalColors.current.primary

Yeah I didn't think it was worth having yet another clashing class with the name Color. There are already enough of these around...

I don't think it actually clashes, anywhere except the one function in this file.

bemusementpark · 2024-07-18T04:06:47Z

...c/main/java/org/thoughtcrime/securesms/preferences/appearance/AppearanceSettingsViewModel.kt

    }

    fun setNewFollowSystemSettings(followSystemSettings: Boolean) {
        prefs.setFollowSystemSettings(followSystemSettings)
        _uiState.value = prefs.themeState()
+
+        // force compose to refresh its style reference
+        selectedTheme = null


The currently open activity won't update though. It doesn't have any compose now...

app/src/main/java/org/thoughtcrime/securesms/ui/AlertDialog.kt

bemusementpark · 2024-07-18T04:16:08Z

app/src/main/java/org/thoughtcrime/securesms/ui/components/Button.kt

    Button(onClick, ButtonType.Outline(LocalColors.current.primaryButtonFill), modifier, enabled, content = content)
 }

-@Composable fun SlimOutlineButton(onClick: () -> Unit, modifier: Modifier = Modifier, color: Color = LocalColors.current.text, enabled: Boolean = true, content: @Composable RowScope.() -> Unit) {
+@Composable fun SlimOutlineButton(modifier: Modifier = Modifier, color: Color = LocalColors.current.text, enabled: Boolean = true, onClick: () -> Unit, content: @Composable RowScope.() -> Unit) {


Probably makes sense to follow the order of the base component unless there's a good reason, no?

@Composable fun Button( onClick: () -> Unit, modifier: Modifier = Modifier, enabled: Boolean = true, shape: Shape = ButtonDefaults.shape, colors: ButtonColors = ButtonDefaults.buttonColors(), elevation: ButtonElevation? = ButtonDefaults.buttonElevation(), border: BorderStroke? = null, contentPadding: PaddingValues = ButtonDefaults.ContentPadding, interactionSource: MutableInteractionSource = remember { MutableInteractionSource() }, content: @Composable RowScope.() -> Unit ) {

I think I had the other method with onClick at the end so you could use kotlin-trailing-lambda syntax, because there was no content but we could change that one, as it's probably confusing to have a trailing-lambda that is a click listener when people expect it to be @Compasable.

That would have been great indeed but you matched all the other signature a different way so this was the only one that wasn't like the others.
Ideally we'd change all of them to match the original Button but that was quicker to at least have all our composable be the same...

bemusementpark · 2024-07-18T06:00:11Z

app/src/main/java/org/thoughtcrime/securesms/ui/theme/ThemeFromPreferences.kt

+fun TextSecurePreferences.getComposeTheme(): ThemeColors {
+    val selectedTheme = getThemeStyle()
+
+    // get the chosen primary color from the preferences
+    val selectedPrimary = primaryColor()
+
+    // create a theme set with the appropriate primary
+    val colorSet = when(selectedTheme){
+        TextSecurePreferences.OCEAN_DARK,
+        TextSecurePreferences.OCEAN_LIGHT -> ThemeColorSet(
+            light = OceanLight(selectedPrimary),
+            dark = OceanDark(selectedPrimary)
+        )
+
+        else -> ThemeColorSet(
+            light = ClassicLight(selectedPrimary),
+            dark = ClassicDark(selectedPrimary)
+        )
+    }
+
+    // deliver the right set from the light/dark mode chosen
+    val theme = when{
+        getFollowSystemSettings() -> if(isSystemInDarkTheme()) colorSet.dark else colorSet.light
+
+        selectedTheme == TextSecurePreferences.CLASSIC_LIGHT ||
+                selectedTheme == TextSecurePreferences.OCEAN_LIGHT -> colorSet.light
+
+        else -> colorSet.dark
+    }
+
+    return theme
+}


Perhaps simpler if we either delete ColorSet, or defer some responsibility to it.

Suggested change

fun TextSecurePreferences.getComposeTheme(): ThemeColors {

val selectedTheme = getThemeStyle()

// get the chosen primary color from the preferences

val selectedPrimary = primaryColor()

// create a theme set with the appropriate primary

val colorSet = when(selectedTheme){

TextSecurePreferences.OCEAN_DARK,

TextSecurePreferences.OCEAN_LIGHT -> ThemeColorSet(

light = OceanLight(selectedPrimary),

dark = OceanDark(selectedPrimary)

)

else -> ThemeColorSet(

light = ClassicLight(selectedPrimary),

dark = ClassicDark(selectedPrimary)

)

}

// deliver the right set from the light/dark mode chosen

val theme = when{

getFollowSystemSettings() -> if(isSystemInDarkTheme()) colorSet.dark else colorSet.light

selectedTheme == TextSecurePreferences.CLASSIC_LIGHT ||

selectedTheme == TextSecurePreferences.OCEAN_LIGHT -> colorSet.light

else -> colorSet.dark

}

return theme

}

@Composable

fun TextSecurePreferences.getComposeTheme(): ThemeColors {

val selectedTheme = getThemeStyle()

// get the chosen primary color from the preferences

val selectedPrimary = primaryColor()

// create a theme set with the appropriate primary

return (if (getFollowSystemSettings()) when (selectedTheme) {

OCEAN_DARK -> ::OceanDark

OCEAN_LIGHT -> ::OceanLight

CLASSIC_LIGHT -> ::ClassicLight

else -> ::ClassicDark

} else when (selectedTheme) {

OCEAN_DARK, OCEAN_LIGHT -> if (isSystemInDarkTheme()) ::OceanDark else ::OceanLight

else -> if (isSystemInDarkTheme()) ::ClassicDark else ::ClassicLight

})(selectedPrimary)

}

ThomasSession added 14 commits July 10, 2024 16:01

Moving color files

d4e3c58

Moving theme classes into their own package

2194d0d

Only obtain new theme when required

ce0308e

Merge branch 'dev' into feature/compose-cleanup

e45ee13

UI Tweaks

1466897

Using the proper color for danger as opposed to one hardcoded color Reusing BlackAlpha40 Using the right delete icon in settings

matching figma

127ac1c

Merge branch 'pr/1451' into feature/compose-cleanup

666a04c

Matching signature across buttons

af1c39b

Dialogs do not have a 'x' button by default

5061d37

Updated typography

f8e238c

Updated the typography in an composition local so it can be accessed from anyehere in compose and matching the figma declarations.

Centered text in control messages

c9c715b

Fixing new color access for action items

facbee9

renaming spacing to be more general

bb630fc

Unifying dimensions and rectifying design inconsistencies

166d0e8

Unifying spacing values and reusing common ones Identified spacing issues and inconsistencies in design and figma

bemusementpark reviewed Jul 15, 2024

View reviewed changes

app/src/main/java/org/thoughtcrime/securesms/components/menu/ActionItem.kt Show resolved Hide resolved

ThomasSession added 6 commits July 15, 2024 13:06

Rounded corners for the new conversation sheet

779d740

Rounding sheets corners

98e9d8a

Spacing fixes and UI consolidating

e193105

Discussed with QA to make sure the 'new message' screen should indeed behave as the other screens and use disabled state instead of disappearing

Disappearing messages tweaks

a1fe1f1

Re-using our new radio buttons in disappearing messages Tweaking UI to match designs

Cleaning up spaces

aabfb47

Migrating to Material3

a84ba25

AL-Session reviewed Jul 15, 2024

View reviewed changes

Fixing UI issues found

78110a4

bemusementpark reviewed Jul 16, 2024

View reviewed changes

...java/org/thoughtcrime/securesms/conversation/disappearingmessages/ui/DisappearingMessages.kt Show resolved Hide resolved

bemusementpark reviewed Jul 16, 2024

View reviewed changes

PR feedbacks

d9407a1

alansley approved these changes Jul 17, 2024

View reviewed changes

ThomasSession marked this pull request as ready for review July 17, 2024 05:00

ThomasSession and others added 2 commits July 17, 2024 15:54

Merge branch 'pr/1451' into feature/compose-cleanup

a524933

Merge branch 'dev' into feature/compose-cleanup

ef5fd14

bemusementpark reviewed Jul 18, 2024

View reviewed changes

bemusementpark approved these changes Jul 18, 2024

View reviewed changes

bemusementpark reviewed Jul 18, 2024

View reviewed changes

ThomasSession merged commit 9e2b24f into dev Jul 18, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/compose cleanup #1543

Feature/compose cleanup #1543

ThomasSession commented Jul 12, 2024

AL-Session Jul 15, 2024

ThomasSession Jul 16, 2024

bemusementpark Jul 16, 2024

bemusementpark Jul 16, 2024

ThomasSession Jul 16, 2024

bemusementpark Jul 16, 2024

ThomasSession Jul 16, 2024

bemusementpark Jul 16, 2024

ThomasSession Jul 16, 2024

bemusementpark Jul 16, 2024

bemusementpark Jul 18, 2024

ThomasSession Jul 18, 2024

alansley left a comment

alansley Jul 17, 2024

ThomasSession Jul 17, 2024

bemusementpark Jul 18, 2024

alansley Jul 17, 2024

ThomasSession Jul 17, 2024

bemusementpark Jul 18, 2024

bemusementpark Jul 18, 2024

bemusementpark Jul 18, 2024

ThomasSession Jul 18, 2024

bemusementpark Jul 18, 2024

bemusementpark Jul 18, 2024

ThomasSession Jul 18, 2024

bemusementpark Jul 18, 2024 •

edited

Loading

bemusementpark Jul 18, 2024

bemusementpark Jul 18, 2024

ThomasSession Jul 18, 2024

bemusementpark Jul 18, 2024 •

edited

Loading

Feature/compose cleanup #1543

Feature/compose cleanup #1543

Conversation

ThomasSession commented Jul 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alansley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bemusementpark Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bemusementpark Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

bemusementpark Jul 18, 2024 •

edited

Loading

bemusementpark Jul 18, 2024 •

edited

Loading