Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

Closes #26750: add Wallpapers Onboarding dialog #26758

Merged

Conversation

mavduevskiy
Copy link
Contributor

@mavduevskiy mavduevskiy commented Sep 1, 2022

Closes #26750

Screenshot_20220831_174940

Screenshot_20220831_174940

Pull Request checklist

  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

QA

  • QA Needed

To download an APK when reviewing a PR (after all CI tasks finished running):

  1. Click on Checks at the top of the PR page.
  2. Click on the firefoxci-taskcluster group on the left to expand all tasks.
  3. Click on the build-debug task.
  4. Click on View task in Taskcluster in the new DETAILS section.
  5. The APK links should be on the right side of the screen, named for each CPU architecture.

GitHub Automation

Fixes #26750

@mavduevskiy mavduevskiy requested review from a team as code owners September 1, 2022 06:02
@mavduevskiy
Copy link
Contributor Author

mavduevskiy commented Sep 1, 2022

It looks like the dialog fragment breaks a lot (29) of UI tests. Should I adjust conditions of displaying somehow, or should I fix the tests?

@mavduevskiy mavduevskiy force-pushed the wallpapers-onboarding-dialog branch from df9c04f to aeb1197 Compare September 1, 2022 06:54
@mergify
Copy link
Contributor

mergify bot commented Sep 1, 2022

This pull request has conflicts when rebasing. Could you fix it @mavduevskiy? 🙏

@gabrielluong
Copy link
Member

It looks like the dialog fragment breaks a lot (29) of UI tests. Should I adjust conditions of displaying somehow, or should I fix the tests?

It's likely that the wallpaper dialog is showing up in the tests and interrupting their flow. You should ensure that the wallpaper dialog is shown up under the right conditions and you can also add functions to disable the feature/preference by creating a function in https://github.com/mozilla-mobile/fenix/blob/main/app/src/androidTest/java/org/mozilla/fenix/helpers/FeatureSettingsHelper.kt and call it in the affected tests setUp’s like https://github.com/mozilla-mobile/fenix/blob/main/app/src/androidTest/java/org/mozilla/fenix/ui/NavigationToolbarTest.kt#L46

@mavduevskiy mavduevskiy force-pushed the wallpapers-onboarding-dialog branch from aeb1197 to fbc7e3c Compare September 1, 2022 21:08
@mergify
Copy link
Contributor

mergify bot commented Sep 1, 2022

This pull request has conflicts when rebasing. Could you fix it @mavduevskiy? 🙏

@mavduevskiy mavduevskiy force-pushed the wallpapers-onboarding-dialog branch from 9b78661 to 1f6143e Compare September 1, 2022 22:40
@mergify
Copy link
Contributor

mergify bot commented Sep 2, 2022

This pull request has conflicts when rebasing. Could you fix it @mavduevskiy? 🙏

@mavduevskiy mavduevskiy force-pushed the wallpapers-onboarding-dialog branch 2 times, most recently from cf418f2 to c147882 Compare September 5, 2022 05:30
@mergify
Copy link
Contributor

mergify bot commented Sep 5, 2022

This pull request has conflicts when rebasing. Could you fix it @mavduevskiy? 🙏

Copy link
Contributor

@MatthewTighe MatthewTighe left a comment

Choose a reason for hiding this comment

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

First pass: this is looking good so far! I think it's pretty close, just had a few comments to share. Everything seems to work correctly when I try it locally

Comment on lines 68 to 70
val wallpapers = appStore.observeAsComposableState { state ->
state.wallpaperState.availableWallpapers
}.value ?: listOf()
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you mentioned already, but we will want to limit the number of wallpapers we show on the tool once we have more than THUMBNAILS_COUNT. I would suggest possible adding an extension function in the WallpaperOnboarding file that had a signature like List<Wallpaper>.toOnboardingList(): List<Wallpaper>. By using a public extension function, it should be pretty easy to test. Feel free to change the names or follow a different strategy if something else makes more sense to you, though.

This could also be addressed in a separate follow-up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll limit to THUMBNAILS_COUNT for now, will wait till WallpaperSettingsFragment would be updated with filtering.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with waiting to impose the limit along with the filtering, if you'd prefer. Up to you

wallpapers: List<Wallpaper>,
currentWallpaper: Wallpaper,
onCloseClicked: () -> Unit,
onButtonClicked: () -> Unit,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a more descriptive name might be good here. Something like onSettingsButtonClicked or onBottomButtonClicked. It would make it easier to add additional buttons in the future as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thought process was there is just one button and the Yagni principle, but I'll update with onBottomButtonClicked

