Set minimum line number on snippets in narrow layout.

Set minimum line number on snippets in narrow layout.

BUG=638543
Committed: https://crrev.com/d59c22f8ddbda4a7df3f220562165c025a6f4007
Cr-Commit-Position: refs/heads/master@{#414500}

[PEConn, 2016-08-22 21:37:38]

peconn@chromium.org changed reviewers:
+ bauerb@chromium.org, dgn@chromium.org

[PEConn, 2016-08-22 21:37:39]

PTAL!

https://codereview.chromium.org/2270443002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
(right):

https://codereview.chromium.org/2270443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:258:
mHeadlineTextView.setMinLines(narrow ? 3 : 1);
An alternate way to do this would be to change the snippet layout to be:

LinearLayout (vertical)
|-RelativeLayout
| |-Headline
| |-Snippet
| \-Thumbnail
\-Publisher

I think hopefully setting minimum lines will allow Android to have a bit more
flexibility laying the text out.

[PEConn, 2016-08-22 21:44:57]

On 2016/08/22 21:37:39, PEConn wrote:
> PTAL!
> 
>
https://codereview.chromium.org/2270443002/diff/1/chrome/android/java/src/org...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
> (right):
> 
>
https://codereview.chromium.org/2270443002/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:258:
> mHeadlineTextView.setMinLines(narrow ? 3 : 1);
> An alternate way to do this would be to change the snippet layout to be:
> 
> LinearLayout (vertical)
> |-RelativeLayout
> | |-Headline
> | |-Snippet
> | \-Thumbnail
> \-Publisher
> 
> I think hopefully setting minimum lines will allow Android to have a bit more
> flexibility laying the text out.

Example:
https://drive.google.com/a/google.com/file/d/0B_ABoZqo1irYQmh3MThZaDNwQmIyVF9...
(sorry it's not a public link)

[PEConn, 2016-08-22 21:44:59]

On 2016/08/22 21:37:39, PEConn wrote:
> PTAL!
> 
>
https://codereview.chromium.org/2270443002/diff/1/chrome/android/java/src/org...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
> (right):
> 
>
https://codereview.chromium.org/2270443002/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:258:
> mHeadlineTextView.setMinLines(narrow ? 3 : 1);
> An alternate way to do this would be to change the snippet layout to be:
> 
> LinearLayout (vertical)
> |-RelativeLayout
> | |-Headline
> | |-Snippet
> | \-Thumbnail
> \-Publisher
> 
> I think hopefully setting minimum lines will allow Android to have a bit more
> flexibility laying the text out.

Example:
https://drive.google.com/a/google.com/file/d/0B_ABoZqo1irYQmh3MThZaDNwQmIyVF9...
(sorry it's not a public link)

[Bernhard Bauer, 2016-08-23 08:50:13]

LGTM

On 2016/08/22 21:44:59, PEConn wrote:
> On 2016/08/22 21:37:39, PEConn wrote:
> > PTAL!
> > 
> >
>
https://codereview.chromium.org/2270443002/diff/1/chrome/android/java/src/org...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2270443002/diff/1/chrome/android/java/src/org...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:258:
> > mHeadlineTextView.setMinLines(narrow ? 3 : 1);
> > An alternate way to do this would be to change the snippet layout to be:
> > 
> > LinearLayout (vertical)
> > |-RelativeLayout
> > | |-Headline
> > | |-Snippet
> > | \-Thumbnail
> > \-Publisher
> > 
> > I think hopefully setting minimum lines will allow Android to have a bit
more
> > flexibility laying the text out.
> 
> Example:
>
https://drive.google.com/a/google.com/file/d/0B_ABoZqo1irYQmh3MThZaDNwQmIyVF9...
> (sorry it's not a public link)

Yeah... it's not great to have an empty line when the publisher would not
overlap the image anyway (and there could be even more edge cases, e.g. when the
font size is bigger), but the LinearLayout solution would also always restrict
the horizontal space.

[dgn, 2016-08-23 08:56:36]

https://codereview.chromium.org/2270443002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
(right):

https://codereview.chromium.org/2270443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:258:
mHeadlineTextView.setMinLines(narrow ? 3 : 1);
Shouldn't this take into account the minimal layout? We won't have a thumbnail
in this case, so we should not enforce the minlines constraint.

[Bernhard Bauer, 2016-08-23 09:27:52]

https://codereview.chromium.org/2270443002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
(right):

https://codereview.chromium.org/2270443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:258:
mHeadlineTextView.setMinLines(narrow ? 3 : 1);
On 2016/08/23 08:56:36, dgn wrote:
> Shouldn't this take into account the minimal layout? We won't have a thumbnail
> in this case, so we should not enforce the minlines constraint.

(This would be an excellent candidate for some UI render tests, BTW! 😃)

[PEConn, 2016-08-23 18:39:59]

peconn@chromium.org changed reviewers:
+ jbudorick@chromium.org

[PEConn, 2016-08-23 18:39:59]

jbudorick@ - Could you please look at the change in RenderUtils.java

(also bauerb@ & dgn@, I seem to have picked up a change to the new tab page
behaviour - about which sections are enabled in the NewTabPageTest)

https://codereview.chromium.org/2270443002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
(right):

https://codereview.chromium.org/2270443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:258:
mHeadlineTextView.setMinLines(narrow ? 3 : 1);
On 2016/08/23 08:56:36, dgn wrote:
> Shouldn't this take into account the minimal layout? We won't have a thumbnail
> in this case, so we should not enforce the minlines constraint.

Done.

https://codereview.chromium.org/2270443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:258:
mHeadlineTextView.setMinLines(narrow ? 3 : 1);
On 2016/08/23 09:27:52, Bernhard Bauer wrote:
> On 2016/08/23 08:56:36, dgn wrote:
> > Shouldn't this take into account the minimal layout? We won't have a
thumbnail
> > in this case, so we should not enforce the minlines constraint.
> 
> (This would be an excellent candidate for some UI render tests, BTW! 😃)

Done.

[jbudorick, 2016-08-23 18:42:39]

On 2016/08/23 18:39:59, PEConn wrote:
> jbudorick@ - Could you please look at the change in RenderUtils.java

RenderUtils.java lgtm

> 
> (also bauerb@ & dgn@, I seem to have picked up a change to the new tab page
> behaviour - about which sections are enabled in the NewTabPageTest)
> 
>
https://codereview.chromium.org/2270443002/diff/1/chrome/android/java/src/org...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
> (right):
> 
>
https://codereview.chromium.org/2270443002/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:258:
> mHeadlineTextView.setMinLines(narrow ? 3 : 1);
> On 2016/08/23 08:56:36, dgn wrote:
> > Shouldn't this take into account the minimal layout? We won't have a
thumbnail
> > in this case, so we should not enforce the minlines constraint.
> 
> Done.
> 
>
https://codereview.chromium.org/2270443002/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:258:
> mHeadlineTextView.setMinLines(narrow ? 3 : 1);
> On 2016/08/23 09:27:52, Bernhard Bauer wrote:
> > On 2016/08/23 08:56:36, dgn wrote:
> > > Shouldn't this take into account the minimal layout? We won't have a
> thumbnail
> > > in this case, so we should not enforce the minlines constraint.
> > 
> > (This would be an excellent candidate for some UI render tests, BTW! 😃)
> 
> Done.

[PEConn, 2016-08-23 19:11:25]

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

[commit-bot: I haz the power, 2016-08-23 19:11:47]

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-08-23 19:48:58]

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

[commit-bot: I haz the power, 2016-08-23 19:49:00]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[PEConn, 2016-08-23 20:48:55]

Since I added a lot of goldens (I was kinda piggybacking on this CL), the
interesting ones are:
chrome/test/data/android/render_tests/ArticleSnippetsTest.snippets.Nexus_5X.port.png
chrome/test/data/android/render_tests/ArticleSnippetsTest.snippets_narrow.Nexus_5.port.png
chrome/test/data/android/render_tests/ArticleSnippetsTest.snippets_narrow.Nexus_5X.port.png

[PEConn, 2016-08-23 21:34:11]

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

[commit-bot: I haz the power, 2016-08-23 21:34:52]

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-08-23 23:36:34]

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

[commit-bot: I haz the power, 2016-08-23 23:36:35]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dgn, 2016-08-24 10:49:22]

>
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:258:
> > > mHeadlineTextView.setMinLines(narrow ? 3 : 1);
> > > An alternate way to do this would be to change the snippet layout to be:
> > > 
> > > LinearLayout (vertical)
> > > |-RelativeLayout
> > > | |-Headline
> > > | |-Snippet
> > > | \-Thumbnail
> > > \-Publisher
> > > 
> > > I think hopefully setting minimum lines will allow Android to have a bit
> > > more flexibility laying the text out.
> > 
> > Example:
> >
> >
https://drive.google.com/a/google.com/file/d/0B_ABoZqo1irYQmh3MThZaDNwQmIyVF9...
> > (sorry it's not a public link)
> 
> Yeah... it's not great to have an empty line when the publisher would not
> overlap the image anyway (and there could be even more edge cases, e.g. when
the
> font size is bigger), but the LinearLayout solution would also always restrict
> the horizontal space.

On N6P with font size set to the smallest the min-3-lines still keep the
publisher string below the thumbnail so that's good I suppose. And with the
biggest it still looks all right. But that's not a strong constraint.

Something more explicit in the layout (what you said, or maybe
`android:layout_below="@id/thumbnail"`) would be nice but I don't think we will
run into issues with the current code so lgtm.

https://codereview.chromium.org/2270443002/diff/50001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
(right):

https://codereview.chromium.org/2270443002/diff/50001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:259:
mHeadlineTextView.setMaxLines(narrow ? 4 : 2);
nit: keep this close to line where we set the MinLines?

https://codereview.chromium.org/2270443002/diff/50001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:270:
// header lines to prevent overlap.
nit: clarify that it's to make the publisher text not overlap the thumbnail.
Right now it's not clear what is overlapping.

[PEConn, 2016-08-24 14:02:22]

On 2016/08/24 10:49:22, dgn wrote:
> >
>
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:258:
> > > > mHeadlineTextView.setMinLines(narrow ? 3 : 1);
> > > > An alternate way to do this would be to change the snippet layout to be:
> > > > 
> > > > LinearLayout (vertical)
> > > > |-RelativeLayout
> > > > | |-Headline
> > > > | |-Snippet
> > > > | \-Thumbnail
> > > > \-Publisher
> > > > 
> > > > I think hopefully setting minimum lines will allow Android to have a bit
> > > > more flexibility laying the text out.
> > > 
> > > Example:
> > >
> > >
>
https://drive.google.com/a/google.com/file/d/0B_ABoZqo1irYQmh3MThZaDNwQmIyVF9...
> > > (sorry it's not a public link)
> > 
> > Yeah... it's not great to have an empty line when the publisher would not
> > overlap the image anyway (and there could be even more edge cases, e.g. when
> the
> > font size is bigger), but the LinearLayout solution would also always
restrict
> > the horizontal space.
> 
> On N6P with font size set to the smallest the min-3-lines still keep the
> publisher string below the thumbnail so that's good I suppose. And with the
> biggest it still looks all right. But that's not a strong constraint.
> 
> Something more explicit in the layout (what you said, or maybe
> `android:layout_below="@id/thumbnail"`) would be nice but I don't think we
will
> run into issues with the current code so lgtm.

I did originally try to implement this with just a RelativeLayout, but it ended
up either always overlapping in various configurations or stretching the image
to fill the space until the publisher.

[PEConn, 2016-08-24 15:57:14]

https://codereview.chromium.org/2270443002/diff/50001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
(right):

https://codereview.chromium.org/2270443002/diff/50001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:259:
mHeadlineTextView.setMaxLines(narrow ? 4 : 2);
On 2016/08/24 10:49:22, dgn wrote:
> nit: keep this close to line where we set the MinLines?

Done.

https://codereview.chromium.org/2270443002/diff/50001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:270:
// header lines to prevent overlap.
On 2016/08/24 10:49:22, dgn wrote:
> nit: clarify that it's to make the publisher text not overlap the thumbnail.
> Right now it's not clear what is overlapping.
> 

Done.

[PEConn, 2016-08-24 22:15:27]

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

[commit-bot: I haz the power, 2016-08-24 22:16:27]

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-08-24 23:51:46]

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

[commit-bot: I haz the power, 2016-08-24 23:51:47]

Dry run: This issue passed the CQ dry run.

[PEConn, 2016-08-25 18:58:52]

The CQ bit was checked by peconn@chromium.org

[PEConn, 2016-08-25 18:58:52]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
jbudorick@chromium.org, dgn@chromium.org
Link to the patchset: https://codereview.chromium.org/2270443002/#ps70001
(title: "Nits.")

[commit-bot: I haz the power, 2016-08-25 19:00:00]

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-08-25 19:06:11]

Committed patchset #5 (id:70001)

[commit-bot: I haz the power, 2016-08-25 19:09:22]

Description was changed from

==========
Set minimum line number on snippets in narrow layout.

BUG=638543
==========

to

==========
Set minimum line number on snippets in narrow layout.

BUG=638543

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

[commit-bot: I haz the power, 2016-08-25 19:09:24]

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