Http Bad: Put icon on the left of warning message and make value and label in one line on Android

Http Bad: Put icon on the left of warning message and make value and label in one line on Android

The order of elements in http bad warning message is: icon, value, label. This
CL puts icon on the left of autofill entry in an advised dropdown layout called
dropdown_item_left_icon.xml. This CL also adjusts value and label to be in one
line.

specs: go/fns-ui-spec

BUG=662297
Committed: https://crrev.com/a6070555569eaaa95f95c259542781c7ccc6e777
Cr-Commit-Position: refs/heads/master@{#435823}

[robbyflya330, 2016-11-18 00:49:00]

robbyflya330@gmail.com changed reviewers:
+ robbyflya330@gmail.com

[lshang, 2016-11-18 04:16:45]

Description was changed from

==========
Http Bad: Put icon on the left of warning message and make value and label in
one line


horizontal


change


rebase


some change

BUG=
==========

to

==========
Http Bad: Put icon on the left of warning message and make value and label in
one line




BUG=
==========

[lshang, 2016-11-18 04:16:45]

lshang@chromium.org changed reviewers:
- robbyflya330@gmail.com

[lshang, 2016-11-20 11:31:05]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-20 11:31:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-20 12:10:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-20 12:10:12]

Dry run: This issue passed the CQ dry run.

[lshang, 2016-11-27 05:23:46]

Patchset #2 (id:20001) has been deleted

[lshang, 2016-11-27 05:23:50]

Patchset #1 (id:1) has been deleted

[lshang, 2016-11-27 05:28:01]

Description was changed from

==========
Http Bad: Put icon on the left of warning message and make value and label in
one line




BUG=
==========

to

==========
Http Bad: Put icon on the left of warning message and make value and label in
one line on Android

The order of elements in http bad warning message is: icon, value, label. This
CL puts icon on the left of autofill entry in an advised dropdown layout called
dropdown_item_left_icon.xml. This CL also adjusts value and label to be in one
line.

specs: go/fns-ui-spec

BUG=662297
==========

[lshang, 2016-11-27 05:34:20]

lshang@chromium.org changed reviewers:
+ mathp@chromium.org, tedchoc@chromium.org

[lshang, 2016-11-27 05:34:21]

Hi Mathieu and Ted, please take a look at this CL. I made a new layout file to
put the icon in front of label and sublabel in the autofill dropdown. I can't
think of other ways to adjust icon's position programmatically in the linear
layout, so please let me know your thoughts about this. Thanks!

[Mathieu, 2016-11-28 15:04:01]

On 2016/11/27 05:34:21, lshang wrote:
> Hi Mathieu and Ted, please take a look at this CL. I made a new layout file to
> put the icon in front of label and sublabel in the autofill dropdown. I can't
> think of other ways to adjust icon's position programmatically in the linear
> layout, so please let me know your thoughts about this. Thanks!

I'm slightly out of my depth. Ted can you have a look whether things are
possible without creating a new layout file entirely?

[Ted C, 2016-11-28 18:29:12]

https://codereview.chromium.org/2510283002/diff/40001/ui/android/java/res/lay...
File ui/android/java/res/layout/dropdown_item_left_icon.xml (right):

https://codereview.chromium.org/2510283002/diff/40001/ui/android/java/res/lay...
ui/android/java/res/layout/dropdown_item_left_icon.xml:16: <ImageView
instead of creating a different xml file, just add an ImageView to the existing
dropdown_item.xml.

I would name them start_dropdown_icon, end_dropdown_icon (or
dropdown_icon_start, dropdown_icon_end).

Left/right isn't correct either because in RTL they will be flipped around.  So
you'll want to use start/end.

https://codereview.chromium.org/2510283002/diff/40001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):

https://codereview.chromium.org/2510283002/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:62: layout =
inflater.inflate(R.layout.dropdown_item_left_icon, null);
You can't actually use different layouts.  getView attempts to reuse views via
the passed in convertView.  By using different layouts, we are not going to be
able to distinguish between the two types and then are going to need to create a
new view each time, which is not a good thing.