Comment on lines 66 to 88
Icon(
painter = painterResource(id = R.drawable.mozac_ic_close),
contentDescription = stringResource(id = R.string.close_tab),
tint = FirefoxTheme.colors.iconPrimary,
modifier = Modifier
.clickable { onCloseClicked() }
.size(24.dp)
.align(Alignment.End)
)
Spacer(modifier = Modifier.height(8.dp))
Text(
text = stringResource(R.string.wallpapers_onboarding_dialog_title_text),
color = FirefoxTheme.colors.textPrimary,
fontWeight = FontWeight.Bold,
fontSize = 16.sp,
overflow = TextOverflow.Ellipsis,
maxLines = 1,
)
Spacer(modifier = Modifier.height(4.dp))
Text(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Generally speaking, I think we like to have empty lines between elements separated by Spacers for the sake of readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fair, changed :)

app/src/main/res/values/styles.xml Show resolved Hide resolved
@@ -322,6 +330,21 @@ class SessionControlInteractor(
controller.handleReadPrivacyNoticeClicked()
}

override fun showWallpapersOnboardingDialog(state: AppState?): Boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think state could be non-null, especially if the suggestion to switch to a when statement is SessionControlView is taken.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point!

Comment on lines 335 to 344
wallpapers.filter { wallpaper ->
wallpaper.thumbnailFileState == Wallpaper.ImageFileState.Downloaded
}.size.let { downloadedCount ->
downloadedCount >= WallpaperOnboardingDialogFragment.THUMBNAILS_COUNT &&
downloadedCount == wallpapers.size
}.also { isThumbnailsDownloaded ->
if (isThumbnailsDownloaded) {
controller.handleShowWallpapersOnboardingDialog()
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would make more sense to move this logic into SessionControlController. This suggestion may help with readability as well

Suggested change
wallpapers.filter { wallpaper ->
wallpaper.thumbnailFileState == Wallpaper.ImageFileState.Downloaded
}.size.let { downloadedCount ->
downloadedCount >= WallpaperOnboardingDialogFragment.THUMBNAILS_COUNT &&
downloadedCount == wallpapers.size
}.also { isThumbnailsDownloaded ->
if (isThumbnailsDownloaded) {
controller.handleShowWallpapersOnboardingDialog()
}
}
if (wallpapers.size >= WallpaperOnboardingDialogFragment.THUMBNAILS_COUNT && wallpapers.all { it.thumbnailFileState == Wallpaper.ImageFileState.Downloaded }) {
controller.handleShowWallpapersOnboardingDialog()
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@mergify
Copy link
Contributor

mergify bot commented Sep 7, 2022

This pull request has conflicts when rebasing. Could you fix it @mavduevskiy? 🙏

@mavduevskiy mavduevskiy force-pushed the wallpapers-onboarding-dialog branch 3 times, most recently from 8cab2a4 to c7271eb Compare September 9, 2022 07:16
@mavduevskiy
Copy link
Contributor Author

After rebase, lint gone berserk, trying to change 931 files. Investigating

@mcarare
Copy link
Contributor

mcarare commented Sep 9, 2022

@mavduevskiy After #26846 landed, a baseline file for ktlint was added.
After rebasing, the code in your PR either:

  • introduced code that does not follow ktlint rules (most likely the trailing comma rules)
  • introduced lines of code in files referenced in basline file, making the references (line, row) useless.

In this PR, you can either fix issues raised by ktlint or change the references in ktlint basline file. I would recommend the first option.

@mergify
Copy link
Contributor

mergify bot commented Sep 9, 2022

This pull request has conflicts when rebasing. Could you fix it @mavduevskiy? 🙏

Copy link
Contributor

@MatthewTighe MatthewTighe left a comment

Choose a reason for hiding this comment

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

Few more small comments, but this is otherwise looking ready to land to me!

* @param wallpapers Wallpapers to add to grid.
* @param currentWallpaper The currently selected wallpaper.
* @param onCloseClicked Callback for when the close button is clicked.
* @param onButtonClicked Callback for when the bottom text button is clicked.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This doc string doesn't reflect the new name

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param onButtonClicked Callback for when the bottom text button is clicked.
* @param onButtonClicked Callback for when the "Explore more" button is clicked.

currentWallpaper: Wallpaper,
onCloseClicked: () -> Unit,
onBottomButtonClicked: () -> Unit,
loadWallpaperResource: suspend (Wallpaper) -> Bitmap?,
Copy link
Member

Choose a reason for hiding this comment

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

Can we reorder this to be above onCloseClicked? We want to group all the event handlers together ideally.

wallpapers: List<Wallpaper>,
currentWallpaper: Wallpaper,
onCloseClicked: () -> Unit,
onBottomButtonClicked: () -> Unit,
Copy link
Member

@gabrielluong gabrielluong Sep 9, 2022

Choose a reason for hiding this comment

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

Suggested change
onBottomButtonClicked: () -> Unit,
onExploreMoreButtonClicked: () -> Unit,

We could always be more descriptive with our naming here. This would also avoid issues when we have more than one bottom button.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but in that case we create coupling between a string resource and the parameter naming. it might go out of sync too easily

Copy link
Contributor

@MozillaNoah MozillaNoah Sep 9, 2022

Choose a reason for hiding this comment

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

I think this is an assumed risk for the benefit of having clear documentation/APIs. InactiveTabsList has similar lamdbas that are directly tied to their use cases & user-facing descriptions (e.g. onDeleteAllButtonClick, onAutoCloseDismissClick, etc.). It makes it really clear what each lambda's responsibility is. Worst case, the lambda's name gets refactored if the button's text is updated and significantly deviates from the original wording.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, three versus one, democracy, changing to onExploreMoreButtonClicked

Comment on lines 81 to 82
fontWeight = FontWeight.Bold,
fontSize = 16.sp,
Copy link
Member

Choose a reason for hiding this comment

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

Do we have a matching FenixTypography that we can just utilize from the designs? https://searchfox.org/mozilla-mobile/source/fenix/app/src/main/java/org/mozilla/fenix/theme/FenixTypography.kt

Copy link
Member

Choose a reason for hiding this comment

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

This can be fixed as a followup

@@ -191,6 +191,7 @@ private fun collectionTabItems(collection: TabCollection) =
* @property interactor [SessionControlInteractor] which will have delegated to all user
* interactions.
*/
@Suppress("NestedBlockDepth")
Copy link
Member

@gabrielluong gabrielluong Sep 9, 2022

Choose a reason for hiding this comment

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

Perhaps move this before the init() block instead of suppressing it at the class level.

Comment on lines 42 to 50
val result = getCFRToShow()
when (result) {
is Result.SyncedTab -> showSyncedTabCFR(view = result.view)
is Result.JumpBackIn -> showJumpBackInCFR(view = result.view)
else -> {
// no-op
}
}
return result != Result.None
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
val result = getCFRToShow()
when (result) {
is Result.SyncedTab -> showSyncedTabCFR(view = result.view)
is Result.JumpBackIn -> showJumpBackInCFR(view = result.view)
else -> {
// no-op
}
}
return result != Result.None
return when (val result = getCFRToShow()) {
is Result.SyncedTab -> {
showSyncedTabCFR(view = result.view)
true
}
is Result.JumpBackIn -> {
showJumpBackInCFR(view = result.view)
true
}
else -> false
}

I would also consider writing it like this so we can get rid of the no-op

context = context,
recyclerView = view,
).show()
if (!featureRecommended && !context.settings().showHomeOnboardingDialog) {
Copy link
Member

@gabrielluong gabrielluong Sep 9, 2022

Choose a reason for hiding this comment

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

Wouldn't featureRecommended always be false anyways when this class is created? I think we can avoid this check since it's always true and simply declare featureRecommended inside the if block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it will be. but onLayoutCompleted is being called multiple times during one lifecycle, in cases like adding items, keyboard, orientation change, and it would launch multiple CFR during one homescreen lifecycle if we don't keep track of CFRs being actually displayed during a single fragment lifecycle

Copy link
Contributor

Choose a reason for hiding this comment

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

I think in general there is a future opportunity to solidify our work around when different onboarding items are shown. That said, I think this solution would work for now given the details @mavduevskiy has listed and our time constraints.

/**
* Show Wallpapers onboarding dialog to onboard users about the feature if conditions are met.
* Returns true if the call has been passed down to the controller.
* @param state The wallpaper state.
Copy link
Member

Choose a reason for hiding this comment

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

nit: Add a new line above to separate the description and @param

Comment on lines 161 to 162
verticalPadding: Int = 30,
horizontalPadding: Int = 20
Copy link
Member

Choose a reason for hiding this comment

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

nit: Missing @param

@mavduevskiy mavduevskiy force-pushed the wallpapers-onboarding-dialog branch from 42b5e4d to 014e8db Compare September 9, 2022 19:17
Copy link
Contributor

@MatthewTighe MatthewTighe left a comment

Choose a reason for hiding this comment

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

Couple more small nits, but I think this is otherwise ready to land. Functionality looks good and as I would expect. Thanks for helping out with this!

I would maybe give @gabrielluong a bit to make sure his concerns are addressed before landing.

* Returns true if the call has been passed down to the controller.
*
* @param state The wallpaper state.
* @return Whether the onboarding dialog is currently shown
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: a period at the end of this comment for consistency

setContent {
FirefoxTheme {
val wallpapers = appStore.observeAsComposableState { state ->
state.wallpaperState.availableWallpapers.subList(0, THUMBNAILS_COUNT)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: .take() feels slightly more idiomatic than .subList() to me, but I would also be okay with this for now

Comment on lines 154 to 162
fun WallpaperThumbnails(
wallpapers: List<Wallpaper>,
defaultWallpaper: Wallpaper,
loadWallpaperResource: suspend (Wallpaper) -> Bitmap?,
selectedWallpaper: Wallpaper,
numColumns: Int = 3,
onSelectWallpaper: (Wallpaper) -> Unit,
verticalPadding: Int = 30,
horizontalPadding: Int = 20,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: while we're here, we can group the lambdas and the parameters that have defaults

Suggested change
fun WallpaperThumbnails(
wallpapers: List<Wallpaper>,
defaultWallpaper: Wallpaper,
loadWallpaperResource: suspend (Wallpaper) -> Bitmap?,
selectedWallpaper: Wallpaper,
numColumns: Int = 3,
onSelectWallpaper: (Wallpaper) -> Unit,
verticalPadding: Int = 30,
horizontalPadding: Int = 20,
fun WallpaperThumbnails(
wallpapers: List<Wallpaper>,
defaultWallpaper: Wallpaper,
selectedWallpaper: Wallpaper,
loadWallpaperResource: suspend (Wallpaper) -> Bitmap?,
onSelectWallpaper: (Wallpaper) -> Unit,
numColumns: Int = 3,
verticalPadding: Int = 30,
horizontalPadding: Int = 20,

@gabrielluong
Copy link
Member

No need to wait for me.

@mavduevskiy
Copy link
Contributor Author

@MatthewTighe
Updated!

text = stringResource(R.string.wallpapers_onboarding_dialog_explore_more_button_text),
fontWeight = FontWeight.Bold,
color = FirefoxTheme.colors.textAccent,
fontSize = 14.sp,
Copy link
Member

Choose a reason for hiding this comment

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

This could be FirefoxTheme.typography.button. We shouldn't ever have to define fontWeight because our typography is well defined. We also typically encourage following the order of the named parameters that you would see defined by the calling function.

@Composable
fun Text(
    text: String,
    modifier: Modifier = Modifier,
    color: Color = Color.Unspecified,
    fontSize: TextUnit = TextUnit.Unspecified,
    fontStyle: FontStyle? = null,
    fontWeight: FontWeight? = null,
    fontFamily: FontFamily? = null,
    letterSpacing: TextUnit = TextUnit.Unspecified,
    textDecoration: TextDecoration? = null,
    textAlign: TextAlign? = null,
    lineHeight: TextUnit = TextUnit.Unspecified,
    overflow: TextOverflow = TextOverflow.Clip,
    softWrap: Boolean = true,
    maxLines: Int = Int.MAX_VALUE,
    onTextLayout: (TextLayoutResult) -> Unit = {},
    style: TextStyle = LocalTextStyle.current
)

So, we would typically expect style to be the last named parameter.

@mavduevskiy mavduevskiy added the pr:needs-landing PRs that are ready to land [Will be merged by Mergify] label Sep 9, 2022
@mergify mergify bot merged commit 97987db into mozilla-mobile:main Sep 9, 2022
@csadilek
Copy link
Contributor

@mavduevskiy @MatthewTighe ⚠️ We have numerous reports of Nightly (home screen) freezing starting Saturday. I will put up a revert to verify if caused by this change, or #26794.

@csadilek
Copy link
Contributor

@mavduevskiy @MatthewTighe We can close the revert if it was the other change.

as nit: Please let's squash commits like these before landing "pr review changes" "add format changes" etc.

@csadilek
Copy link
Contributor

@mavduevskiy @MatthewTighe OK, we didn't have the time to verify which PR caused the problem. It's likely it's #26794 instead, and this can just re-land after discussion on Monday. For now, we just want to get a fixed Nightly out.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr:needs-landing PRs that are ready to land [Will be merged by Mergify]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a simple wallpaper dialog
6 participants