|
|
Created:
3 years, 10 months ago by Justin Donnelly Modified:
3 years, 10 months ago Reviewers:
Justin Donnelly1, rohitrao (ping after 24h), Justin Cohen (wrong one), justincohen, pschaffner_corp CC:
chromium-reviews, marq+watch_chromium.org, jdonnelly+watch_chromium.org, pkl (ping after 24h if needed), noyau+watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange OmniboxPopupTruncatingLabel to be based on UILabel.
When OmniboxPopupTruncatingLabel was created in 2012, UILabel did not
support attributed strings. My assumption is that this is why it's based
on CATextLayer. As of iOS 6, however, UILabel has the attributedString
property which allows us to set the font characteristics we need.
Consequently, OmniboxPopupMaterialViewController no longer needs to
create and cache CTFontRefs.
Tested on the following simulators: iPhone 6s/iOS 9.3, iPhone 7/iOS
10.2, and iPad Air/iOS 10.2.
BUG=
Review-Url: https://codereview.chromium.org/2695413003
Cr-Commit-Position: refs/heads/master@{#452638}
Committed: https://chromium.googlesource.com/chromium/src/+/af40f7a6563089a73b4a18e1d9abac7cbff3b88f
Patch Set 1 #
Total comments: 10
Patch Set 2 : Remove handling of the unused self.text case. #Patch Set 3 : Cache gradient, restore handling of text alignment. #
Messages
Total messages: 38 (23 generated)
Description was changed from ========== Change OmniboxPopupTruncatingLabel to be based on UILabel. BUG= ========== to ========== Change OmniboxPopupTruncatingLabel to be based on UILabel. When OmniboxPopupTruncatingLabel was created in 2012, UILabel did not support attributed strings. My assumption is that this is why it's based on CATextLayer. As of iOS 6, however, UILabel has the attributedString property which allows us to set the font characteristics we need. This change makes OmniboxPopupTruncatingLabel a much closer copy of GTMFadeTruncatingLabel. The only difference now is a couple conditional cases to support UILabels with attributedString set. BUG= ==========
Description was changed from ========== Change OmniboxPopupTruncatingLabel to be based on UILabel. When OmniboxPopupTruncatingLabel was created in 2012, UILabel did not support attributed strings. My assumption is that this is why it's based on CATextLayer. As of iOS 6, however, UILabel has the attributedString property which allows us to set the font characteristics we need. This change makes OmniboxPopupTruncatingLabel a much closer copy of GTMFadeTruncatingLabel. The only difference now is a couple conditional cases to support UILabels with attributedString set. BUG= ========== to ========== Change OmniboxPopupTruncatingLabel to be based on UILabel. When OmniboxPopupTruncatingLabel was created in 2012, UILabel did not support attributed strings. My assumption is that this is why it's based on CATextLayer. As of iOS 6, however, UILabel has the attributedString property which allows us to set the font characteristics we need. Tested on the following simulators: iPhone 6s/iOS 9.3, iPhone 7/iOS 10.2, and iPad Air/iOS 10.2. BUG= ==========
Description was changed from ========== Change OmniboxPopupTruncatingLabel to be based on UILabel. When OmniboxPopupTruncatingLabel was created in 2012, UILabel did not support attributed strings. My assumption is that this is why it's based on CATextLayer. As of iOS 6, however, UILabel has the attributedString property which allows us to set the font characteristics we need. Tested on the following simulators: iPhone 6s/iOS 9.3, iPhone 7/iOS 10.2, and iPad Air/iOS 10.2. BUG= ========== to ========== Change OmniboxPopupTruncatingLabel to be based on UILabel. When OmniboxPopupTruncatingLabel was created in 2012, UILabel did not support attributed strings. My assumption is that this is why it's based on CATextLayer. As of iOS 6, however, UILabel has the attributedString property which allows us to set the font characteristics we need. Tested on the following simulators: iPhone 6s/iOS 9.3, iPhone 7/iOS 10.2, and iPad Air/iOS 10.2. BUG= ==========
Description was changed from ========== Change OmniboxPopupTruncatingLabel to be based on UILabel. When OmniboxPopupTruncatingLabel was created in 2012, UILabel did not support attributed strings. My assumption is that this is why it's based on CATextLayer. As of iOS 6, however, UILabel has the attributedString property which allows us to set the font characteristics we need. Tested on the following simulators: iPhone 6s/iOS 9.3, iPhone 7/iOS 10.2, and iPad Air/iOS 10.2. BUG= ========== to ========== Change OmniboxPopupTruncatingLabel to be based on UILabel. When OmniboxPopupTruncatingLabel was created in 2012, UILabel did not support attributed strings. My assumption is that this is why it's based on CATextLayer. As of iOS 6, however, UILabel has the attributedString property which allows us to set the font characteristics we need. Consequently, OmniboxPopupMaterialViewController no longer needs to create and cache CTFontRefs. Tested on the following simulators: iPhone 6s/iOS 9.3, iPhone 7/iOS 10.2, and iPad Air/iOS 10.2. BUG= ==========
jdonnelly@chromium.org changed reviewers: + justincohen@google.com, rohitrao@chromium.org
This change makes OmniboxPopupTruncatingLabel a much closer copy of GTMFadeTruncatingLabel. The only difference now is a couple conditional cases to support UILabels with attributedString values. It's so close, in fact that I'm currently trying to set up a Mac google3 environment to add this change to GTMFadeTruncatingLabel. In the meantime, can you take a look and let me know if this approach makes sense and if you'd be open to landing this until I get the change to GTMFadeTruncatingLabel done. Aside from general simplification, the motivation for this is to switch answers in suggest (weather, facts, stock quotes) to use plain UILabels*. If OmniboxPopupTruncatingLabel is a descendant of UILabel then I can change the type of the labels in OmniboxPopupMaterialRow to UILabel and just instantiate the correct type based on whether it's an answer or a regular suggestion. * The motivation for *this* is that answers generally don't need truncation except in the case of dictionary definitions, which are usually multi-line and per direction from UX, we'd prefer truncation with ellipses in this case.
The CQ bit was checked by jdonnelly@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ah, hang on. GTM is managed on github. I'm a bit less confident about making a change there and getting it pulled over in a timely manner so please do let me know what you think of making this change here. Regardless, in the meantime I've filed https://github.com/google/google-toolbox-for-mac/issues/133 and will follow-up on the GTM mailing list.
justincohen@chromium.org changed reviewers: + justincohen@chromium.org, pschaffner@google.com
I didn't see a resolution on the email 'Ellipses in iOS' thread with pschaffner@. Does this depend on that?
https://codereview.chromium.org/2695413003/diff/1/ios/chrome/browser/ui/omnib... File ios/chrome/browser/ui/omnibox/truncating_attributed_label.mm (right): https://codereview.chromium.org/2695413003/diff/1/ios/chrome/browser/ui/omnib... ios/chrome/browser/ui/omnibox/truncating_attributed_label.mm:60: UIImage* image = [self getLinearGradient:requestedRect]; Does it matter this isn't cached like it used to be? https://codereview.chromium.org/2695413003/diff/1/ios/chrome/browser/ui/omnib... ios/chrome/browser/ui/omnibox/truncating_attributed_label.mm:76: [attributedString drawInRect:requestedRect]; Is lineBreakMode and textAlignment different in self.attributedText? Why is this copy necessary? https://codereview.chromium.org/2695413003/diff/1/ios/chrome/browser/ui/omnib... ios/chrome/browser/ui/omnibox/truncating_attributed_label.mm:97: [self.text drawInRect:requestedRect withAttributes:attributes]; Why not just create an attributed string from text and hit the same path as above?
On 2017/02/20 19:56:28, justincohen wrote: > I didn't see a resolution on the email 'Ellipses in iOS' thread with > pschaffner@. Does this depend on that? There was a lot of subsequent discussion of what the long-term approach should be but his initial response is still clear, I think: "For eliding characters from a multiline label, like what you’ve described, you should use an ellipsis. You are right, it does create inconsistency with our single-line pattern, but that is OK (for now)."
The CQ bit was checked by jdonnelly@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2695413003/diff/1/ios/chrome/browser/ui/omnib... File ios/chrome/browser/ui/omnibox/truncating_attributed_label.mm (right): https://codereview.chromium.org/2695413003/diff/1/ios/chrome/browser/ui/omnib... ios/chrome/browser/ui/omnibox/truncating_attributed_label.mm:60: UIImage* image = [self getLinearGradient:requestedRect]; On 2017/02/20 20:07:24, justincohen wrote: > Does it matter this isn't cached like it used to be? I don't think so. GTMFadeTruncatingLabel doesn't cache it nor does the omnibox text field when it handles the clipping itself: https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/omnibox/omnibox_te... https://codereview.chromium.org/2695413003/diff/1/ios/chrome/browser/ui/omnib... ios/chrome/browser/ui/omnibox/truncating_attributed_label.mm:76: [attributedString drawInRect:requestedRect]; On 2017/02/20 20:07:24, justincohen wrote: > Is lineBreakMode and textAlignment different in self.attributedText? No, they behave the same. Maybe it would be better to just hardcode the line break mode here to NSLineBreakByClipping (since the other ones probably don't make sense with fading) and to have the client specify the alignment as part of their attributed string. I was trying to stay consistent with the .text case and how GTMFadeTruncatingLabel currently works, but I could go either way. Let me know what you think. > Why is this copy necessary? The copy is necessary because self.attributedText is immutable. https://codereview.chromium.org/2695413003/diff/1/ios/chrome/browser/ui/omnib... ios/chrome/browser/ui/omnibox/truncating_attributed_label.mm:97: [self.text drawInRect:requestedRect withAttributes:attributes]; On 2017/02/20 20:07:24, justincohen wrote: > Why not just create an attributed string from text and hit the same path as > above? It seemed simpler and safer to me to just copy the code from GTMFadeTruncatingLabel and match the existing behavior for self.text. But on second thought, this case is dead code. The only use of this class is in OmniboxPopupMaterialRow and OmniboxPopupMaterialViewController which use attributedText. I've removed it. Now this class can just be the attributedText version of GTMFadeTruncatingLabel until such time as I can get GTMFadeTruncatingLabel to handle both cases.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2695413003/diff/1/ios/chrome/browser/ui/omnib... File ios/chrome/browser/ui/omnibox/truncating_attributed_label.mm (right): https://codereview.chromium.org/2695413003/diff/1/ios/chrome/browser/ui/omnib... ios/chrome/browser/ui/omnibox/truncating_attributed_label.mm:60: UIImage* image = [self getLinearGradient:requestedRect]; On 2017/02/21 18:16:50, Justin Donnelly wrote: > On 2017/02/20 20:07:24, justincohen wrote: > > Does it matter this isn't cached like it used to be? > > I don't think so. GTMFadeTruncatingLabel doesn't cache it nor does the omnibox > text field when it handles the clipping itself: > > https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/omnibox/omnibox_te... I had to go back to 2012 ( https://b.corp.google.com/issues/5954680 ) to find where we added this caching. Do you think (four years later!) maybe getLinearGradient is faster, or drawTextInRect is called less? https://codereview.chromium.org/2695413003/diff/1/ios/chrome/browser/ui/omnib... ios/chrome/browser/ui/omnibox/truncating_attributed_label.mm:76: [attributedString drawInRect:requestedRect]; On 2017/02/21 18:16:50, Justin Donnelly wrote: > On 2017/02/20 20:07:24, justincohen wrote: > > Is lineBreakMode and textAlignment different in self.attributedText? > > No, they behave the same. Maybe it would be better to just hardcode the line > break mode here to NSLineBreakByClipping (since the other ones probably don't > make sense with fading) and to have the client specify the alignment as part of > their attributed string. I was trying to stay consistent with the .text case and > how GTMFadeTruncatingLabel currently works, but I could go either way. Let me > know what you think. > > > Why is this copy necessary? > > The copy is necessary because self.attributedText is immutable. What's the default linebreakmode? If the client specifies everything in their attributed string is any of this necessary? I don't feel strongly either way -- this is fine too.
The CQ bit was checked by jdonnelly@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2695413003/diff/1/ios/chrome/browser/ui/omnib... File ios/chrome/browser/ui/omnibox/truncating_attributed_label.mm (right): https://codereview.chromium.org/2695413003/diff/1/ios/chrome/browser/ui/omnib... ios/chrome/browser/ui/omnibox/truncating_attributed_label.mm:60: UIImage* image = [self getLinearGradient:requestedRect]; On 2017/02/22 03:52:29, justincohen wrote: > On 2017/02/21 18:16:50, Justin Donnelly wrote: > > On 2017/02/20 20:07:24, justincohen wrote: > > > Does it matter this isn't cached like it used to be? > > > > I don't think so. GTMFadeTruncatingLabel doesn't cache it nor does the omnibox > > text field when it handles the clipping itself: > > > > > https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/omnibox/omnibox_te... > > I had to go back to 2012 ( https://b.corp.google.com/issues/5954680 ) to find > where we added this caching. Do you think (four years later!) maybe > getLinearGradient is faster, or drawTextInRect is called less? I don't honestly know. Without the caching, there's no jank in the simulator but I don't have an old device to test it on to see if there's a performance regression. I've restored the caching in the latest patch set, lmkwyt. (The latest patch set also restores the handling of text alignment which I mistakenly removed. And I actually tested this in an RTL language this time to verify it's working correctly.) https://codereview.chromium.org/2695413003/diff/1/ios/chrome/browser/ui/omnib... ios/chrome/browser/ui/omnibox/truncating_attributed_label.mm:76: [attributedString drawInRect:requestedRect]; On 2017/02/22 03:52:29, justincohen wrote: > On 2017/02/21 18:16:50, Justin Donnelly wrote: > > On 2017/02/20 20:07:24, justincohen wrote: > > > Is lineBreakMode and textAlignment different in self.attributedText? > > > > No, they behave the same. Maybe it would be better to just hardcode the line > > break mode here to NSLineBreakByClipping (since the other ones probably don't > > make sense with fading) and to have the client specify the alignment as part > of > > their attributed string. I was trying to stay consistent with the .text case > and > > how GTMFadeTruncatingLabel currently works, but I could go either way. Let me > > know what you think. > > > > > Why is this copy necessary? > > > > The copy is necessary because self.attributedText is immutable. > > What's the default linebreakmode? If the client specifies everything in their > attributed string is any of this necessary? > > I don't feel strongly either way -- this is fine too. The default is NSLineBreakByTruncatingTail (trailing ellipsis). Yes, we could have the client specify these properties instead. But if you don't feel strongly, I'd prefer to keep it this way to make it easier to switch to GTMFadeTruncatingLabel in the future.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm!
The CQ bit was checked by jdonnelly@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
rohitrao: would you mind giving this an OWNERS review when you get a chance?
lgtm
The CQ bit was checked by jdonnelly@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487885084458600, "parent_rev": "d31fa8e715fc2baa87357e62a106f5fce6f2d4cc", "commit_rev": "af40f7a6563089a73b4a18e1d9abac7cbff3b88f"}
Message was sent while issue was closed.
Description was changed from ========== Change OmniboxPopupTruncatingLabel to be based on UILabel. When OmniboxPopupTruncatingLabel was created in 2012, UILabel did not support attributed strings. My assumption is that this is why it's based on CATextLayer. As of iOS 6, however, UILabel has the attributedString property which allows us to set the font characteristics we need. Consequently, OmniboxPopupMaterialViewController no longer needs to create and cache CTFontRefs. Tested on the following simulators: iPhone 6s/iOS 9.3, iPhone 7/iOS 10.2, and iPad Air/iOS 10.2. BUG= ========== to ========== Change OmniboxPopupTruncatingLabel to be based on UILabel. When OmniboxPopupTruncatingLabel was created in 2012, UILabel did not support attributed strings. My assumption is that this is why it's based on CATextLayer. As of iOS 6, however, UILabel has the attributedString property which allows us to set the font characteristics we need. Consequently, OmniboxPopupMaterialViewController no longer needs to create and cache CTFontRefs. Tested on the following simulators: iPhone 6s/iOS 9.3, iPhone 7/iOS 10.2, and iPad Air/iOS 10.2. BUG= Review-Url: https://codereview.chromium.org/2695413003 Cr-Commit-Position: refs/heads/master@{#452638} Committed: https://chromium.googlesource.com/chromium/src/+/af40f7a6563089a73b4a18e1d9ab... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/af40f7a6563089a73b4a18e1d9ab... |