If you do want to use different layouts (although I don't recommend it in this
case), then you'd need to overwrite getItemViewType and getViewTypeCount on the
adapter to inform them of these different types and have it reuse them
accordingly.

By using the same layout (with potentially two imageviews), you'll need to be
careful to always reset any previous state.

i.e. something like this:

ImageView iconViewStart = (ImageView) layout.findViewById(
        R.id.dropdown_icon_start);
ImageView iconViewEnd = (ImageView) layout.findViewById(
        R.id.dropdown_icon_end);
if (isIconAtStart()) {
    iconViewEnd.setVisibility(GONE);
} else {
    iconViewStart.setVisibility(GONE);
}

ImageView iconView = isIconAtStart() ? iconViewStart : iconViewEnd;

... existing image behavior ...

https://codereview.chromium.org/2510283002/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:99:
labelView.setSingleLine(!item.isMultilineLabel());
Do the labels align correctly per the mocks with this change? 

In the mocks, it looks like it should be
+----------------------------------+
| Label ...... ws ....... SubLabel |
+----------------------------------+

But looking at this, it seems like it would be:
+----------------------------------+
| Label  SubLabel ....... ws ..... |
+----------------------------------+

Because dropdown_label's width is wrap_content, it doesn't seem like it would
do what you want.

I think in this case, you'd want to set the layout params (based on the is all
on one line flag) to be:

if (all on one line) {
     labelView.setLayoutParams(new LinearLayout.LayoutParams(
             0, LayoutParams.WRAP_CONTENT, 1));
} else {
     labelView.setLayoutParams(new LinearLayout.LayoutParams(
             LayoutParams.WRAP_CONTENT, LayoutParams.WRAP_CONTENT, 0));
}

I "believe" that should get it to what you want.

https://codereview.chromium.org/2510283002/diff/40001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/DropdownItem.java (right):

https://codereview.chromium.org/2510283002/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownItem.java:49: int
getLabelSublabelOrientation();
instead of passing an internal layout param from LinearLayout, I think this
would be better to get something along the lines of stating the sublabel should
appear on the same line as the label.

boolean isLabelAndSublabelOnSameLine();  <-- I really hope there is a better way
to say it than that.  that is quite long.

The idea is that if we want to not use a linearlayout for these at some point
then it would be good to not have to change these behaviors.

[lshang, 2016-11-30 10:35:44]

https://codereview.chromium.org/2510283002/diff/40001/ui/android/java/res/lay...
File ui/android/java/res/layout/dropdown_item_left_icon.xml (right):

https://codereview.chromium.org/2510283002/diff/40001/ui/android/java/res/lay...
ui/android/java/res/layout/dropdown_item_left_icon.xml:16: <ImageView
On 2016/11/28 18:29:11, Ted C wrote:
> instead of creating a different xml file, just add an ImageView to the
existing
> dropdown_item.xml.
> 
> I would name them start_dropdown_icon, end_dropdown_icon (or
> dropdown_icon_start, dropdown_icon_end).
> 
> Left/right isn't correct either because in RTL they will be flipped around. 
So
> you'll want to use start/end.

That's a good idea! Done!

https://codereview.chromium.org/2510283002/diff/40001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):

