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

Show snackbar in large screen #1296

Closed

Conversation

Jaehwa-Noh
Copy link
Contributor

@Jaehwa-Noh Jaehwa-Noh commented Mar 18, 2024

What I have done and why
Add SafeDrawing bottom padding to show snackbar over bottom navigation bar

I'd set the windowInsetsPadding to NiaBackground, because SnackBar was drawn on the NiaBackground not NiaApp.

Fixes #1295

Foldable

For you Saved Interests
For you Saved Interests

Tablet

For you Saved Interests
For you Saved Interests

@JoseAlcerreca
Copy link
Contributor

Any chance to cover this with an UI behavior or screenshot regression test?

@Jaehwa-Noh
Copy link
Contributor Author

Jaehwa-Noh commented Mar 18, 2024

@JoseAlcerreca
Unfortunately, I can't.
I try to make some tests, but assertIsDisplay is always true whether behind System UI or not.
And screenshot test always throw exception

java.lang.RuntimeException: Unable to resolve activity for Intent { act=android.intent.action.MAIN cat=[android.intent.category.LAUNCHER] cmp=com.google.samples.apps.nowinandroid.core.designsystem.test/androidx.activity.ComponentActivity }

Does it exception cause because of I am using Windows 11?

@JoseAlcerreca
Copy link
Contributor

How are you running the test?

@Jaehwa-Noh
Copy link
Contributor Author

Jaehwa-Noh commented Mar 18, 2024

Oh It had discussed in issues. I forgotten.
• Run with ./gradlew testDemoDebug
The result as #1242

test



• Run test directly, you can see above mentioned exception.

By this reason, It is hard to make screenshot test on my own.

@Jaehwa-Noh
Copy link
Contributor Author

Jaehwa-Noh commented Mar 18, 2024

I finally found the formular to check position, but it only work when the simulator was Foldable(Resizable), not Phone and Tablet.
The problem is how to run tests only on Foldable device.

