1158295 - Dynamically determine space between search engines in search engine bar

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Follow-up to bug 1137483.

So we can fit all the search engines in the search engine bar if it doesn't look too bad.

Have a minimum margin between items so it doesn't look terrible, based on antlam feedback.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: nexus4_bar_default.png

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: nexus4_bar_reduced.png

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: nexus9_bar_default.png

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: nexus9_bar_overflow.png

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

@Anthony: What is your opinion on the minimum margin between the items? I added some screenshots of my current implementation, see nexus9_bar_overflow.png for the minimum margin being applied.

Comment 6

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

I spec'd this out originally on the N5 and I had each engine's button sized at 72dp x 48dp. The icon then centered itself inside (which I spec'd out in bug 1156917). These would then be placed one after another with no extra padding/margin. That way, smaller phones would naturally have to horizontally scroll.

That was the minimum I had in mind. So, in a sense, I didn't spec out margins in between buttons.

Does that help?

(In reply to :Sebastian Kaspari from comment #4)
> Created attachment 8615622 [details]
> nexus9_bar_overflow.png

this seems alright :) What values are you using here? 

(In reply to :Sebastian Kaspari from comment #2)
> Created attachment 8615619 [details]
> nexus4_bar_reduced.png

I like this! Is this what happens when the user only has 3 engines?

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Anthony Lam (:antlam) from comment #6) 
> > Created attachment 8615622 [details]
> > nexus9_bar_overflow.png
> 
> this seems alright :) What values are you using here? 

The search engine bar has a height of 48dp. The icons are 24dp x 24dp and are centered in a container (the actual button) that has a minimum width of 72dp. So yeah, the minimum size of the button should be exactly your 72dp x 48dp.


(In reply to Anthony Lam (:antlam) from comment #6) 
> > Created attachment 8615619 [details]
> > nexus4_bar_reduced.png
> 
> I like this! Is this what happens when the user only has 3 engines?

Exactly! If there's more space available then the buttons will grow to fill this space (e.g. with default number of engines on N9: nexus9_bar_default.png).

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1158295-dynamic-space.patch

As a result of this patch I have been able to remove SearchEngineBarContainer (the SearchEngineBar now spans the whole width and takes care of drawing the divider).

Comment 9

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

Comment on attachment 8615753 [details] [diff] [review]
1158295-dynamic-space.patch

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

Sweet - I love how it's even simpler than before.

::: mobile/android/base/home/SearchEngineBar.java
@@ +27,5 @@
>  public class SearchEngineBar extends TwoWayView
>                               implements AdapterView.OnItemClickListener {
>      private static final String LOGTAG = "Gecko" + SearchEngineBar.class.getSimpleName();
>  
> +    private static final float ICON_CONTAINER_MIN_SIZE = 72;

nit: -> ICON_CONTAINER_MIN_WIDTH_DP

@@ +47,5 @@
>          super(context, attrs);
>  
> +        dividerPaint = new Paint();
> +        dividerPaint.setColor(getResources().getColor(R.color.divider_light));
> +        dividerPaint.setStyle(Paint.Style.FILL_AND_STROKE);

Why FILL_AND_STROKE? I haven't used it before.

@@ +96,5 @@
> +            );
> +
> +            if (desiredIconContainerSize != iconContainerWidth) {
> +                iconContainerWidth = desiredIconContainerSize;
> +                adapter.notifyDataSetChanged();

I'm assuming this forces the adapter to call getView on all of the visible adapter items? Nice.

::: mobile/android/base/resources/layout/browser_search.xml
@@ +19,5 @@
>              android:layout_height="0dp"
>              android:layout_weight="1" />
>  
> +    <!--
> +         listSelector is too slow for showing pressed state

nit: Our convention is to have "<!--" on the same line as the starting text.

::: mobile/android/base/resources/layout/search_engine_bar_item.xml
@@ +10,5 @@
>       Note: the layout_height values are shared with the parent
> +     View (browser_search at the time of this writing).
> +
> +      The actual width of the FrameLayout is calculated at runtime by the
> +      SearchEngineBar class to spread the icons across the device's width. -->

nit: This text looks slightly more indented than the text above it.

@@ +15,4 @@
>  <FrameLayout
>      xmlns:android="http://schemas.android.com/apk/res/android"
>      android:id="@+id/search_engine_icon_container"
> +    android:layout_height="wrap_content"

This would reduce the hit area of the button to be less than the size of the parent, if I'm not mistaken - why wrap_content?

Comment 10

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

My only real concern here is the wrap_content question.

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1158295-dynamic-space-v2.patch

I uploaded a new patch addressing the review points.

(In reply to Michael Comella (:mcomella) from comment #9)
> Why FILL_AND_STROKE? I haven't used it before.

I changed the divider drawing from drawLine to drawRect because technically a 1dp line can span multiple pixels in height (depending on density). Afaik the default is that drawRect only draws the outline but in this case we want to draw a filled rect.

> I'm assuming this forces the adapter to call getView on all of the visible
> adapter items? Nice.

Exactly! It's not 100% what it is made for but it works. :)

> This would reduce the hit area of the button to be less than the size of the
> parent, if I'm not mistaken - why wrap_content?

I agree. This should be match_parent. I changed it in the XML and also in SearchEngineBar.java where we calculate the new LayoutParams.


In addition to the mentioned points I changed getWidth() to getMeasuredWidth() here as it is not guaranteed that the width is already set at this point. And in fact I could reproduce a bug where we would always use the minimum width because getWidth() returned 0 at this point in time.

@@ -83,27 +84,34 @@ public class SearchEngineBar extends Two
>  
>      @Override
>      protected void onMeasure(int widthMeasureSpec, int heightMeasureSpec) {
>          super.onMeasure(widthMeasureSpec, heightMeasureSpec);
>  
>          final int searchEngineCount = getCount();
>  
>          if (searchEngineCount > 0) {
> -            final float availableWidthPerContainer = getWidth() / searchEngineCount;
> +            final float availableWidthPerContainer = getMeasuredWidth() / searchEngineCount;

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Please run this through Try first.

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e3b70ef6292

Comment 14

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

https://hg.mozilla.org/integration/fx-team/rev/70924ca52423

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/70924ca52423