https://codereview.chromium.org/2510283002/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:62: layout =
inflater.inflate(R.layout.dropdown_item_left_icon, null);
On 2016/11/28 18:29:11, Ted C wrote:
> You can't actually use different layouts.  getView attempts to reuse views via
> the passed in convertView.  By using different layouts, we are not going to be
> able to distinguish between the two types and then are going to need to create
a
> new view each time, which is not a good thing.
> 
> If you do want to use different layouts (although I don't recommend it in this
> case), then you'd need to overwrite getItemViewType and getViewTypeCount on
the
> adapter to inform them of these different types and have it reuse them
> accordingly.
> 
> By using the same layout (with potentially two imageviews), you'll need to be
> careful to always reset any previous state.
> 
> i.e. something like this:
> 
> ImageView iconViewStart = (ImageView) layout.findViewById(
>         R.id.dropdown_icon_start);
> ImageView iconViewEnd = (ImageView) layout.findViewById(
>         R.id.dropdown_icon_end);
> if (isIconAtStart()) {
>     iconViewEnd.setVisibility(GONE);
> } else {
>     iconViewStart.setVisibility(GONE);
> }
> 
> ImageView iconView = isIconAtStart() ? iconViewStart : iconViewEnd;
> 
> ... existing image behavior ...

Done.

https://codereview.chromium.org/2510283002/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:99:
labelView.setSingleLine(!item.isMultilineLabel());
On 2016/11/28 18:29:12, Ted C wrote:
> Do the labels align correctly per the mocks with this change? 
> 
> In the mocks, it looks like it should be
> +----------------------------------+
> | Label ...... ws ....... SubLabel |
> +----------------------------------+
> 
> But looking at this, it seems like it would be:
> +----------------------------------+
> | Label  SubLabel ....... ws ..... |
> +----------------------------------+
> 
> Because dropdown_label's width is wrap_content, it doesn't seem like it would
> do what you want.
> 
> I think in this case, you'd want to set the layout params (based on the is all
> on one line flag) to be:
> 
> if (all on one line) {
>      labelView.setLayoutParams(new LinearLayout.LayoutParams(
>              0, LayoutParams.WRAP_CONTENT, 1));
> } else {
>      labelView.setLayoutParams(new LinearLayout.LayoutParams(
>              LayoutParams.WRAP_CONTENT, LayoutParams.WRAP_CONTENT, 0));
> }
> 
> I "believe" that should get it to what you want.

You're amazing! Yeah I tried fixing it by gravity and it didn't work.
Layout_weight is interesting. Now this is in line with the mocks. And I took out
the else part to keep the padding before label in normal entry.

https://codereview.chromium.org/2510283002/diff/40001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/DropdownItem.java (right):

https://codereview.chromium.org/2510283002/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownItem.java:49: int
getLabelSublabelOrientation();
On 2016/11/28 18:29:12, Ted C wrote:
> instead of passing an internal layout param from LinearLayout, I think this
> would be better to get something along the lines of stating the sublabel
should
> appear on the same line as the label.
> 
> boolean isLabelAndSublabelOnSameLine();  <-- I really hope there is a better
way
> to say it than that.  that is quite long.
> 
> The idea is that if we want to not use a linearlayout for these at some point
> then it would be good to not have to change these behaviors.

Yeah your point makes sense. but I can't think of better names at the moment,
done with this name.

[lshang, 2016-11-30 10:37:47]

Patchset #3 (id:80001) has been deleted

[Ted C, 2016-11-30 22:21:33]

lgtm w/ a couple followups

thanks!

https://codereview.chromium.org/2510283002/diff/100001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):

https://codereview.chromium.org/2510283002/diff/100001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:92: }
you'll need an else, that is:

wrapper.setOrientation(LinearLayout.VERTICAL);

Since the items can be reused.

https://codereview.chromium.org/2510283002/diff/100001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:103: }
same here for the else,

one with:

new LinearLayout.LayoutParams(LayoutParams.WRAP_CONTENT,
LayoutParams.WRAP_CONTENT)

https://codereview.chromium.org/2510283002/diff/100001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/DropdownItem.java (right):

https://codereview.chromium.org/2510283002/diff/100001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownItem.java:51: * Returns whether the
icon should be displayed on the left, before label
s/on the left/at the start

[lshang, 2016-12-01 08:50:43]

Patchset #4 (id:120001) has been deleted

[lshang, 2016-12-01 08:52:53]

Thanks Ted, but I still have a question in the comment, take a look please?

https://codereview.chromium.org/2510283002/diff/100001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):

