1210242 - Support assist icon for Search Activity in Lollipop

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Search Activity, defect)

Description

[Christian Schmitz (:tynn)]

AOSP removed the generic assist icon in Lollipop. If Fennec is the default application for ACTION_ASSIST intents, only the white circle is displayed. Instead an icon should be defined and displayed.

Comment 1

[Christian Schmitz (:tynn)]

Attachment: search_panel_view_assist.png

I've referenced icon_search_empty_firefox for this shot. Setting the same reference to 0 for pre Lollipop keeps things unchanged there.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8668189 [details]
search_panel_view_assist.png

Oh cool!

Anthony is our UX designer and would be a better person to ask here.

Comment 3

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8668189 [details]
search_panel_view_assist.png

Neat!

Though I'm not sure this is the best icon to use in this situation.

We have an app icon for the Search Activity. Could we try that? It's the orange circle with our Search icon inside.

Comment 4

[Christian Schmitz (:tynn)]

Attachment: search_panel_view_assist.png

Comment 5

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8671541 [details]
search_panel_view_assist.png

Nice! What's limiting the size here? Or is that the way all icons sit inside this white circle?

Comment 6

[Christian Schmitz (:tynn)]

Attachment: bug1210242.patch

The size is limited by the actual size of the icon itself. Additionally the white circle has its maximum expansion of the image. But I'm not entirely sure if there's a maximum size. Usually the size of the assist icon is larger than the launcher icon. That's why I chose the gray firefox for the first image.

The SystemUI reads the resource as integer and using 0 for pre Lollipop ensures the usage of the generic assist icon like before.

Comment 7

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8674385 [details] [diff] [review]
bug1210242.patch

Handing off to mcomella for the review here.

Comment 8

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Hey tynn,

So, just to clarify... are we using our largest available asset there? The icon looks a bit small and I know that the white area grows as the user drags further away. 

I just want to make sure we're aligned with system conventions here in terms of how big this icon should be appearing.

Comment 9

[Christian Schmitz (:tynn)]

I don't think there's a system convention for this. I don't even know of any official documentation. The SystemUI itself is heavily modified by some manufacturers; on my phone I'm not even able to replace the icon.

The search_launcher is the only icon like this, so I would say it's the largest available asset. I ran a test and replaced this 48dp icon with a 64dp version, which looked a little nicer. But this would mean to add additional assets.

In contrast, most of the icons used by other apps for the assist action are monochrome, making them look bigger in the context of the circle.

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8674385 [details] [diff] [review]
bug1210242.patch

I'm going to wait until this discussion is finished before reviewing – Christian, can you flag me for review when that is?

Comment 11

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Christian Schmitz (:tynn) from comment #9)
> I don't think there's a system convention for this. I don't even know of any
> official documentation. The SystemUI itself is heavily modified by some
> manufacturers; on my phone I'm not even able to replace the icon.
> 
> The search_launcher is the only icon like this, so I would say it's the
> largest available asset. I ran a test and replaced this 48dp icon with a
> 64dp version, which looked a little nicer. But this would mean to add
> additional assets.
> 
> In contrast, most of the icons used by other apps for the assist action are
> monochrome, making them look bigger in the context of the circle.

Right, I see what you mean now.

My concern is just visual polish in relation to this dynamic white circle that expands as the user drags their finger.

Could I see a build? it will help me see this on my devices to be sure it looks OK.

Comment 12

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Christian showed me more higher res screenshots and he confirmed that he's using our largest build icon with out scaling. 

So, I'm ok with this. It looks to be a nice added touch.

Thanks Christian!

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8674385 [details] [diff] [review]
bug1210242.patch

Review of attachment 8674385 [details] [diff] [review]:
-----------------------------------------------------------------

fwiw, it doesn't mean we can't have this but StackOverflow notes this is not official: http://stackoverflow.com/a/22571545

Please flag me for review again when you have a chance to respond.

::: mobile/android/search/manifests/SearchAndroidManifest_activities.xml.in
@@ +13,5 @@
>              </intent-filter>
>  
> +            <meta-data
> +                android:name="com.android.systemui.action_assist_icon"
> +                android:resource="@integer/search_assist_launch_res"/>