Test Code
/*
 * Copyright 2024 The Android Open Source Project
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     https://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

package com.google.samples.apps.nowinandroid.ui

import androidx.activity.compose.setContent
import androidx.compose.foundation.layout.BoxWithConstraints
import androidx.compose.foundation.layout.WindowInsets
import androidx.compose.foundation.layout.safeDrawing
import androidx.compose.material3.windowsizeclass.ExperimentalMaterial3WindowSizeClassApi
import androidx.compose.material3.windowsizeclass.WindowSizeClass
import androidx.compose.runtime.Composable
import androidx.compose.ui.platform.LocalDensity
import androidx.compose.ui.test.SemanticsNodeInteraction
import androidx.compose.ui.test.assertIsDisplayed
import androidx.compose.ui.test.assertIsSelected
import androidx.compose.ui.test.assertTopPositionInRootIsEqualTo
import androidx.compose.ui.test.hasAnyDescendant
import androidx.compose.ui.test.hasTestTag
import androidx.compose.ui.test.hasText
import androidx.compose.ui.test.isSelectable
import androidx.compose.ui.test.junit4.createAndroidComposeRule
import androidx.compose.ui.test.performClick
import androidx.compose.ui.unit.Dp
import androidx.compose.ui.unit.DpSize
import androidx.compose.ui.unit.dp
import com.google.accompanist.testharness.TestHarness
import com.google.samples.apps.nowinandroid.MainActivity
import com.google.samples.apps.nowinandroid.R
import com.google.samples.apps.nowinandroid.core.data.repository.CompositeUserNewsResourceRepository
import com.google.samples.apps.nowinandroid.core.data.test.networkmonitor.AlwaysOfflineNetworkMonitor
import com.google.samples.apps.nowinandroid.core.data.util.NetworkMonitor
import com.google.samples.apps.nowinandroid.core.data.util.TimeZoneMonitor
import com.google.samples.apps.nowinandroid.core.rules.GrantPostNotificationsPermissionRule
import com.google.samples.apps.nowinandroid.extensions.stringResource
import dagger.hilt.android.testing.BindValue
import dagger.hilt.android.testing.HiltAndroidRule
import dagger.hilt.android.testing.HiltAndroidTest
import org.junit.Before
import org.junit.Rule
import org.junit.Test
import org.junit.rules.TemporaryFolder
import javax.inject.Inject

@HiltAndroidTest
class ConnectSnackBarTest {

    /**
     * Manages the components' state and is used to perform injection on your test
     */
    @get:Rule(order = 0)
    val hiltRule = HiltAndroidRule(this)

    /**
     * Create a temporary folder used to create a Data Store file. This guarantees that
     * the file is removed in between each test, preventing a crash.
     */
    @BindValue
    @get:Rule(order = 1)
    val tmpFolder: TemporaryFolder = TemporaryFolder.builder().assureDeletion().build()

    /**
     * Grant [android.Manifest.permission.POST_NOTIFICATIONS] permission.
     */
    @get:Rule(order = 2)
    val postNotificationsPermission = GrantPostNotificationsPermissionRule()

    /**
     * Use the primary activity to initialize the app normally.
     */
    @get:Rule(order = 3)
    val composeTestRule = createAndroidComposeRule<MainActivity>()

    @Inject
    lateinit var userNewsResourceRepository: CompositeUserNewsResourceRepository

    @Inject
    lateinit var timeZoneMonitor: TimeZoneMonitor

    private val networkMonitor: NetworkMonitor = AlwaysOfflineNetworkMonitor()

    private val forYou by composeTestRule.stringResource(com.google.samples.apps.nowinandroid.feature.foryou.R.string.feature_foryou_title)
    private val interests by composeTestRule.stringResource(com.google.samples.apps.nowinandroid.feature.interests.R.string.feature_interests_title)
    private val saved by composeTestRule.stringResource(com.google.samples.apps.nowinandroid.feature.bookmarks.R.string.feature_bookmarks_title)
    private val netConnected by composeTestRule.stringResource(R.string.not_connected)

    private var height = 0.dp
    private var bottomSafeDrawingHeight = 0.dp

    @Before
    fun setup() = hiltRule.inject()

    @Test
    fun mediumWidth_WhenNotConnectedAndForYou_ConnectSnackBarShowUp() {
        composeTestRule.activity.apply {
            setContent {
                TestHarness(size = DpSize(610.dp, 1000.dp)) {
                    BoxWithConstraints {
                        val density = LocalDensity.current
                        height = maxHeight
                        bottomSafeDrawingHeight =
                            WindowInsets.safeDrawing.getBottom(density = density).dp
                        NiaApp(
                            appState = fakeAppState(maxWidth, maxHeight),
                        )
                    }
                }
            }
        }

        composeTestRule.apply {
            findNavigationButton(forYou).apply {
                performClick()
                assertIsSelected()
            }

            findSnackbarWithMessage(message = netConnected)
                .assertIsDisplayed()
                .assertTopPositionInRootIsEqualTo(height - bottomSafeDrawingHeight)
        }
    }

    private fun findSnackbarWithMessage(message: String): SemanticsNodeInteraction =
        composeTestRule.onNode(
            matcher = hasTestTag("Snackbar") and
                hasAnyDescendant(matcher = hasText(message)),
        )

    private fun findNavigationButton(string: String): SemanticsNodeInteraction =
        composeTestRule.onNode(matcher = isSelectable() and hasText(string))

    @OptIn(ExperimentalMaterial3WindowSizeClassApi::class)
    @Composable
    private fun fakeAppState(maxWidth: Dp, maxHeight: Dp) = rememberNiaAppState(
        windowSizeClass = WindowSizeClass.calculateFromSize(DpSize(maxWidth, maxHeight)),
        networkMonitor = networkMonitor,
        userNewsResourceRepository = userNewsResourceRepository,
        timeZoneMonitor = timeZoneMonitor,
    )
}
I would remain test parts to one who want to contribute.

@Jaehwa-Noh
Copy link
Contributor Author

Jaehwa-Noh commented Mar 19, 2024

I added test case.
But I don't say that this is best practices, but It is the only way to test, now.

Edit:
There's no way to test it.
The git action device is Phone size, and there are no Tablet or Foldable to test.

@Jaehwa-Noh Jaehwa-Noh marked this pull request as draft March 19, 2024 06:21
@Jaehwa-Noh Jaehwa-Noh marked this pull request as ready for review March 19, 2024 07:45
@Jaehwa-Noh
Copy link
Contributor Author

Jaehwa-Noh commented Mar 22, 2024

I breakthrough the testing by check the height of spacer.
If the spacer height is same as SafeDraw bottom height, It guarantee the snackbar will have shown above the System UI.

@Jaehwa-Noh
Copy link
Contributor Author

Jaehwa-Noh commented Mar 27, 2024

The test failed from testharness.

