Dynamically determine space between search engines in search engine bar

RESOLVED FIXED in Firefox 41

Status

()

Firefox for Android
General
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mcomella, Assigned: sebastian)

Tracking

unspecified
Firefox 41
All
Android
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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.
(Assignee)

Updated

2 years ago
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
(Assignee)

Comment 1

2 years ago
Created attachment 8615618 [details]
nexus4_bar_default.png
(Assignee)

Comment 2

2 years ago
Created attachment 8615619 [details]
nexus4_bar_reduced.png
(Assignee)

Comment 3

2 years ago
Created attachment 8615621 [details]
nexus9_bar_default.png
(Assignee)

Comment 4

2 years ago
Created attachment 8615622 [details]
nexus9_bar_overflow.png
(Assignee)

Comment 5

2 years ago
@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.
Flags: needinfo?(alam)
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?
Flags: needinfo?(alam) → needinfo?(s.kaspari)
(Assignee)

Comment 7

2 years ago
(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).
Flags: needinfo?(s.kaspari)
(Assignee)

Comment 8

2 years ago
Created attachment 8615753 [details] [diff] [review]
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).
Attachment #8615753 - Flags: review?(michael.l.comella)
(Reporter)

Comment 9

2 years ago
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?
Attachment #8615753 - Flags: review?(michael.l.comella) → review+
(Reporter)

Comment 10

2 years ago
My only real concern here is the wrap_content question.
(Assignee)

Comment 11

2 years ago
Created attachment 8617884 [details] [diff] [review]
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;
Attachment #8615753 - Attachment is obsolete: true
Attachment #8617884 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
Please run this through Try first.
Keywords: checkin-needed
(Assignee)

Comment 13

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e3b70ef6292
(Reporter)

Comment 14

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/70924ca52423
https://hg.mozilla.org/mozilla-central/rev/70924ca52423
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
You need to log in before you can comment on or make changes to this bug.