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.

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

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 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 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?

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 on attachment 8674385 [details] [diff] [review] bug1210242.patch Handing off to mcomella for the review here.

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.

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 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?

(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.

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 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?

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.

(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.

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

This is what I get, Christian. NI self to try more devices tomorrow.

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.

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

I can't repro this on my Lollipop N4 either. Can you emulate a Nexus device on the emulator, Christian?

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.

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 on attachment 8677657 [details] [diff] [review] bug1210242.patch Review of attachment 8677657 [details] [diff] [review]: ----------------------------------------------------------------- Looks good – thanks Christian!