Why is `@integer` necessary? Why can't we use `@drawable` directly here?

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

The provided patch also doesn't work on my N9 5.0.1 – are there specific devices this works on, Christian?

(In reply to Christian Schmitz (:tynn) from comment #6)
> The SystemUI reads the resource as integer and using 0 for pre Lollipop
> ensures the usage of the generic assist icon like before.

If I'm understanding correctly, there is a generic icon pre-Lollipop and we don't want to overwrite that so we use integers to ensure we only replace the icon on Lollipop+? If so, add a comment to explain the use of integers and flag me for re-review please.

Comment 15

[Christian Schmitz (:tynn)]

(In reply to Michael Comella (:mcomella) from comment #13)
> fwiw, it doesn't mean we can't have this but StackOverflow notes this is not
> official: http://stackoverflow.com/a/22571545

There's no official documentation about this I know about, but considering the default implementation of SystemUI, it was not changed much the last years:
For Marshmallow+: https://android.googlesource.com/platform/frameworks/base/+/android-cts-6.0_r1/packages/SystemUI/src/com/android/systemui/assist/AssistManager.java
For Lollipop-: https://android.googlesource.com/platform/frameworks/base/+/android-5.1.1_r24/packages/SystemUI/src/com/android/systemui/SearchPanelView.java

Only the default icon was remove in Lollipop.

> > +                android:resource="@integer/search_assist_launch_res"/>
> 
> Why is `@integer` necessary? Why can't we use `@drawable` directly here?

<meta-data> is passed as a Bundle and the resource must be retrieved with Bundle.getInt(). I chose to use integer over drawable, because the value 0 for a drawable, or just not providing one for lower API levels, could result in an error when used as real drawable in code or xml.

(In reply to Michael Comella (:mcomella) from comment #14)
> The provided patch also doesn't work on my N9 5.0.1 – are there specific
> devices this works on, Christian?

What result did you see? The Google icon or none at all? Or did the assistant fail completely due to some exceptions?
This should work for any device using the AOSP implementation of SystemUI. If it's a modified version, it might not work though. But most importantly Firefox Search must be the only or the default assistant to make the icon show.

But the main reason I proposed this is, that it's almost impossible to break something. You only gain a better UX for all the devices supporting the default implementation, while nothing changes for all the others. At the end it's just a little meta data tag added to the Activity.

> If I'm understanding correctly, there is a generic icon pre-Lollipop and we
> don't want to overwrite that so we use integers to ensure we only replace
> the icon on Lollipop+? If so, add a comment to explain the use of integers
> and flag me for re-review please.

I will update the patch with it later tomorrow, but where can I set the flag for re-review of it? I didn't find it this morning.

Comment 16

[Christian Schmitz (:tynn)]

Attachment: bug1210242.patch

Ok, I've added the comment now. Thank you for all the feedback!

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: N9 blank assist intent

This is what I get, Christian.

NI self to try more devices tomorrow.

Comment 18

[Christian Schmitz (:tynn)]

Comment on attachment 8680402 [details]
N9 blank assist intent

I'm out of ideas. This should be a vanilla and work. Let's defer the testing until I've access to a Nexus device or walk through it together later today.

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Christian, I'll get to this tomorrow for sure.

Comment 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I can't repro this on my Lollipop N4 either.

Can you emulate a Nexus device on the emulator, Christian?

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8677657 [details] [diff] [review]
bug1210242.patch

Review of attachment 8677657 [details] [diff] [review]:
-----------------------------------------------------------------

Clear review flag until we figure out why this isn't working for me.

Comment 22

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I think I got it! I was building with gradle & Intellij which probably was not building the search activity – d'oh! It works now, yay! :)

Sorry about that Christian – let me get to that review.

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8677657 [details] [diff] [review]
bug1210242.patch

Review of attachment 8677657 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good – thanks Christian!

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/1dd1ef5e1150

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1dd1ef5e1150