https://codereview.chromium.org/2510283002/diff/100001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:92: }
On 2016/11/30 22:21:33, Ted C wrote:
> you'll need an else, that is:
> 
> wrapper.setOrientation(LinearLayout.VERTICAL);
> 
> Since the items can be reused.

Done.

https://codereview.chromium.org/2510283002/diff/100001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:103: }
On 2016/11/30 22:21:33, Ted C wrote:
> same here for the else,
> 
> one with:
> 
> new LinearLayout.LayoutParams(LayoutParams.WRAP_CONTENT,
> LayoutParams.WRAP_CONTENT)

Done with setMarginStart() and setMarginEnd() afterwards. I'm also a little
worried that wouldn't the default layout params in .xml file be overridden by
creating a new LayoutParams here for normal entries?

https://codereview.chromium.org/2510283002/diff/100001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/DropdownItem.java (right):

https://codereview.chromium.org/2510283002/diff/100001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownItem.java:51: * Returns whether the
icon should be displayed on the left, before label
On 2016/11/30 22:21:33, Ted C wrote:
> s/on the left/at the start

Done.

[Ted C, 2016-12-01 17:39:24]

https://codereview.chromium.org/2510283002/diff/100001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):

https://codereview.chromium.org/2510283002/diff/100001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:103: }
On 2016/12/01 08:52:52, lshang wrote:
> On 2016/11/30 22:21:33, Ted C wrote:
> > same here for the else,
> > 
> > one with:
> > 
> > new LinearLayout.LayoutParams(LayoutParams.WRAP_CONTENT,
> > LayoutParams.WRAP_CONTENT)
> 
> Done with setMarginStart() and setMarginEnd() afterwards. I'm also a little
> worried that wouldn't the default layout params in .xml file be overridden by
> creating a new LayoutParams here for normal entries?

Ahhh..you are spot on there.

I think instead of creating a new LayoutParams ever, we could do this:

LinearLayout.LayoutParams layoutParams =
        (LinearLayout.LayoutParams) labelView.getLayoutParams();
if (item.isLabelAndSublabelOnSameLine()) {
    layoutParams.weight = 1;
    layoutParams.width = 0;
    layoutParams.height = LayoutParams.WRAP_CONTENT;
} else {
    layoutParams.weight = 0;
    layoutParams.width = LayoutParams.WRAP_CONTENT;
    layoutParams.height = LayoutParams.WRAP_CONTENT;
}

Then you're not clobbering margins ever.  But if you need to
clear the margins in the isLabelAndSublabelOnSameLine(), then
your change is correct.

https://codereview.chromium.org/2510283002/diff/140001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):

https://codereview.chromium.org/2510283002/diff/140001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:108: int label_margin =
mContext.getResources().getDimensionPixelSize(
if needed:

s/label_margin/labelMargin

[lshang, 2016-12-02 00:44:19]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 00:44:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lshang, 2016-12-02 00:49:08]

Thanks Ted! ping Mathieu (●'◡'●)

https://codereview.chromium.org/2510283002/diff/100001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):

https://codereview.chromium.org/2510283002/diff/100001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:103: }
On 2016/12/01 17:39:24, Ted C wrote:
> On 2016/12/01 08:52:52, lshang wrote:
> > On 2016/11/30 22:21:33, Ted C wrote:
> > > same here for the else,
> > > 
> > > one with:
> > > 
> > > new LinearLayout.LayoutParams(LayoutParams.WRAP_CONTENT,
> > > LayoutParams.WRAP_CONTENT)
> > 
> > Done with setMarginStart() and setMarginEnd() afterwards. I'm also a little
> > worried that wouldn't the default layout params in .xml file be overridden
by
> > creating a new LayoutParams here for normal entries?
> 
> Ahhh..you are spot on there.
> 
> I think instead of creating a new LayoutParams ever, we could do this:
> 
> LinearLayout.LayoutParams layoutParams =
>         (LinearLayout.LayoutParams) labelView.getLayoutParams();
> if (item.isLabelAndSublabelOnSameLine()) {
>     layoutParams.weight = 1;
>     layoutParams.width = 0;
>     layoutParams.height = LayoutParams.WRAP_CONTENT;
> } else {
>     layoutParams.weight = 0;
>     layoutParams.width = LayoutParams.WRAP_CONTENT;
>     layoutParams.height = LayoutParams.WRAP_CONTENT;
> }
> 
> Then you're not clobbering margins ever.  But if you need to
> clear the margins in the isLabelAndSublabelOnSameLine(), then
> your change is correct.

