[Glic] Add spark display on history page [chromium/src : main]
Hailey Wang (Gerrit)
Hailey Wang added 2 comments![]( "Open in Gerrit")
Related details
- Sky Malice
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sky Malice (Gerrit)
Sky Malice voted and added 4 comments![]( "Open in Gerrit")
Votes added by Sky Malice
| Code-Review | +1 |
4 comments
Can you just use oval_surface_0.xml?
android:layout_gravity="start|top"Would you have more sane margins if this was bottom|right?
updateSparkVisibility();Why do you need to call this here?
boolean showSpark = item != null && item.isActorVisit() && !item.wasBlockedVisit();Why? What?
Related details
- Hailey Wang
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hailey Wang (Gerrit)
Hailey Wang added 1 comment![]( "Open in Gerrit")
android:layout_gravity="start|top"Would you have more sane margins if this was bottom|right?
bottom end would act weird on screen resizing, especially width wise, by using start|top, I'm hoping the spark is more consistently pinned to the favicon regardless of screen width.
Related details
- Sky Malice
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hailey Wang (Gerrit)
Hailey Wang added 1 comment![]( "Open in Gerrit")
boolean showSpark = item != null && item.isActorVisit() && !item.wasBlockedVisit();Why? What?
wasBlockedVisit displays a blocked visit drawable which is different from the normal icon, at least in code. So when the visit is blocked, hide the spark for safety.
Sky Malice (Gerrit)
Sky Malice added 1 comment![]( "Open in Gerrit")
boolean showSpark = item != null && item.isActorVisit() && !item.wasBlockedVisit();Hailey WangWhy? What?
wasBlockedVisit displays a blocked visit drawable which is different from the normal icon, at least in code. So when the visit is blocked, hide the spark for safety.
- Hailey Wang
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wenyu Fu (Gerrit)
Wenyu Fu added 5 comments![]( "Open in Gerrit")
Can you please
<FrameLayoutThis is odd... HistoryItemView is extending a LinearLayout. If you declare a new member as its child, would it get added before / after other children views?
(I see you are calling #bringToFront in Java code, so follow up question there)
mSparkContainer = findViewById(R.id.spark_container);Can you please add a test case in chrome/android/junit/src/org/chromium/chrome/browser/history/HistoryUiTest.java
mSparkContainer.bringToFront();Does this makes the item to be the first child view of parent, or just change the Z-axis?
updateSparkVisibility();Why do you need to call this here?
+1 I was thinking following `updateChipView` pattern is sufficient
Related details
- Hailey Wang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hailey Wang (Gerrit)
Hailey Wang voted and added 10 comments![]( "Open in Gerrit")
Votes added by Hailey Wang
| Commit-Queue | +1 |
Can you please
I can
Can you just use oval_surface_0.xml?
oh yeah totally LOL, I did not notice this, thanks!
This is odd... HistoryItemView is extending a LinearLayout. If you declare a new member as its child, would it get added before / after other children views?
(I see you are calling #bringToFront in Java code, so follow up question there)
According to Gemini; seems like the content of xml is added first, then the content from code is added next, which is why there's a call to bring to front later.
Hailey WangWould you have more sane margins if this was bottom|right?
bottom end would act weird on screen resizing, especially width wise, by using start|top, I'm hoping the spark is more consistently pinned to the favicon regardless of screen width.
Acknowledged
Can you please add a test case in chrome/android/junit/src/org/chromium/chrome/browser/history/HistoryUiTest.java
Gemini says yes to your request 😂
Does this makes the item to be the first child view of parent, or just change the Z-axis?
This moves the spark container to the end of the child list, to ensure it is drawn last (on top of everything else).
Wenyu FuWhy do you need to call this here?
+1 I was thinking following `updateChipView` pattern is sufficient
Because we want the spark to disappear when the checkmark appears when an item is selected, we need to keep this in updateView().
boolean showSpark = item != null && item.isActorVisit() && !item.wasBlockedVisit();Hailey WangWhy? What?
Sky MalicewasBlockedVisit displays a blocked visit drawable which is different from the normal icon, at least in code. So when the visit is blocked, hide the spark for safety.
Code comment explaining this?
- Sky Malice
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wenyu Fu (Gerrit)
Wenyu Fu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Wenyu Fu
| Code-Review | +1 |
1 comment
<FrameLayoutHailey WangThis is odd... HistoryItemView is extending a LinearLayout. If you declare a new member as its child, would it get added before / after other children views?
(I see you are calling #bringToFront in Java code, so follow up question there)
According to Gemini; seems like the content of xml is added first, then the content from code is added next, which is why there's a call to bring to front later.
TIL!
the content of xml is added first, then the content from code is added next
- Hailey Wang
- Sky Malice
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hailey Wang (Gerrit)
Hailey Wang added 1 comment![]( "Open in Gerrit")
<FrameLayoutHailey WangThis is odd... HistoryItemView is extending a LinearLayout. If you declare a new member as its child, would it get added before / after other children views?
(I see you are calling #bringToFront in Java code, so follow up question there)
Wenyu FuAccording to Gemini; seems like the content of xml is added first, then the content from code is added next, which is why there's a call to bring to front later.
TIL!
the content of xml is added first, then the content from code is added next
optional: add this as comment
- Sky Malice
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hailey Wang (Gerrit)
Hailey Wang voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Sky Malice
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
3 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/android/java/src/org/chromium/chrome/browser/history/HistoryItemView.java
Insertions: 3, Deletions: 1.
@@ -91,7 +91,9 @@
mChipView.getPrimaryTextView().setEllipsize(TextUtils.TruncateAt.END);
mSparkContainer = findViewById(R.id.spark_container);
- // Ensure the spark is drawn on top of the favicon and its background.
+ // Ensure the spark is drawn on top of the favicon and its background. This is needed
+ // because the content of xml is added first, then the content from code is added next (on
+ // top of the xml children).
mSparkContainer.bringToFront();
}
```
Change information
[Glic] Add spark display on history page
https://screenshot.googleplex.com/4xQcGDgy6sNSkds
- M chrome/android/java/res/layout/history_item_view.xml
- M chrome/android/java/res/values/dimens.xml
- M chrome/android/java/src/org/chromium/chrome/browser/history/HistoryItemView.java
- M chrome/android/junit/src/org/chromium/chrome/browser/history/HistoryUiTest.java
- M chrome/test/android/javatests/src/org/chromium/chrome/browser/history/StubbedHistoryProvider.java
Code-Review: +1 by Wenyu Fu
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |