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

For #26329 Refactor the search widget to use the A-C common behaviour #26331

Closed
wants to merge 1 commit into from
Closed

For #26329 Refactor the search widget to use the A-C common behaviour #26331

wants to merge 1 commit into from

Conversation

iorgamgabriel
Copy link
Contributor

@iorgamgabriel iorgamgabriel commented Aug 5, 2022 •
edited by github-actions bot
Loading

It should behave like the current version .

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

@iorgamgabriel iorgamgabriel requested review from a team as code owners August 5, 2022 14:20
@iorgamgabriel iorgamgabriel marked this pull request as draft August 5, 2022 14:21
@Mugurell Mugurell added the pr:needs-ac-bump PR that needs a AC bump label Aug 5, 2022
@iorgamgabriel iorgamgabriel marked this pull request as ready for review August 8, 2022 09:13
@iorgamgabriel iorgamgabriel changed the title For #26329 Implement the search widget Fenix part For #26329 Refactor the search widget to use the A-C common behaviour Aug 8, 2022
@iorgamgabriel iorgamgabriel force-pushed the 26329 branch 6 times, most recently from 75cc0b6 to 6978990 Compare August 10, 2022 10:39
Mugurell
Mugurell approved these changes Aug 16, 2022
View reviewed changes
Copy link
Contributor

@Mugurell Mugurell left a comment

Choose a reason for hiding this comment

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

LGTM.
Though nothing stands out I saw a case in which the widget would not show the app icon, don't know if it's a new issue / device issue, to be assessed by QA.

app/src/main/java/org/mozilla/fenix/widget/VoiceSearchActivity.kt

/**
* Launches voice recognition then uses it to start a new web search.
* Implementation of voice search that is needed in search widget
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: The previous documentation is probably better since it has a wider reach and there are other ways to use voice search than just from the widget - eg: the url bar also has a microphone button which starts this functionality.

app/src/main/java/org/mozilla/gecko/search/SearchWidgetProvider.kt Outdated
return Intent(context, IntentReceiverActivity::class.java)
.let { intent ->
val createTextSearchIntentFlags = IntentUtils.defaultIntentPendingFlags or
PendingIntent.FLAG_UPDATE_CURRENT
intent.flags = Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TASK
intent.putExtra(HomeActivity.OPEN_TO_SEARCH, StartSearchIntentProcessor.SEARCH_WIDGET)
intent.putExtra(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: unneeded reformatting.

@Mugurell
Copy link
Contributor

LGTM. Though nothing stands out I saw a case in which the widget would not show the app icon, don't know if it's a new issue / device issue, to be assessed by QA.

The issue reproduces on main also for any update of search_widget_info.xml like this patch needs to do. Looks like this needs more investigations.

@Mugurell Mugurell self-requested a review August 16, 2022 07:34
@iorgamgabriel
Copy link
Contributor Author

I can reproduce the issue with disappearing icons on Pixel 3XL .
If I install the old version from main , I added the search widget and after that I install this version from this branch both the app icon and the microphone icon will disappear .
If I keep exactly the layout search_widget_info only the app icon is disappearing .
Until now I didn't find any fix . @Mugurell @jonalmeida

@Mugurell
Copy link
Contributor

This then seems like a framework / launcher issue.
If we can't update the configuration without users seeing a broken widget looks like Fenix shouldn't be refactored.
Curious if there are other reports online about this, seems like an important issue. if not - giving how easy it is to reproduce maybe we should at least report it to Google - issuetracker.google.com

@mcarare
Copy link
Contributor

mcarare commented Aug 17, 2022

LGTM. Though nothing stands out I saw a case in which the widget would not show the app icon, don't know if it's a new issue / device issue, to be assessed by QA.

The issue reproduces on main also for any update of search_widget_info.xml like this patch needs to do. Looks like this needs more investigations.

We could send a one-time APPWIDGET_UPDATE as a workaround if we still want to go ahead and change something related to the widget. But maybe that should be a last resort solution.

@Mugurell
Copy link
Contributor

We could send a one-time APPWIDGET_UPDATE as a workaround if we still want to go ahead and change something related to the widget. But maybe that should be a last resort solution.

Not sure when and where from if the issue reproduces immediately after updating the app but without the app having a chance to open. ?

@mcarare
Copy link
Contributor

mcarare commented Aug 17, 2022

For me, the bug reproduced only after opening the updated app.

@iorgamgabriel iorgamgabriel force-pushed the 26329 branch 4 times, most recently from 5494021 to 1fa66d4 Compare August 18, 2022 13:34
@iorgamgabriel
Copy link
Contributor Author

For the bug with icons disappearing at update I made a fix (an APPWIDGET_UPDATE in FenixApplication only one time ) , but sometimes I saw that the icons have disappeared random .
I don't have a scenario how to reproduce this bug and I don't know if this is happening in the old version of the app.
@jonalmeida what is your opinion regarding this ? Do we merge the patch or not ?

@jonalmeida
Copy link
Contributor

jonalmeida commented Aug 18, 2022 •
edited
Loading

@iorgamgabriel I would not land this in Fenix if we are unsure of what is causing the widget to be removed. For now, the original intention was to share the component in Focus, so we don't need to block that work on this PR. What do you think?

@iorgamgabriel
Copy link
Contributor Author

@jonalmeida It's ok . Thank you for the answer.

@iorgamgabriel iorgamgabriel mentioned this pull request Feb 13, 2023
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Mugurell Mugurell Awaiting requested review from Mugurell

Assignees
No one assigned
Labels
pr:needs-ac-bump PR that needs a AC bump
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor the search widget to use the A-C common behaviour
4 participants
@iorgamgabriel @Mugurell @mcarare @jonalmeida