Use a plain UILabel for answers in suggest detail text.

Use a plain UILabel for answers in suggest detail text.

This is to support truncation with ellipses instead of the truncation by
fading used for regular suggestion detail text. The use of ellipses is
preferred for answers because they sometimes contain multi-line text and
the use of an ellipse looks more natural.

BUG=591803
Review-Url: https://codereview.chromium.org/2721383002
Cr-Commit-Position: refs/heads/master@{#454325}
Committed: https://chromium.googlesource.com/chromium/src/+/eb8df71ea60fb11ce27598039c85b9a7256f963f

[Justin Donnelly, 2017-03-01 17:31:24]

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

[commit-bot: I haz the power, 2017-03-01 17:31:43]

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

[Justin Donnelly, 2017-03-01 17:32:12]

Description was changed from

==========
Use a plain UILabel for answers in suggest detail text.

This is to support truncation with ellipses instead of truncation by
fading with regular suggestion detail text. The use of ellipses is
preferred for answers because they sometimes contain multi-line text and
the use of an ellipse looks more natural.

BUG=591803
==========

to

==========
Use a plain UILabel for answers in suggest detail text.

This is to support truncation with ellipses instead of the truncation by
fading used for regular suggestion detail text. The use of ellipses is
preferred for answers because they sometimes contain multi-line text and
the use of an ellipse looks more natural.

BUG=591803
==========

[Justin Donnelly, 2017-03-01 17:42:19]

jdonnelly@chromium.org changed reviewers:
+ justincohen@chromium.org

[Justin Donnelly, 2017-03-01 17:42:20]

https://codereview.chromium.org/2721383002/diff/1/ios/chrome/browser/ui/omnib...
File ios/chrome/browser/ui/omnibox/omnibox_popup_material_view_controller.mm
(right):

https://codereview.chromium.org/2721383002/diff/1/ios/chrome/browser/ui/omnib...
ios/chrome/browser/ui/omnibox/omnibox_popup_material_view_controller.mm:204:
const CGFloat kAnswerLabelHeight = 36;
This changes dramatically because the layout is different with UILabel instead
of OmniboxPopupTruncatingLabel. I'm not sure why, perhaps because
OmniboxPopupTruncatingLabel isn't handling DPI conversion correctly? Regardless,
this change actually simplifies the overall layout logic a bit as you can see
below. Visually, the result with the new code is the same (tested on several
different simulator devices).

https://codereview.chromium.org/2721383002/diff/1/ios/chrome/browser/ui/omnib...
ios/chrome/browser/ui/omnibox/omnibox_popup_material_view_controller.mm:260:
answerPresent ? row.detailAnswerLabel : row.detailTruncatingLabel;
Having two different labels that we swap between seems slightly funky, to be
honest. I have some thoughts about how to refactor OmniboxPopupMaterialRow I'd
like to discuss* but I'm hoping you're ok with this for now. Any refactorings
won't be possible before tomorrow's branch point and without this CL we'll have
to run the experiment on showing the new types of answers without iOS and have
it be a milestone behind.

* Primarily around the fact that it seems a little odd to have the row expose
these labels and ask the controller to lay them out, and also I'm interested in
a more dynamic layout if possible.

[commit-bot: I haz the power, 2017-03-01 17:44:01]

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

[commit-bot: I haz the power, 2017-03-01 17:44:02]

Dry run: This issue passed the CQ dry run.

[Justin Donnelly, 2017-03-01 19:14:19]

jdonnelly@chromium.org changed reviewers:
+ rohitrao@chromium.org

[Justin Donnelly, 2017-03-01 19:15:05]

+rohitrao

Hi, just noticed that justincohen is OOO. Can you review this instead or suggest
a reviewer?

Thanks!

[justincohen, 2017-03-01 20:14:18]

https://codereview.chromium.org/2721383002/diff/1/ios/chrome/browser/ui/omnib...
File ios/chrome/browser/ui/omnibox/omnibox_popup_material_row.mm (right):

https://codereview.chromium.org/2721383002/diff/1/ios/chrome/browser/ui/omnib...
ios/chrome/browser/ui/omnibox/omnibox_popup_material_row.mm:176: return
_detailTruncatingLabel.hidden
This seems weird to me... if detailTruncLabel is hidden using detailAnswerLabel?

https://codereview.chromium.org/2721383002/diff/1/ios/chrome/browser/ui/omnib...
File ios/chrome/browser/ui/omnibox/omnibox_popup_material_view_controller.mm
(right):

https://codereview.chromium.org/2721383002/diff/1/ios/chrome/browser/ui/omnib...
ios/chrome/browser/ui/omnibox/omnibox_popup_material_view_controller.mm:204:
const CGFloat kAnswerLabelHeight = 36;
On 2017/03/01 17:42:20, Justin Donnelly wrote:
> This changes dramatically because the layout is different with UILabel instead
> of OmniboxPopupTruncatingLabel. I'm not sure why, perhaps because
> OmniboxPopupTruncatingLabel isn't handling DPI conversion correctly?
Regardless,
> this change actually simplifies the overall layout logic a bit as you can see
> below. Visually, the result with the new code is the same (tested on several
> different simulator devices).

i wonder why it's different

https://codereview.chromium.org/2721383002/diff/1/ios/chrome/browser/ui/omnib...
ios/chrome/browser/ui/omnibox/omnibox_popup_material_view_controller.mm:260:
answerPresent ? row.detailAnswerLabel : row.detailTruncatingLabel;
On 2017/03/01 17:42:20, Justin Donnelly wrote:
> Having two different labels that we swap between seems slightly funky, to be
> honest. I have some thoughts about how to refactor OmniboxPopupMaterialRow I'd
> like to discuss* but I'm hoping you're ok with this for now. Any refactorings
> won't be possible before tomorrow's branch point and without this CL we'll
have
> to run the experiment on showing the new types of answers without iOS and have
> it be a milestone behind.
> 
> * Primarily around the fact that it seems a little odd to have the row expose
> these labels and ask the controller to lay them out, and also I'm interested
in
> a more dynamic layout if possible.

How about a bug tracking this and a reference to it here

[Justin Donnelly, 2017-03-01 23:20:55]

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

[Justin Donnelly, 2017-03-01 23:21:16]

https://codereview.chromium.org/2721383002/diff/1/ios/chrome/browser/ui/omnib...
File ios/chrome/browser/ui/omnibox/omnibox_popup_material_row.mm (right):

https://codereview.chromium.org/2721383002/diff/1/ios/chrome/browser/ui/omnib...
ios/chrome/browser/ui/omnibox/omnibox_popup_material_row.mm:176: return
_detailTruncatingLabel.hidden
On 2017/03/01 20:14:18, justincohen (OOO until Mar 20) wrote:
> This seems weird to me... if detailTruncLabel is hidden using
detailAnswerLabel?

I don't disagree. :) In the controller, either one or the other is hidden. This
logic thus returns the text of the one that's not hidden. Would it look more
natural if it was the other way around? (in other words, if !X.hidden return
X.text?

Or is it that I'm using the visibility to determine this? I could instead have
the controller send a more explicit signal about which label it's using but it
would, at the moment, only be used in this one spot. (In terms of the
refactoring I was thinking of, the controller would pass in the data and then
the logic about which label to use and how to style it would be in this class,
making the decision about what text to return here a lot clearer.)

https://codereview.chromium.org/2721383002/diff/1/ios/chrome/browser/ui/omnib...
File ios/chrome/browser/ui/omnibox/omnibox_popup_material_view_controller.mm
(right):

https://codereview.chromium.org/2721383002/diff/1/ios/chrome/browser/ui/omnib...
ios/chrome/browser/ui/omnibox/omnibox_popup_material_view_controller.mm:204:
const CGFloat kAnswerLabelHeight = 36;
On 2017/03/01 20:14:18, justincohen (OOO until Mar 20) wrote:
> On 2017/03/01 17:42:20, Justin Donnelly wrote:
> > This changes dramatically because the layout is different with UILabel
instead
> > of OmniboxPopupTruncatingLabel. I'm not sure why, perhaps because
> > OmniboxPopupTruncatingLabel isn't handling DPI conversion correctly?
> Regardless,
> > this change actually simplifies the overall layout logic a bit as you can
see
> > below. Visually, the result with the new code is the same (tested on several
> > different simulator devices).
> 
> i wonder why it's different

I can tell you why, sort of, in that it's caused by OmniboxPopupTruncatingLabel
overriding UILabel's draw behavior with [attributedString
drawInRect:requestedRect]. That drawInRect is behaving differently than whatever
UILabel is doing to draw the text, for reasons I can only guess at. But given
that I'm now using the platform's UILabel in the answer case this new layout
must be "right" by definition.

https://codereview.chromium.org/2721383002/diff/1/ios/chrome/browser/ui/omnib...
ios/chrome/browser/ui/omnibox/omnibox_popup_material_view_controller.mm:260:
answerPresent ? row.detailAnswerLabel : row.detailTruncatingLabel;
On 2017/03/01 20:14:18, justincohen (OOO until Mar 20) wrote:
> On 2017/03/01 17:42:20, Justin Donnelly wrote:
> > Having two different labels that we swap between seems slightly funky, to be
> > honest. I have some thoughts about how to refactor OmniboxPopupMaterialRow
I'd
> > like to discuss* but I'm hoping you're ok with this for now. Any
refactorings
> > won't be possible before tomorrow's branch point and without this CL we'll
> have
> > to run the experiment on showing the new types of answers without iOS and
have
> > it be a milestone behind.
> > 
> > * Primarily around the fact that it seems a little odd to have the row
expose
> > these labels and ask the controller to lay them out, and also I'm interested
> in
> > a more dynamic layout if possible.
> 
> How about a bug tracking this and a reference to it here

Done.

[commit-bot: I haz the power, 2017-03-01 23:21:25]

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, 2017-03-01 23:59:57]

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

