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

Remove ability to toggle wallpapers by tapping Firefox logo #25985

Closed
Tracked by #26034
MatthewTighe opened this issue Jul 11, 2022 · 13 comments · Fixed by #26481 or nathanmkaya/fenix#108
Closed
Tracked by #26034

Remove ability to toggle wallpapers by tapping Firefox logo #25985

MatthewTighe opened this issue Jul 11, 2022 · 13 comments · Fixed by #26481 or nathanmkaya/fenix#108
Labels
eng:qa:verified QA Verified eng:ready Ready for engineering Feature:Wallpapers
Milestone

Comments

@MatthewTighe
Copy link
Contributor

MatthewTighe commented Jul 11, 2022

The ability to toggle the selected wallpaper by tapping the Firefox logo introduces additional constraints on the freedom to choose different wallpaper download strategies. It is also a feature that can be activated accidentally pretty easily, leading to unexpected results. It should be removed for now.

Acceptance criteria:

  • Logo can no longer be tapped to switch wallpaper
  • Logo no longer animates
  • Settings data and preference keys used to determine if logo should animate are removed

┆Issue is synchronized with this Jira Task

@github-actions github-actions bot added the needs:triage Issue needs triage label Jul 11, 2022
@MatthewTighe MatthewTighe removed the needs:triage Issue needs triage label Jul 11, 2022
@sunildev0
Copy link
Contributor

hi @MatthewTighe can i do this?

@MatthewTighe
Copy link
Contributor Author

@sunilk9211 Sure, this seems like a good issue for a community contributor to pick up! I added some more explicit acceptance criteria to the OP to try and help guide you through it. I would start by looking at the following functions:

WallpaperManager::switchToNextWallpaper
WallpaperManager::animateLogoIfNeeded

Let me know if I can help with anything else 🙂 Thank you!

@sunildev0
Copy link
Contributor

Thanks @MatthewTighe. 😊
I have one question. Do we want to remove all the code related to this feature?
I can see in code, this feature is also used for some promotion related stuff.

@MatthewTighe
Copy link
Contributor Author

For the purposes of this issue we want to remove only the following:

  1. the ability to click the logo to change the wallpaper
  2. checks around whether the logo should be animated
  3. the animation of the logo

If it helps, feel free to start with just the first part. We can expand to include the others as part of the review process, or we can file a separate issue for the other 2 parts and address them in a separate PR.

@sunildev0
Copy link
Contributor

Okay, starting with the first part then. Thanks.

@MasterInQuestion
Copy link

Is not it optional: "Settings" -> "Homepage" -> "Wallpapers" -> "Change wallpaper by tapping Firefox homepage logo"?

sunildev0 added a commit to sunildev0/fenix that referenced this issue Jul 15, 2022
sunildev0 added a commit to sunildev0/fenix that referenced this issue Jul 15, 2022
sunildev0 added a commit to sunildev0/fenix that referenced this issue Jul 15, 2022
sunildev0 added a commit to sunildev0/fenix that referenced this issue Jul 15, 2022
sunildev0 added a commit to sunildev0/fenix that referenced this issue Jul 15, 2022
sunildev0 added a commit to sunildev0/fenix that referenced this issue Jul 15, 2022
sunildev0 added a commit to sunildev0/fenix that referenced this issue Jul 15, 2022
sunildev0 added a commit to sunildev0/fenix that referenced this issue Jul 15, 2022
@MatthewTighe
Copy link
Contributor Author

@MasterInQuestion It is optional, but the decision has still been made to remove it based on the reasons quoted in the original issue, in addition to the fact that it had low discoverability.

@MasterInQuestion
Copy link

Rather than removing the function completely, would not it be less destructive to just override the user config value before the version?

@MatthewTighe
Copy link
Contributor Author

@MasterInQuestion We aren't really interested in continuing to maintain the feature, and its existence is having an impact on our ability to plan improvements and additions to the wallpaper feature in general. Normally I would completely agree that we don't want remove existing features, but data has shown that this one has low awareness and usage as is, especially since it's on the newer side.

@sunilk9211 Are you still planning to submit a patch for this issue? I'm working through some refactoring of the underlying implementation of this feature, and will need to touch some of same code. I thought I'd check in beforehand and warn you that you may need to keep an eye on this space in order to keep your branch up-to-date if you're still working through it.

@sunildev0
Copy link
Contributor

sunildev0 commented Aug 1, 2022

Yes @MatthewTighe i am keeping my branch updated with the latest changes for this features.

sunildev0 added a commit to sunildev0/fenix that referenced this issue Aug 10, 2022
sunildev0 added a commit to sunildev0/fenix that referenced this issue Aug 10, 2022
@MasterInQuestion
Copy link

    Way too insignificant to worth further serious considerations.
    For this specific case, proceed at will. None should be really bothered anyway.

sunildev0 added a commit to sunildev0/fenix that referenced this issue Aug 15, 2022
sunildev0 added a commit to sunildev0/fenix that referenced this issue Aug 15, 2022
sunildev0 added a commit to sunildev0/fenix that referenced this issue Aug 15, 2022
sunildev0 added a commit to sunildev0/fenix that referenced this issue Aug 15, 2022
sunildev0 added a commit to sunildev0/fenix that referenced this issue Aug 15, 2022
sunildev0 added a commit to sunildev0/fenix that referenced this issue Aug 15, 2022
@github-actions github-actions bot removed the eng:reopen-for-qa Reopens and tags the issue for QA needed when the issue is merged label Aug 15, 2022
@github-actions github-actions bot added the eng:reopen-for-qa Reopens and tags the issue for QA needed when the issue is merged label Aug 15, 2022
MatthewTighe pushed a commit to MatthewTighe/fenix that referenced this issue Aug 16, 2022
MatthewTighe pushed a commit to MatthewTighe/fenix that referenced this issue Aug 16, 2022
@mergify mergify bot closed this as completed in #26481 Aug 16, 2022
mergify bot pushed a commit to MatthewTighe/fenix that referenced this issue Aug 16, 2022
@github-actions github-actions bot added this to the 105 milestone Aug 16, 2022
@github-actions github-actions bot reopened this Aug 16, 2022
@github-actions github-actions bot added eng:qa:needed QA Needed and removed eng:reopen-for-qa Reopens and tags the issue for QA needed when the issue is merged labels Aug 16, 2022
@delia-pop
Copy link

delia-pop commented Aug 17, 2022

Verified as fixed on the latest Nightly 105.0a1 from 08/17 with Xiaomi 12 Pro (Android 12):

  • the FF logo has no functionality;
  • the "Change wallpaper by tapping Firefox homepage logo" option is no longer available in the Wallpapers menu;
  • the FF logo is no longer animated;

https://user-images.githubusercontent.com/89388888/185168755-802cf9fb-be17-45da-b90e-9701a1f73532.mp4
Note that in the attached video, a delay in loading the wallpaper on homepage can be observed and the issue was reported in #26511 .

@delia-pop delia-pop added eng:qa:verified QA Verified and removed eng:qa:needed QA Needed labels Aug 17, 2022
@delia-pop
Copy link

Verified as fixed on Firefox Beta 105.0b1 as well, with Lenovo Yoga Tab 11 (Android 11).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.