Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2808773002:
Fixed ellipsize of publisher instead of timespan string. (Closed)
DescriptionFixed ellipsize of publisher instead of timespan string.
Screenshots here:
https://drive.google.com/drive/folders/0B7eRCSITD4qmYl9MbDRWWFFBZGs?usp=sharing
BUG=625775
Review-Url: https://codereview.chromium.org/2808773002
Cr-Commit-Position: refs/heads/master@{#465564}
Committed: https://chromium.googlesource.com/chromium/src/+/1611b2644697ca7bc868e9a68bf28dd15619d250
Patch Set 1 #
Total comments: 1
Patch Set 2 : Refactor variable name. #
Total comments: 19
Patch Set 3 : Refactor. #
Total comments: 1
Patch Set 4 : LTR in Jelly Bean works. RTL wrong order, but correct formatting. #
Messages
Total messages: 29 (18 generated)
Description was changed from ========== Format. Fixed ellipsize of publisher instead of timespan string. Screenshots here: https://drive.google.com/drive/folders/0B7eRCSITD4qmYl9MbDRWWFFBZGs?usp=sharing BUG=625775 ========== to ========== Fixed ellipsize of publisher instead of timespan string. Screenshots here: https://drive.google.com/drive/folders/0B7eRCSITD4qmYl9MbDRWWFFBZGs?usp=sharing BUG=625775 ==========
galinap@google.com changed reviewers: + dgn@chromium.org, mvanouwerkerk@chromium.org
Hi, Fixed the ellipsizing of the publisher string. Could you have a look please.
The CQ bit was checked by galinap@google.com 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.
lgtm with nit https://codereview.chromium.org/2808773002/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/2808773002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:201: mPublisherTextView.setText(getPublisherString(article)); nit: refer to mArticle to maintain consistency with the rest of this method, here and below.
Thanks! that looks really good! Added comments on the code. Additionally, RTL support has been properly added in KitKat, if I remember correctly. Can you please try your patch and add screenshots on a Jelly Bean phone? https://codereview.chromium.org/2808773002/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/new_tab_page_snippets_card.xml (right): https://codereview.chromium.org/2808773002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/new_tab_page_snippets_card.xml:58: to ellipsize before pushing the ImageView off the screen. See: https://crbug.com/678568 --> nit: update the description. The thing it would be pushing off the screen here is the suggestion age. https://codereview.chromium.org/2808773002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/new_tab_page_snippets_card.xml:72: <!-- We need to have the publisher and the time span in separete TextViews to be able to s/separete/separate https://codereview.chromium.org/2808773002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/new_tab_page_snippets_card.xml:73: ellipsize the publisher instead of the time span.--> nit: + 4 spaces to align second line of comment with first letter of the actual comment above. For comment style, the comment below is wrong, the comment in the copyright notice is right. https://codereview.chromium.org/2808773002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/new_tab_page_snippets_card.xml:75: android:id="@+id/article_time" nit: I think something like article_age is more representative of the value of this field. If you update it, also update the variables in the view holder. https://codereview.chromium.org/2808773002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/new_tab_page_snippets_card.xml:78: android:drawablePadding="8dp" do you need the drawable padding here? when is it used? https://codereview.chromium.org/2808773002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2808773002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:281: if (article.mPublishTimestampMilliseconds == 0) return ""; physical web URLs don't have a publish timestamp. As implemented here that would result in something like "www.coolsite.com - " for the attribution line. The current code would only use the publisher name and not append the unecessary dash and spaces. https://codereview.chromium.org/2808773002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:300: // We add a dash before the elapsed time. E.g. " - 14 minutes ago" nit: add "." at the end of sentences. https://codereview.chromium.org/2808773002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:301: return String.format(TIME_SPAN_FORMAT_STRING, relativeTimeSpan); doesn't that raise a warning about unspecified locale?
bauerb@chromium.org changed reviewers: + bauerb@chromium.org
https://codereview.chromium.org/2808773002/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/new_tab_page_snippets_card.xml (right): https://codereview.chromium.org/2808773002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/new_tab_page_snippets_card.xml:73: ellipsize the publisher instead of the time span.--> On 2017/04/10 21:42:00, dgn (in PST timezone) wrote: > nit: + 4 spaces to align second line of comment with first letter of the actual > comment above. For comment style, the comment below is wrong, the comment in the > copyright notice is right. Also, add a space before the end of the comment. https://codereview.chromium.org/2808773002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2808773002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:300: // We add a dash before the elapsed time. E.g. " - 14 minutes ago" On 2017/04/10 21:42:00, dgn (in PST timezone) wrote: > nit: add "." at the end of sentences. Super-nit: also add a comma before the e.g. so we don't end up with a sentence without a predicate.
https://codereview.chromium.org/2808773002/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/new_tab_page_snippets_card.xml (right): https://codereview.chromium.org/2808773002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/new_tab_page_snippets_card.xml:58: to ellipsize before pushing the ImageView off the screen. See: https://crbug.com/678568 --> On 2017/04/10 21:42:00, dgn (in PST timezone) wrote: > nit: update the description. The thing it would be pushing off the screen here > is the suggestion age. Done. https://codereview.chromium.org/2808773002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/new_tab_page_snippets_card.xml:72: <!-- We need to have the publisher and the time span in separete TextViews to be able to On 2017/04/10 21:42:00, dgn (in PST timezone) wrote: > s/separete/separate I'll remove that comment and explain all of this in the one above the two TextViews. https://codereview.chromium.org/2808773002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/new_tab_page_snippets_card.xml:73: ellipsize the publisher instead of the time span.--> On 2017/04/10 21:42:00, dgn (in PST timezone) wrote: > nit: + 4 spaces to align second line of comment with first letter of the actual > comment above. For comment style, the comment below is wrong, the comment in the > copyright notice is right. Done. https://codereview.chromium.org/2808773002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/new_tab_page_snippets_card.xml:75: android:id="@+id/article_time" On 2017/04/10 21:42:00, dgn (in PST timezone) wrote: > nit: I think something like article_age is more representative of the value of > this field. If you update it, also update the variables in the view holder. Done. https://codereview.chromium.org/2808773002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/new_tab_page_snippets_card.xml:78: android:drawablePadding="8dp" On 2017/04/10 21:42:00, dgn (in PST timezone) wrote: > do you need the drawable padding here? when is it used? Removed. Thanks. https://codereview.chromium.org/2808773002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2808773002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:281: if (article.mPublishTimestampMilliseconds == 0) return ""; On 2017/04/10 21:42:00, dgn (in PST timezone) wrote: > physical web URLs don't have a publish timestamp. As implemented here that would > result in something like "www.coolsite.com - " for the attribution line. The > current code would only use the publisher name and not append the unecessary > dash and spaces. This function returns a string like " - 14 minutes ago" if there is a timestamp to the URL. If there isn't one, then an empty string will be returned and the publisher ("www.coolsite.com") will be the only text on the screen. I didn't see any problems with this when I was testing the cod eon devices. https://codereview.chromium.org/2808773002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:300: // We add a dash before the elapsed time. E.g. " - 14 minutes ago" On 2017/04/10 21:42:00, dgn (in PST timezone) wrote: > nit: add "." at the end of sentences. Done. https://codereview.chromium.org/2808773002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:301: return String.format(TIME_SPAN_FORMAT_STRING, relativeTimeSpan); On 2017/04/10 21:42:00, dgn (in PST timezone) wrote: > doesn't that raise a warning about unspecified locale? No. I was looking at this before and the dash goes on the correct side when I change LTR and RTL languages. And the Hebrew string for "14 minutes ago" looks correct too when I compare the shape of the letters to the google translate result :D. I think the comment which was here before referred to the BidiFormatter.getInstance().unicodeWrap() function, which is used for the formatting the publisher string. And I am using this function in getPublisherString().
https://codereview.chromium.org/2808773002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2808773002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:281: if (article.mPublishTimestampMilliseconds == 0) return ""; On 2017/04/11 15:54:32, Galia wrote: > On 2017/04/10 21:42:00, dgn (in PST timezone) wrote: > > physical web URLs don't have a publish timestamp. As implemented here that > would > > result in something like "www.coolsite.com - " for the attribution line. The > > current code would only use the publisher name and not append the unecessary > > dash and spaces. > > This function returns a string like " - 14 minutes ago" if there is a timestamp > to the URL. If there isn't one, then an empty string will be returned and the > publisher ("www.coolsite.com") will be the only text on the screen. I didn't see > any problems with this when I was testing the cod eon devices. You're right, I misread. It also looks like there is no extra padding between the strings so you don't need to set the visibility of the view to GONE. All good then!
https://codereview.chromium.org/2808773002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2808773002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:301: return String.format(ARTICLE_AGE_FORMAT_STRING, relativeTimeSpan); Does using the BidiFormatter here fix the trailing dash issue on JB?
The CQ bit was checked by galinap@google.com
The CQ bit was unchecked by galinap@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mvanouwerkerk@chromium.org Link to the patchset: https://codereview.chromium.org/2808773002/#ps60001 (title: "LTR in Jelly Bean works. RTL wrong order, but correct formatting.")
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 galinap@google.com
The CQ bit was checked by galinap@google.com 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.
lgtm. Can you please add jelly bean screenshots to the drive folder?
The CQ bit was checked by galinap@google.com
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": 60001, "attempt_start_ts": 1492603604967090,
"parent_rev": "92f8c0b3ff53422eb761b53793bac0b424ec45df", "commit_rev":
"1611b2644697ca7bc868e9a68bf28dd15619d250"}
Message was sent while issue was closed.
Description was changed from ========== Fixed ellipsize of publisher instead of timespan string. Screenshots here: https://drive.google.com/drive/folders/0B7eRCSITD4qmYl9MbDRWWFFBZGs?usp=sharing BUG=625775 ========== to ========== Fixed ellipsize of publisher instead of timespan string. Screenshots here: https://drive.google.com/drive/folders/0B7eRCSITD4qmYl9MbDRWWFFBZGs?usp=sharing BUG=625775 Review-Url: https://codereview.chromium.org/2808773002 Cr-Commit-Position: refs/heads/master@{#465564} Committed: https://chromium.googlesource.com/chromium/src/+/1611b2644697ca7bc868e9a68bf2... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/1611b2644697ca7bc868e9a68bf2... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