[commit-bot: I haz the power, 2017-03-01 23:59:58]

Dry run: This issue passed the CQ dry run.

[justincohen, 2017-03-02 00:02:17]

https://codereview.chromium.org/2721383002/diff/1/ios/chrome/browser/ui/omnib...
File ios/chrome/browser/ui/omnibox/omnibox_popup_material_row.mm (right):

https://codereview.chromium.org/2721383002/diff/1/ios/chrome/browser/ui/omnib...
ios/chrome/browser/ui/omnibox/omnibox_popup_material_row.mm:176: return
_detailTruncatingLabel.hidden
On 2017/03/01 23:21:16, Justin Donnelly wrote:
> On 2017/03/01 20:14:18, justincohen (OOO until Mar 20) wrote:
> > This seems weird to me... if detailTruncLabel is hidden using
> detailAnswerLabel?
> 
> I don't disagree. :) In the controller, either one or the other is hidden.
This
> logic thus returns the text of the one that's not hidden. Would it look more
> natural if it was the other way around? (in other words, if !X.hidden return
> X.text?
> 
> Or is it that I'm using the visibility to determine this? I could instead have
> the controller send a more explicit signal about which label it's using but it
> would, at the moment, only be used in this one spot. (In terms of the
> refactoring I was thinking of, the controller would pass in the data and then
> the logic about which label to use and how to style it would be in this class,
> making the decision about what text to return here a lot clearer.)

Would crbug.com/697647 also involve clean this up?

[Justin Donnelly, 2017-03-02 01:05:17]

On 2017/03/02 00:02:17, justincohen (OOO until Mar 20) wrote:
>
https://codereview.chromium.org/2721383002/diff/1/ios/chrome/browser/ui/omnib...
> File ios/chrome/browser/ui/omnibox/omnibox_popup_material_row.mm (right):
> 
>
https://codereview.chromium.org/2721383002/diff/1/ios/chrome/browser/ui/omnib...
> ios/chrome/browser/ui/omnibox/omnibox_popup_material_row.mm:176: return
> _detailTruncatingLabel.hidden
> On 2017/03/01 23:21:16, Justin Donnelly wrote:
> > On 2017/03/01 20:14:18, justincohen (OOO until Mar 20) wrote:
> > > This seems weird to me... if detailTruncLabel is hidden using
> > detailAnswerLabel?
> > 
> > I don't disagree. :) In the controller, either one or the other is hidden.
> This
> > logic thus returns the text of the one that's not hidden. Would it look more
> > natural if it was the other way around? (in other words, if !X.hidden return
> > X.text?
> > 
> > Or is it that I'm using the visibility to determine this? I could instead
have
> > the controller send a more explicit signal about which label it's using but
it
> > would, at the moment, only be used in this one spot. (In terms of the
> > refactoring I was thinking of, the controller would pass in the data and
then
> > the logic about which label to use and how to style it would be in this
class,
> > making the decision about what text to return here a lot clearer.)
> 
> Would crbug.com/697647 also involve clean this up?

Yes. Added a comment on the bug to that effect.

[justincohen, 2017-03-02 03:03:11]

lgtm

[rohitrao (ping after 24h), 2017-03-02 18:18:32]

lgtm

https://codereview.chromium.org/2721383002/diff/20001/ios/chrome/browser/ui/o...
File ios/chrome/browser/ui/omnibox/omnibox_popup_material_row.mm (right):

https://codereview.chromium.org/2721383002/diff/20001/ios/chrome/browser/ui/o...
ios/chrome/browser/ui/omnibox/omnibox_popup_material_row.mm:72:
_detailAnswerLabel = [[UILabel alloc] initWithFrame:CGRectZero];
How does the frame for this label (and all the other labels) get set?

[Justin Donnelly, 2017-03-02 18:52:05]

https://codereview.chromium.org/2721383002/diff/20001/ios/chrome/browser/ui/o...
File ios/chrome/browser/ui/omnibox/omnibox_popup_material_row.mm (right):

https://codereview.chromium.org/2721383002/diff/20001/ios/chrome/browser/ui/o...
ios/chrome/browser/ui/omnibox/omnibox_popup_material_row.mm:72:
_detailAnswerLabel = [[UILabel alloc] initWithFrame:CGRectZero];
On 2017/03/02 18:18:31, rohitrao wrote:
> How does the frame for this label (and all the other labels) get set?

The layout of these labels is done in OmniboxPopupMaterialViewController. See
http://crbug.com/697647 for my proposal to change this.

[Justin Donnelly, 2017-03-02 18:52:11]

The CQ bit was checked by jdonnelly@chromium.org

[commit-bot: I haz the power, 2017-03-02 18:52:35]

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, 2017-03-02 18:59:14]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488480731879990,
"parent_rev": "f62ccb5cb4426418e86516a96f2dfdff41b6e789", "commit_rev":
"eb8df71ea60fb11ce27598039c85b9a7256f963f"}

[commit-bot: I haz the power, 2017-03-02 18:59:57]

Description was changed from

==========
Use a plain UILabel for answers in suggest detail text.

This is to support truncation with ellipses instead of the truncation by
fading used for regular suggestion detail text. The use of ellipses is
preferred for answers because they sometimes contain multi-line text and
the use of an ellipse looks more natural.

BUG=591803
==========

to

==========
Use a plain UILabel for answers in suggest detail text.

This is to support truncation with ellipses instead of the truncation by
fading used for regular suggestion detail text. The use of ellipses is
preferred for answers because they sometimes contain multi-line text and
the use of an ellipse looks more natural.

BUG=591803

Review-Url: https://codereview.chromium.org/2721383002
Cr-Commit-Position: refs/heads/master@{#454325}
Committed:
https://chromium.googlesource.com/chromium/src/+/eb8df71ea60fb11ce27598039c85...
==========

[commit-bot: I haz the power, 2017-03-02 18:59:58]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/eb8df71ea60fb11ce27598039c85...