java.lang.ClassCastException: com.google.accompanist.testharness.ContextThemeWrapper cannot be cast to android.app.Activity
at androidx.window.layout.WindowMetricsCalculatorCompat.computeCurrentWindowMetrics(WindowMetricsCalculatorCompat.kt:68

The testharness deprecated so that I wait to bump Compose to use DeviceConfigurationOverride

Note: DeviceConfigurationOverride is a new API first available in Compose 1.7.0-alpha03

And I start discussion for screenshot test #1347, for this issue #1295.

Copy link
Contributor

@JoseAlcerreca JoseAlcerreca left a comment

Choose a reason for hiding this comment

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

For testing, how about you add it to #1343? You are checking for dimensions, so screenshots are a much easier way to do it.

I think you can do this by overriding WindowInsetsHolder? It has a setUseTestInsets function, but I haven't looked into this.

@Jaehwa-Noh
Copy link
Contributor Author

Jaehwa-Noh commented Mar 28, 2024

Thank you for your review.
Could you tell me more details?

For testing, how about you add it to #1343?

  1. Does it mean I'd merge your branch into this branch?


You are checking for dimensions, so screenshots are a much easier way to do it.

  1. This indicates this code?
  Spacer(
      modifier = Modifier
          .height(bottomNavigationHeight)
          .semantics {
              testTag = "Bottom padding for snackbar"
          },
  )



I think you can do this by overriding WindowInsetsHolder? It has a setUseTestInsets function, but I haven't looked into this.

  1. This suggestion for what situation? testharness fail problems? or screenshot tests? And could you give me a documentation or link about WindowInsetsHolder, I can't find it on android developer documentation.

Best regards.

@Jaehwa-Noh Jaehwa-Noh marked this pull request as draft April 12, 2024 12:37
@Jaehwa-Noh Jaehwa-Noh force-pushed the show-snackbar-in-large-screen branch from 18c1eee to 57eeb76 Compare April 16, 2024 07:51
@Jaehwa-Noh Jaehwa-Noh marked this pull request as ready for review April 16, 2024 08:13
@Jaehwa-Noh
Copy link
Contributor Author

Jaehwa-Noh commented Apr 16, 2024

Does Anyone have ideas giving the fake insets to WindowInsets.safeDrawing to test screenshot test?
My code does avoid System UI by calculate WindowInsets.safeDrawing.

val bottomNavigationHeight = density.run {
        WindowInsets.safeDrawing
            .getBottom(density = density)
            .toDp()
    }

Kindly leave your ideas in #1347

If you think my instrumented test ConnectSnackBarTest.kt is sufficient to test that avoid System UI logic, kindly merge this pull request.

@Jaehwa-Noh Jaehwa-Noh marked this pull request as draft April 17, 2024 02:59
@Jaehwa-Noh
Copy link
Contributor Author

I'd added screenshots. Done.

@Jaehwa-Noh Jaehwa-Noh marked this pull request as ready for review April 17, 2024 05:07
@Jaehwa-Noh Jaehwa-Noh marked this pull request as draft May 9, 2024 13:09
@Jaehwa-Noh Jaehwa-Noh marked this pull request as ready for review May 10, 2024 05:12
@alexvanyo
Copy link
Contributor

With the merging of #1498 , we should be able to directly test the fix here for the snackbar across device configurations. Would you be able to rebase on top of latest to check the result?

@Jaehwa-Noh
Copy link
Contributor Author

Jaehwa-Noh commented Jul 3, 2024

@alexvanyo Thank you for introducing. I'll try it or re-open this PR.

Change-Id: If26e71be75bb76b3f31b575a92044ea72aafc641
Change-Id: I93dbc31d9cdf6d23fb8cf5b080846f6e23e8a855
Change-Id: I914d391730d00860d23668d13359e3734e9d6324
Change-Id: Ie592f98a7ecd4fba0d02a671a4083859976e0cf2
Change-Id: I063eb5ec8dc55baaf48206db8801a050c4d6035a
@Jaehwa-Noh Jaehwa-Noh closed this Jul 3, 2024
@Jaehwa-Noh Jaehwa-Noh force-pushed the show-snackbar-in-large-screen branch from a7089e2 to 644e618 Compare July 3, 2024 06:36
Jaehwa-Noh and others added 6 commits July 3, 2024 06:41
Change-Id: I59dd7c3071a0d11d8a747c9b801d3de136e67770
Change-Id: I71bc6279b4fc9fe12a4783d87b2ed43491cace04
…ackbar-copy

Change-Id: I176216bbbf1562f951fe7c9be0cb0d9a6f94f4e5
@Jaehwa-Noh Jaehwa-Noh reopened this Jul 3, 2024
@Jaehwa-Noh
Copy link
Contributor Author

Jaehwa-Noh commented Jul 3, 2024

@alexvanyo @JoseAlcerreca
If you all have some spare time, kindly check this PR.
I changed some screenshot test code from #1498, because of the SnackBar was drawn on NiaBackground not on NiaApp.
It seemed to be done after Navigation library updated.

Thank you all.

@Jaehwa-Noh
Copy link
Contributor Author

Superseded by #1697

@Jaehwa-Noh Jaehwa-Noh closed this Nov 17, 2024
Copy link

@Saketram7575 Saketram7575 left a comment

Choose a reason for hiding this comment

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

Yyy

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.

[Bug]: Can't see a snackbar in medium and expanded screen
4 participants