Yep according to the mocks there's only a 8dp icon margin between icon and value
in the isLabelAndSublabelOnSameLine(), so value's margins need to be cleared.

https://codereview.chromium.org/2510283002/diff/140001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):

https://codereview.chromium.org/2510283002/diff/140001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:108: int label_margin =
mContext.getResources().getDimensionPixelSize(
On 2016/12/01 17:39:24, Ted C wrote:
> if needed:
> 
> s/label_margin/labelMargin

Done.

[commit-bot: I haz the power, 2016-12-02 01:24:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 01:24:25]

Dry run: This issue passed the CQ dry run.

[Mathieu, 2016-12-02 02:01:42]

lgtm, thanks!

[lshang, 2016-12-02 02:02:48]

The CQ bit was checked by lshang@chromium.org

[lshang, 2016-12-02 02:02:48]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2510283002/#ps160001
(title: "minor change")

[commit-bot: I haz the power, 2016-12-02 02:03:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 02:07:30]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1480644168367200,
"parent_rev": "9780f01bcb773d6082d8424576b59745b7c4abf9", "commit_rev":
"0ba16f3c8ca5c926f931c4255810ae78c7530cdc"}

[commit-bot: I haz the power, 2016-12-02 02:09:07]

Description was changed from

==========
Http Bad: Put icon on the left of warning message and make value and label in
one line on Android

The order of elements in http bad warning message is: icon, value, label. This
CL puts icon on the left of autofill entry in an advised dropdown layout called
dropdown_item_left_icon.xml. This CL also adjusts value and label to be in one
line.

specs: go/fns-ui-spec

BUG=662297
==========

to

==========
Http Bad: Put icon on the left of warning message and make value and label in
one line on Android

The order of elements in http bad warning message is: icon, value, label. This
CL puts icon on the left of autofill entry in an advised dropdown layout called
dropdown_item_left_icon.xml. This CL also adjusts value and label to be in one
line.

specs: go/fns-ui-spec

BUG=662297
==========

[commit-bot: I haz the power, 2016-12-02 02:09:09]

Committed patchset #5 (id:160001)

[commit-bot: I haz the power, 2016-12-02 02:12:50]

Description was changed from

==========
Http Bad: Put icon on the left of warning message and make value and label in
one line on Android

The order of elements in http bad warning message is: icon, value, label. This
CL puts icon on the left of autofill entry in an advised dropdown layout called
dropdown_item_left_icon.xml. This CL also adjusts value and label to be in one
line.

specs: go/fns-ui-spec

BUG=662297
==========

to

==========
Http Bad: Put icon on the left of warning message and make value and label in
one line on Android

The order of elements in http bad warning message is: icon, value, label. This
CL puts icon on the left of autofill entry in an advised dropdown layout called
dropdown_item_left_icon.xml. This CL also adjusts value and label to be in one
line.

specs: go/fns-ui-spec

BUG=662297

Committed: https://crrev.com/a6070555569eaaa95f95c259542781c7ccc6e777
Cr-Commit-Position: refs/heads/master@{#435823}
==========

[commit-bot: I haz the power, 2016-12-02 02:12:52]

Patchset 5 (id:??) landed as
https://crrev.com/a6070555569eaaa95f95c259542781c7ccc6e777
Cr-Commit-Position: refs/heads/master@{#435823}