Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

For #12565 Implement the common part of search widget in Android Components #12584

Merged
merged 1 commit into from
Aug 11, 2022

Conversation

iorgamgabriel
Copy link
Contributor

@iorgamgabriel iorgamgabriel commented Aug 1, 2022

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
  • Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

GitHub Automation

Fixes #12565

@iorgamgabriel iorgamgabriel requested review from a team as code owners August 1, 2022 14:07
@iorgamgabriel iorgamgabriel requested review from hneiva and removed request for a team August 1, 2022 14:07
@iorgamgabriel iorgamgabriel marked this pull request as draft August 1, 2022 14:07
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.

Just a quick driveby, this is looking great!

@@ -0,0 +1,83 @@
<?xml version="1.0" encoding="utf-8"?>
Copy link
Contributor

Choose a reason for hiding this comment

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

A-C should be "client agnostic" and not include the launcher icon of Fenix.
If this resource is really needed maybe we can set the source to be mozac_ic_globe.

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 added mozac_search_widget_icon that is an empty file . This file will be overridden in Focus and Fenix with the specific app icon .

@iorgamgabriel iorgamgabriel force-pushed the 12565 branch 3 times, most recently from a9027c9 to c37f416 Compare August 2, 2022 13:32
@iorgamgabriel iorgamgabriel marked this pull request as ready for review August 2, 2022 13:33
Copy link
Contributor

@jonalmeida jonalmeida left a comment

Choose a reason for hiding this comment

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

Looks really nice! Left a few comments below too.

package="mozilla.components.feature.search">

<uses-permission
android:name="android.permission.QUERY_ALL_PACKAGES"
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 I missed this earlier - why do we need the permission to query all packages (apps) installed on the user's phone? This permission is very powerful and requires us to provide a reason on how we use it when submitting an app to the Play store.

The warning QueryAllPackagesPermission below is meant to make us aware of what we are requesting, so if we do need this permission here, we should at the very least leave a comment on how it's used. 🙂

Copy link
Contributor Author

@iorgamgabriel iorgamgabriel Aug 4, 2022

Choose a reason for hiding this comment

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

This is needed for intentSpeech.resolveActivity from
val intentSpeech = Intent(RecognizerIntent.ACTION_RECOGNIZE_SPEECH)

    return intentSpeech.resolveActivity(context.packageManager)?.let {
        PendingIntent.getActivity(
            context,
            REQUEST_CODE_VOICE, voiceIntent, PendingIntentUtils.defaultFlags
        )
    }

I can do this in manifest like in Focus and Fenix ,it's the same code or I can add this @SuppressLint("QueryPermissionsNeeded") to the method .
@jonalmeida

Copy link
Contributor

Choose a reason for hiding this comment

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

@iorgamgabriel it's not clear to me from the docs why the component or the apps need to have this permission. Could you share a link to where this requirement is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done I rewrote this code without resolveActivity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was needed for
val intentSpeech = Intent(RecognizerIntent.ACTION_RECOGNIZE_SPEECH)
intentSpeech.resolveActivity(context.packageManager)

to check if the phone has voice recognition.

Copy link
Contributor

Choose a reason for hiding this comment

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

Left an offline comment to this discuss this since the permission was removed from the change and no longer a concern for this PR.

@hneiva hneiva removed their request for review August 3, 2022 17:18
@hneiva
Copy link

hneiva commented Aug 3, 2022

No RelEng items, removing review request

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!
Seeing all previous comments were addressed I'll also give this a r+ but we should check with Jonathan for any other updates to be made before merging.

package="mozilla.components.feature.search">

<uses-permission
android:name="android.permission.QUERY_ALL_PACKAGES"
Copy link
Contributor

Choose a reason for hiding this comment

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

@iorgamgabriel it's not clear to me from the docs why the component or the apps need to have this permission. Could you share a link to where this requirement is?

Comment on lines 29 to 30
@VisibleForTesting
private var activityResultLauncher: ActivityResultLauncher<Intent>? = null
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we create this result launcher in onCreate to only use it later on in the same callback method?

Also, you use the @VisibleForTesting, but I don't see any tests that use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

It's still unclear to me why we need to hold this in a var in this fashion. :)

Copy link
Contributor

@jonalmeida jonalmeida left a comment

Choose a reason for hiding this comment

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

This looks good to me to land (after the logger lint is fixed). Great work! 🎉

Please make sure the land as a single change so we can squash the commits that land on main into one.

@iorgamgabriel iorgamgabriel force-pushed the 12565 branch 3 times, most recently from 3ddb57c to 4d4565e Compare August 11, 2022 08:41
@iorgamgabriel iorgamgabriel reopened this Aug 11, 2022
@iorgamgabriel iorgamgabriel added the 🛬 needs landing PRs that are ready to land label Aug 11, 2022
@mergify mergify bot merged commit 5a30a84 into main Aug 11, 2022
@bors bors bot deleted the 12565 branch August 11, 2022 10:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🛬 needs landing PRs that are ready to land
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement the common part of search widget in Android Components
5 participants