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

For #26489 - Display synced tab pickup message only when recent synced tab view is available #26724

Closed
wants to merge 1 commit into from

Conversation

Alexandru2909
Copy link
Contributor

@Alexandru2909 Alexandru2909 commented Aug 31, 2022

For the users that prefer to sync in after the onboarding, we risk not showing the CFR when there will be a recent synced tab on the homescreen. This also impacts the Jump back in CFR based on the condition here.

We want to first check if there is a recent synced tab on the homescreen and then show the cfr and change the setting to avoid displaying the CFR multiple times.

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 #26489

@Alexandru2909 Alexandru2909 requested review from a team as code owners August 31, 2022 07:47
Comment on lines 210 to 228
if (context.settings().showSyncCFR) {
SyncCFRPresenter(
context = context,
recyclerView = view,
).showSyncCFR()
context.settings().showSyncCFR = false
}
Copy link
Member

@gabrielluong gabrielluong Aug 31, 2022

Choose a reason for hiding this comment

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

I am wondering why moving this if check inside of showSyncCFR fixes the problem.

From my point of view, I would like to avoid doing any type of work such as initializing SyncCFRPresenter and finding the sync tab view if showSyncCFR is already false. So, it seems appropriate to check showSyncCFR before doing any unnecessary compute.

Copy link
Contributor Author

@Alexandru2909 Alexandru2909 Sep 1, 2022

Choose a reason for hiding this comment

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

I moved the showSyncCFR check back in SessionControlView, but noticed the same change applied here. Should we close this and handle both scenarios in #26757?

@gabrielluong gabrielluong self-assigned this Aug 31, 2022
@mergify
Copy link
Contributor

mergify bot commented Sep 1, 2022

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

@Alexandru2909 Alexandru2909 deleted the 26489 branch September 2, 2022 13:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Synced Tab Pickup Onboarding Message
2 participants