Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1913433004:
Supporting shrinking/enlarging for notifications (Closed)
DescriptionSupporting shrinking/enlarging for notifications
Extensions have api to update text of a displayed
notification. Due to such update, notification recalculates
it's size. However, there was a bug in toast_contents_view,
that updating preferred_size_ didn't trigger update of
corresponding widget. Not updating widget lead to a visual
glitch.
BUG=605656
R=dewittj@chromium.com
Committed: https://crrev.com/2f78eb16262f1442e4c2b0cc9a03689e5cc47353
Cr-Commit-Position: refs/heads/master@{#392906}
Patch Set 1 #
Total comments: 14
Patch Set 2 : review, test for many notifications #
Total comments: 3
Patch Set 3 : Attempt to create a good test, small review fixes #Patch Set 4 : test fix #Patch Set 5 : review, enlarging notifications with animation #Patch Set 6 : rebase on master #Patch Set 7 : rebase on master 2 #Patch Set 8 : compilation: stl variations #
Messages
Total messages: 48 (17 generated)
Description was changed from ========== Supporting shrinking/enlarging for notifications Extensions have api to update text of a displayed notification. Due to such update, notification recalculates it's size. However, there was a bug in toast_contents_view, that updating preferred_size_ didn't trigger update of corresponding widget. Not updating widget lead to a visual glitch. BUG=605656 R=dewittj@chromium.com ========== to ========== Supporting shrinking/enlarging for notifications Extensions have api to update text of a displayed notification. Due to such update, notification recalculates it's size. However, there was a bug in toast_contents_view, that updating preferred_size_ didn't trigger update of corresponding widget. Not updating widget lead to a visual glitch. BUG=605656 R=dewittj@chromium.com ==========
dyaroshev@yandex-team.ru changed reviewers: + dewittj@chromium.org, mukai@chromium.org, stevenjb@chromium.org - dewittj@chromium.com
On 2016/04/22 14:01:40, dyaroshev wrote: PTAL stevenjb, Jun Mukai, dewittj
any chance you can add a test with multiple notifications to ensure that the popup collection handles the size change correctly and moves the toasts?
https://codereview.chromium.org/1913433004/diff/1/ui/message_center/views/mes... File ui/message_center/views/message_popup_collection_unittest.cc (right): https://codereview.chromium.org/1913433004/diff/1/ui/message_center/views/mes... ui/message_center/views/message_popup_collection_unittest.cc:10: #include <memory> Is this necessary? https://codereview.chromium.org/1913433004/diff/1/ui/message_center/views/mes... ui/message_center/views/message_popup_collection_unittest.cc:547: std::string id = AddNotification(); const std::string& https://codereview.chromium.org/1913433004/diff/1/ui/message_center/views/mes... ui/message_center/views/message_popup_collection_unittest.cc:579: gfx::Rect(0, 0, 600, 400)); // Display-bounds. I believe you don't have to reset it. If necessary, put into TearDown() https://codereview.chromium.org/1913433004/diff/1/ui/message_center/views/toa... File ui/message_center/views/toast_contents_view.cc (right): https://codereview.chromium.org/1913433004/diff/1/ui/message_center/views/toa... ui/message_center/views/toast_contents_view.cc:96: gfx::Size new_size = GetToastSizeForView(message_view); You can remove this line https://codereview.chromium.org/1913433004/diff/1/ui/message_center/views/toa... ui/message_center/views/toast_contents_view.cc:148: if (new_bounds == GetWidget()->GetWindowBoundsInScreen()) This (and line 133) could be incorrect if this happens during the animation -- you should probably check with animated_bounds_end_? Or, since now everything is Aura, you can check GetWidget()->GetNativeWindow()->GetTargetBounds().
https://codereview.chromium.org/1913433004/diff/1/ui/message_center/views/mes... File ui/message_center/views/message_popup_collection_unittest.cc (right): https://codereview.chromium.org/1913433004/diff/1/ui/message_center/views/mes... ui/message_center/views/message_popup_collection_unittest.cc:558: }; Should we also test a smaller notification after the larger one? I assume that should have no effect? https://codereview.chromium.org/1913433004/diff/1/ui/message_center/views/toa... File ui/message_center/views/toast_contents_view.cc (right): https://codereview.chromium.org/1913433004/diff/1/ui/message_center/views/toa... ui/message_center/views/toast_contents_view.cc:96: gfx::Size new_size = GetToastSizeForView(message_view); On 2016/05/04 17:00:09, Jun Mukai wrote: > You can remove this line More than just "can" - this will fail to compile with an "unused variable" error on some platforms.
https://codereview.chromium.org/1913433004/diff/1/ui/message_center/views/mes... File ui/message_center/views/message_popup_collection_unittest.cc (right): https://codereview.chromium.org/1913433004/diff/1/ui/message_center/views/mes... ui/message_center/views/message_popup_collection_unittest.cc:10: #include <memory> On 2016/05/04 17:00:09, Jun Mukai wrote: > Is this necessary? Done. https://codereview.chromium.org/1913433004/diff/1/ui/message_center/views/mes... ui/message_center/views/message_popup_collection_unittest.cc:547: std::string id = AddNotification(); On 2016/05/04 17:00:09, Jun Mukai wrote: > const std::string& not apliable anymore https://codereview.chromium.org/1913433004/diff/1/ui/message_center/views/mes... ui/message_center/views/message_popup_collection_unittest.cc:558: }; On 2016/05/04 23:39:02, stevenjb wrote: > Should we also test a smaller notification after the larger one? I assume that > should have no effect? Done. https://codereview.chromium.org/1913433004/diff/1/ui/message_center/views/mes... ui/message_center/views/message_popup_collection_unittest.cc:579: gfx::Rect(0, 0, 600, 400)); // Display-bounds. On 2016/05/04 17:00:09, Jun Mukai wrote: > I believe you don't have to reset it. > If necessary, put into TearDown() I removed it in other tests too. https://codereview.chromium.org/1913433004/diff/1/ui/message_center/views/toa... File ui/message_center/views/toast_contents_view.cc (right): https://codereview.chromium.org/1913433004/diff/1/ui/message_center/views/toa... ui/message_center/views/toast_contents_view.cc:96: gfx::Size new_size = GetToastSizeForView(message_view); On 2016/05/04 17:00:09, Jun Mukai wrote: > You can remove this line Done. https://codereview.chromium.org/1913433004/diff/1/ui/message_center/views/toa... ui/message_center/views/toast_contents_view.cc:148: if (new_bounds == GetWidget()->GetWindowBoundsInScreen()) On 2016/05/04 17:00:09, Jun Mukai wrote: > This (and line 133) could be incorrect if this happens during the animation -- > you should probably check with animated_bounds_end_? > Or, since now everything is Aura, you can check > GetWidget()->GetNativeWindow()->GetTargetBounds(). 1) It seems like I can check animated_bounds_end_ here. I cannot do so in ...Instantly, cause we might get a text update, while animation isn't finished, so the glitch will still be seen. And it's not "instantly". Don't you agree? 2) Does this method mean, that widget knows about his animation? Maybe, we can get rid of animation_bounds_start_/end_? How I can pull that off?
On 2016/05/04 16:49:15, dewittj wrote: > any chance you can add a test with multiple notifications to ensure that the > popup collection handles the size change correctly and moves the toasts? Done?
owner lgtm (but please wait for lg from mukai@)
https://codereview.chromium.org/1913433004/diff/1/ui/message_center/views/toa... File ui/message_center/views/toast_contents_view.cc (right): https://codereview.chromium.org/1913433004/diff/1/ui/message_center/views/toa... ui/message_center/views/toast_contents_view.cc:148: if (new_bounds == GetWidget()->GetWindowBoundsInScreen()) On 2016/05/05 14:13:19, dyaroshev wrote: > On 2016/05/04 17:00:09, Jun Mukai wrote: > > This (and line 133) could be incorrect if this happens during the animation -- > > you should probably check with animated_bounds_end_? > > Or, since now everything is Aura, you can check > > GetWidget()->GetNativeWindow()->GetTargetBounds(). > > 1) It seems like I can check animated_bounds_end_ here. I cannot do so in > ...Instantly, cause we might get a text update, while animation isn't finished, > so the glitch will still be seen. And it's not "instantly". Don't you agree? > 2) Does this method mean, that widget knows about his animation? Maybe, we can > get rid of animation_bounds_start_/end_? How I can pull that off? Sorry, I was misunderstanding the (current) code a bit. This uses gfx::Animation, compute intermediate size and set it in AnimationProgressed(), thus these start_/end_ are actually required. My point was wrong. Having said that, I think the former part is still true. Let's think that notification size change happens very quickly and animation is still ongoing. And if the new_bounds happens to be same as the current window bounds, that means it doesn't stop the animation and goes to the bounds_end_, which might not be same as new_bounds and result in unexpected widget size. Therefore, you will have to compare with bounds_end_ and re-schedule the animation if new_bounds is not same as bounds_end_. https://codereview.chromium.org/1913433004/diff/20001/ui/message_center/views... File ui/message_center/views/message_popup_collection_unittest.cc (right): https://codereview.chromium.org/1913433004/diff/20001/ui/message_center/views... ui/message_center/views/message_popup_collection_unittest.cc:555: widget->GetWindowBoundsInScreen().width()) nit: EXPECT_EQ(toast->bounds().size(), widget->GetWindowBoundsInScreen().size()) ? https://codereview.chromium.org/1913433004/diff/20001/ui/message_center/views... ui/message_center/views/message_popup_collection_unittest.cc:561: << "\nwidget height: " << widget->GetWindowBoundsInScreen().height(); In case you have to keep EXPECT_TRUE, you may want to consider toast->bounds().size().ToString() for logging. https://codereview.chromium.org/1913433004/diff/20001/ui/message_center/views... File ui/message_center/views/toast_contents_view.cc (right): https://codereview.chromium.org/1913433004/diff/20001/ui/message_center/views... ui/message_center/views/toast_contents_view.cc:303: SetBoundsInstantly(bounds()); Based on the discussion for SetBoundsWithAnimation in the previous patchset -- I think you should invoke SetBoundsWithAnimation() when bounds_animation_ is ongoing. Think that notification is updated very quickly and therefore it's still in the revealing animation. This SetBoundsInstantly() means that the notification jumps to the final position from the intermediate position/size of the animation, and then upcoming animation event cance's the jump and continues the old animation. Instead, it should reschedule the animation and reset the bounds_end_ to the new bounds. This can be done by SetBoundsWithAnimation().
On 2016/05/05 14:13:58, dyaroshev wrote: > On 2016/05/04 16:49:15, dewittj wrote: > > any chance you can add a test with multiple notifications to ensure that the > > popup collection handles the size change correctly and moves the toasts? > > Done? Well, I think dewittj's point is a bit more complicated. Think that there are multiple notification popups, and then the one in the middle got expanded due to the update. Does that expansion result in overlapping with other popups? Or does the controller (MessageCenterPopupCollection) notice and moves the popups to avoid overlaps? Similarly on shrink, does that result in a bigger gap between popups? I think it's not tested in your code.
On 2016/05/05 18:05:23, Jun Mukai wrote:
> On 2016/05/05 14:13:58, dyaroshev wrote:
> > On 2016/05/04 16:49:15, dewittj wrote:
> > > any chance you can add a test with multiple notifications to ensure that
the
> > > popup collection handles the size change correctly and moves the toasts?
> >
> > Done?
>
> Well, I think dewittj's point is a bit more complicated.
> Think that there are multiple notification popups, and then the one in the
> middle
> got expanded due to the update. Does that expansion result in overlapping with
> other popups? Or does the controller (MessageCenterPopupCollection) notice
> and moves the popups to avoid overlaps?
> Similarly on shrink, does that result in a bigger gap between popups?
>
> I think it's not tested in your code.
I've uploaded another patch, and it doesn't aim to be the final one.
However, I've encountered couple of problems, there is hope, you might help me.
1) When we enlarge upper toasts, we do indead have an overlap of widgets,
for a few animation frames. I'm not able to see that.
But what should happened?
- if we enlarge with animation, it looks quite unpleasant,
because underlying view is drawn at fool size, so it gets trimmed in the
middle.
Looks kinda bad.
- If we enlarge to full size, the question arises, what the gap should be
between two toasts?
If we pick one from message_center_style, we don't get any animation.
When shrinking, it indeed leaves bigger gaps, but then widgets get
repositioned with
animation, so it looks alright.
2) Sometimes, test fails, when checking aligment a AnimationEnded, by one pixel
up or down.
Any idea why that might happen?
On 2016/05/06 16:51:02, dyaroshev wrote: > On 2016/05/05 18:05:23, Jun Mukai wrote: > > On 2016/05/05 14:13:58, dyaroshev wrote: > > > On 2016/05/04 16:49:15, dewittj wrote: > > > > any chance you can add a test with multiple notifications to ensure that > the > > > > popup collection handles the size change correctly and moves the toasts? > > > > > > Done? > > > > Well, I think dewittj's point is a bit more complicated. > > Think that there are multiple notification popups, and then the one in the > > middle > > got expanded due to the update. Does that expansion result in overlapping with > > other popups? Or does the controller (MessageCenterPopupCollection) notice > > and moves the popups to avoid overlaps? > > Similarly on shrink, does that result in a bigger gap between popups? > > > > I think it's not tested in your code. > > I've uploaded another patch, and it doesn't aim to be the final one. > However, I've encountered couple of problems, there is hope, you might help me. > > 1) When we enlarge upper toasts, we do indead have an overlap of widgets, > for a few animation frames. I'm not able to see that. > But what should happened? > - if we enlarge with animation, it looks quite unpleasant, > because underlying view is drawn at fool size, so it gets trimmed in the > middle. > Looks kinda bad. > - If we enlarge to full size, the question arises, what the gap should be > between two toasts? > If we pick one from message_center_style, we don't get any animation. > > When shrinking, it indeed leaves bigger gaps, but then widgets get > repositioned with > animation, so it looks alright. > > 2) Sometimes, test fails, when checking aligment a AnimationEnded, by one pixel > up or down. > Any idea why that might happen? I think, I figured out 2. probalby happens, because I have to aligment, when last animation ended, not any of them. My bad.
On 2016/05/05 18:05:23, Jun Mukai wrote: > On 2016/05/05 14:13:58, dyaroshev wrote: > > On 2016/05/04 16:49:15, dewittj wrote: > > > any chance you can add a test with multiple notifications to ensure that the > > > popup collection handles the size change correctly and moves the toasts? > > > > Done? > > Well, I think dewittj's point is a bit more complicated. > Think that there are multiple notification popups, and then the one in the > middle > got expanded due to the update. Does that expansion result in overlapping with > other popups? Or does the controller (MessageCenterPopupCollection) notice > and moves the popups to avoid overlaps? > Similarly on shrink, does that result in a bigger gap between popups? > > I think it's not tested in your code. My previous replies were a bit messy, and I've discovered a few more problems, so here is the summary: 1) Enlarging notification can cause widgets to overlap for a few frames, because top notification instantly became bigger than the gap, and bottom notification moves to it's position with animation. 2) Because each notification has it's own animation, notifications can overlap, when the top one already moved, and the bottom one hasn't yet. It's seems like collection should be in charge of animation. 3) Growing notifications with animation looks unpleasant, because underlying view does not take into consideration real borders, when it's drown. 4) Shrinking notifications doesn't cause problems. The gaps are decreased with animation. Question: What kind of animation should be done in case of enlarging notification?
On 2016/05/09 14:42:28, dyaroshev wrote: > On 2016/05/05 18:05:23, Jun Mukai wrote: > > On 2016/05/05 14:13:58, dyaroshev wrote: > > > On 2016/05/04 16:49:15, dewittj wrote: > > > > any chance you can add a test with multiple notifications to ensure that > the > > > > popup collection handles the size change correctly and moves the toasts? > > > > > > Done? > > > > Well, I think dewittj's point is a bit more complicated. > > Think that there are multiple notification popups, and then the one in the > > middle > > got expanded due to the update. Does that expansion result in overlapping with > > other popups? Or does the controller (MessageCenterPopupCollection) notice > > and moves the popups to avoid overlaps? > > Similarly on shrink, does that result in a bigger gap between popups? > > > > I think it's not tested in your code. > > My previous replies were a bit messy, and I've discovered a few more problems, > so here is the summary: > > 1) Enlarging notification can cause widgets to overlap for a few frames, because > top notification > instantly became bigger than the gap, and bottom notification moves to it's > position with animation. > > 2) Because each notification has it's own animation, notifications can overlap, > when the top one > already moved, and the bottom one hasn't yet. It's seems like collection should > be in charge > of animation. > > 3) Growing notifications with animation looks unpleasant, because underlying > view > does not take into consideration real borders, when it's drown. > > 4) Shrinking notifications doesn't cause problems. The gaps are decreased with > animation. > > Question: > What kind of animation should be done in case of enlarging notification? Let me focus on 3, I am afraid that you are misunderstanding my comments. What I cared was, can there be some case which cancels the new size? Let's think about actual examples. Currently animation happens on appearing; it comes from the right side, i.e. very narrow sized window appears and grows to the full width. If a website creates a notification and then update the content immediately and the new size is bigger. Let's say, the first height was 40 and the second height is 48. 1. window is sized in 5x40, schedule the revealing animation to 320x40 2. resize happens in the middle of the animation, window is resized to 320x48 3. the animation continues, and sized to 320x40 So in this scenario, the final size is not the one we expected -- am I right? If this is wrong, sorry. Then what is the ideal behavior? An idea is: a. if animation is onging, do not change the window (at 2 above). Instead remember this size, and resize the window at the end of the animation. b. or simply set the animation goal size to the new size at 2. I suggested b. first because it's easier to implement, and that would not be so bad. If you care about the graphics in the middle of the animation during b, consider implementing a.
On 2016/05/09 17:14:47, Jun Mukai wrote: > On 2016/05/09 14:42:28, dyaroshev wrote: > > On 2016/05/05 18:05:23, Jun Mukai wrote: > > > On 2016/05/05 14:13:58, dyaroshev wrote: > > > > On 2016/05/04 16:49:15, dewittj wrote: > > > > > any chance you can add a test with multiple notifications to ensure that > > the > > > > > popup collection handles the size change correctly and moves the toasts? > > > > > > > > Done? > > > > > > Well, I think dewittj's point is a bit more complicated. > > > Think that there are multiple notification popups, and then the one in the > > > middle > > > got expanded due to the update. Does that expansion result in overlapping > with > > > other popups? Or does the controller (MessageCenterPopupCollection) notice > > > and moves the popups to avoid overlaps? > > > Similarly on shrink, does that result in a bigger gap between popups? > > > > > > I think it's not tested in your code. > > > > My previous replies were a bit messy, and I've discovered a few more problems, > > so here is the summary: > > > > 1) Enlarging notification can cause widgets to overlap for a few frames, > because > > top notification > > instantly became bigger than the gap, and bottom notification moves to it's > > position with animation. > > > > 2) Because each notification has it's own animation, notifications can > overlap, > > when the top one > > already moved, and the bottom one hasn't yet. It's seems like collection > should > > be in charge > > of animation. > > > > 3) Growing notifications with animation looks unpleasant, because underlying > > view > > does not take into consideration real borders, when it's drown. > > > > 4) Shrinking notifications doesn't cause problems. The gaps are decreased with > > animation. > > > > Question: > > What kind of animation should be done in case of enlarging notification? > > Let me focus on 3, I am afraid that you are misunderstanding my comments. > What I cared was, can there be some case which cancels the new size? > > > Let's think about actual examples. Currently animation happens on appearing; > it comes from the right side, i.e. very narrow sized window appears and grows > to the full width. > > If a website creates a notification and then update the content immediately > and the new size is bigger. Let's say, the first height was 40 and the second > height is 48. > 1. window is sized in 5x40, schedule the revealing animation to 320x40 > 2. resize happens in the middle of the animation, window is resized to 320x48 > 3. the animation continues, and sized to 320x40 > > So in this scenario, the final size is not the one we expected -- am I right? > If this is wrong, sorry. > > Then what is the ideal behavior? > An idea is: > a. if animation is onging, do not change the window (at 2 above). Instead > remember this size, and resize the window at the end of the animation. > b. or simply set the animation goal size to the new size at 2. > > I suggested b. first because it's easier to implement, and that would not be > so bad. > If you care about the graphics in the middle of the animation during b, > consider implementing a. For the animation caused MessagePopupCollection, I am not sure how it's related here. It's okay to keep this code -- i.e. resize happens instantly, and then ask the collection to relocate the notifications. This relocation will cause some animations, but that's out of the scope of this class.
On 2016/05/09 17:22:45, Jun Mukai wrote: > On 2016/05/09 17:14:47, Jun Mukai wrote: > > On 2016/05/09 14:42:28, dyaroshev wrote: > > > On 2016/05/05 18:05:23, Jun Mukai wrote: > > > > On 2016/05/05 14:13:58, dyaroshev wrote: > > > > > On 2016/05/04 16:49:15, dewittj wrote: > > > > > > any chance you can add a test with multiple notifications to ensure > that > > > the > > > > > > popup collection handles the size change correctly and moves the > toasts? > > > > > > > > > > Done? > > > > > > > > Well, I think dewittj's point is a bit more complicated. > > > > Think that there are multiple notification popups, and then the one in the > > > > middle > > > > got expanded due to the update. Does that expansion result in overlapping > > with > > > > other popups? Or does the controller (MessageCenterPopupCollection) notice > > > > and moves the popups to avoid overlaps? > > > > Similarly on shrink, does that result in a bigger gap between popups? > > > > > > > > I think it's not tested in your code. > > > > > > My previous replies were a bit messy, and I've discovered a few more > problems, > > > so here is the summary: > > > > > > 1) Enlarging notification can cause widgets to overlap for a few frames, > > because > > > top notification > > > instantly became bigger than the gap, and bottom notification moves to it's > > > position with animation. > > > > > > 2) Because each notification has it's own animation, notifications can > > overlap, > > > when the top one > > > already moved, and the bottom one hasn't yet. It's seems like collection > > should > > > be in charge > > > of animation. > > > > > > 3) Growing notifications with animation looks unpleasant, because underlying > > > view > > > does not take into consideration real borders, when it's drown. > > > > > > 4) Shrinking notifications doesn't cause problems. The gaps are decreased > with > > > animation. > > > > > > Question: > > > What kind of animation should be done in case of enlarging notification? > > > > Let me focus on 3, I am afraid that you are misunderstanding my comments. > > What I cared was, can there be some case which cancels the new size? > > > > > > Let's think about actual examples. Currently animation happens on appearing; > > it comes from the right side, i.e. very narrow sized window appears and grows > > to the full width. > > > > If a website creates a notification and then update the content immediately > > and the new size is bigger. Let's say, the first height was 40 and the second > > height is 48. > > 1. window is sized in 5x40, schedule the revealing animation to 320x40 > > 2. resize happens in the middle of the animation, window is resized to 320x48 > > 3. the animation continues, and sized to 320x40 > > > > So in this scenario, the final size is not the one we expected -- am I right? > > If this is wrong, sorry. > > > > Then what is the ideal behavior? > > An idea is: > > a. if animation is onging, do not change the window (at 2 above). Instead > > remember this size, and resize the window at the end of the animation. > > b. or simply set the animation goal size to the new size at 2. > > > > I suggested b. first because it's easier to implement, and that would not be > > so bad. > > If you care about the graphics in the middle of the animation during b, > > consider implementing a. > > > For the animation caused MessagePopupCollection, I am not sure how it's > related here. It's okay to keep this code -- i.e. resize happens instantly, > and then ask the collection to relocate the notifications. This relocation > will cause some animations, but that's out of the scope of this class. I'm sorry, I wasn't clear enough about 3. This is what currently happening (with my patch) (numbers are not accurate) 1. - initial state notification0 [100, 250] notification1 [260, 350] 2. - notification0 get's enlarged. notification 0 [100, 300] notification1 [260, 350] -> [270, 360] -> [280, 370] -> .. -> (overlap for a few frames) It doesn't look all that scary, but is still there. While attempting to fix it I've encountered another problem: overlap can happen, during the animation: 1. - initial notification0 [100, 250] notification1 [260, 350] 2. notification0 goes through animation: notification0 [115, 265] notification1 [260, 350] Overlap! (I do not know, if the second one is important). To fix the first problem, I've tried to grow to a bigger size with animation. Not very beautiful, but this a very rear case, and it can be done easily. (original point 3). Another thing, that can be done with the first problem, - is to enlarge to full size, and then build complex animation. This requires somewhat big transformations in message_popup_collection. The second problem, can be solved, by making collection itself react to animation, not notifications individually. So my question is, what should be done, considering overlaps?
On 2016/05/09 18:00:08, dyaroshev wrote: > On 2016/05/09 17:22:45, Jun Mukai wrote: > > On 2016/05/09 17:14:47, Jun Mukai wrote: > > > On 2016/05/09 14:42:28, dyaroshev wrote: > > > > On 2016/05/05 18:05:23, Jun Mukai wrote: > > > > > On 2016/05/05 14:13:58, dyaroshev wrote: > > > > > > On 2016/05/04 16:49:15, dewittj wrote: > > > > > > > any chance you can add a test with multiple notifications to ensure > > that > > > > the > > > > > > > popup collection handles the size change correctly and moves the > > toasts? > > > > > > > > > > > > Done? > > > > > > > > > > Well, I think dewittj's point is a bit more complicated. > > > > > Think that there are multiple notification popups, and then the one in > the > > > > > middle > > > > > got expanded due to the update. Does that expansion result in > overlapping > > > with > > > > > other popups? Or does the controller (MessageCenterPopupCollection) > notice > > > > > and moves the popups to avoid overlaps? > > > > > Similarly on shrink, does that result in a bigger gap between popups? > > > > > > > > > > I think it's not tested in your code. > > > > > > > > My previous replies were a bit messy, and I've discovered a few more > > problems, > > > > so here is the summary: > > > > > > > > 1) Enlarging notification can cause widgets to overlap for a few frames, > > > because > > > > top notification > > > > instantly became bigger than the gap, and bottom notification moves to > it's > > > > position with animation. > > > > > > > > 2) Because each notification has it's own animation, notifications can > > > overlap, > > > > when the top one > > > > already moved, and the bottom one hasn't yet. It's seems like collection > > > should > > > > be in charge > > > > of animation. > > > > > > > > 3) Growing notifications with animation looks unpleasant, because > underlying > > > > view > > > > does not take into consideration real borders, when it's drown. > > > > > > > > 4) Shrinking notifications doesn't cause problems. The gaps are decreased > > with > > > > animation. > > > > > > > > Question: > > > > What kind of animation should be done in case of enlarging notification? > > > > > > Let me focus on 3, I am afraid that you are misunderstanding my comments. > > > What I cared was, can there be some case which cancels the new size? > > > > > > > > > Let's think about actual examples. Currently animation happens on appearing; > > > it comes from the right side, i.e. very narrow sized window appears and > grows > > > to the full width. > > > > > > If a website creates a notification and then update the content immediately > > > and the new size is bigger. Let's say, the first height was 40 and the > second > > > height is 48. > > > 1. window is sized in 5x40, schedule the revealing animation to 320x40 > > > 2. resize happens in the middle of the animation, window is resized to > 320x48 > > > 3. the animation continues, and sized to 320x40 > > > > > > So in this scenario, the final size is not the one we expected -- am I > right? > > > If this is wrong, sorry. > > > > > > Then what is the ideal behavior? > > > An idea is: > > > a. if animation is onging, do not change the window (at 2 above). Instead > > > remember this size, and resize the window at the end of the animation. > > > b. or simply set the animation goal size to the new size at 2. > > > > > > I suggested b. first because it's easier to implement, and that would not be > > > so bad. > > > If you care about the graphics in the middle of the animation during b, > > > consider implementing a. > > > > > > For the animation caused MessagePopupCollection, I am not sure how it's > > related here. It's okay to keep this code -- i.e. resize happens instantly, > > and then ask the collection to relocate the notifications. This relocation > > will cause some animations, but that's out of the scope of this class. > > I'm sorry, I wasn't clear enough about 3. > > This is what currently happening (with my patch) (numbers are not accurate) > 1. - initial state > notification0 [100, 250] > notification1 [260, 350] > > 2. - notification0 get's enlarged. > notification 0 [100, 300] > notification1 [260, 350] -> [270, 360] -> [280, 370] -> .. -> (overlap for a few > frames) > > It doesn't look all that scary, but is still there. > > While attempting to fix it I've encountered another problem: > overlap can happen, during the animation: > 1. - initial > notification0 [100, 250] > notification1 [260, 350] > > 2. notification0 goes through animation: > notification0 [115, 265] > notification1 [260, 350] > Overlap! > (I do not know, if the second one is important). > > To fix the first problem, I've tried to grow to a bigger size with animation. > Not very beautiful, but this a very rear case, and it can be done easily. > (original point 3). > > Another thing, that can be done with the first problem, - is to enlarge to full > size, > and then build complex animation. > This requires somewhat big transformations in message_popup_collection. > > The second problem, can be solved, by making collection itself react to > animation, > not notifications individually. > > So my question is, what should be done, considering overlaps? Sorry for the confusion on my side... I think what we should really care about is the final status of the animation. If the notifications do not overlap at the end, that's fine. The intermediate graphic effect is less important. This means, I think overlap happens during the animation is okay, and I think that looks not so bad -- so I think 'original point 3' would be fine.
On 2016/05/09 18:22:30, Jun Mukai wrote: > On 2016/05/09 18:00:08, dyaroshev wrote: > > On 2016/05/09 17:22:45, Jun Mukai wrote: > > > On 2016/05/09 17:14:47, Jun Mukai wrote: > > > > On 2016/05/09 14:42:28, dyaroshev wrote: > > > > > On 2016/05/05 18:05:23, Jun Mukai wrote: > > > > > > On 2016/05/05 14:13:58, dyaroshev wrote: > > > > > > > On 2016/05/04 16:49:15, dewittj wrote: > > > > > > > > any chance you can add a test with multiple notifications to > ensure > > > that > > > > > the > > > > > > > > popup collection handles the size change correctly and moves the > > > toasts? > > > > > > > > > > > > > > Done? > > > > > > > > > > > > Well, I think dewittj's point is a bit more complicated. > > > > > > Think that there are multiple notification popups, and then the one in > > the > > > > > > middle > > > > > > got expanded due to the update. Does that expansion result in > > overlapping > > > > with > > > > > > other popups? Or does the controller (MessageCenterPopupCollection) > > notice > > > > > > and moves the popups to avoid overlaps? > > > > > > Similarly on shrink, does that result in a bigger gap between popups? > > > > > > > > > > > > I think it's not tested in your code. > > > > > > > > > > My previous replies were a bit messy, and I've discovered a few more > > > problems, > > > > > so here is the summary: > > > > > > > > > > 1) Enlarging notification can cause widgets to overlap for a few frames, > > > > because > > > > > top notification > > > > > instantly became bigger than the gap, and bottom notification moves to > > it's > > > > > position with animation. > > > > > > > > > > 2) Because each notification has it's own animation, notifications can > > > > overlap, > > > > > when the top one > > > > > already moved, and the bottom one hasn't yet. It's seems like collection > > > > should > > > > > be in charge > > > > > of animation. > > > > > > > > > > 3) Growing notifications with animation looks unpleasant, because > > underlying > > > > > view > > > > > does not take into consideration real borders, when it's drown. > > > > > > > > > > 4) Shrinking notifications doesn't cause problems. The gaps are > decreased > > > with > > > > > animation. > > > > > > > > > > Question: > > > > > What kind of animation should be done in case of enlarging notification? > > > > > > > > Let me focus on 3, I am afraid that you are misunderstanding my comments. > > > > What I cared was, can there be some case which cancels the new size? > > > > > > > > > > > > Let's think about actual examples. Currently animation happens on > appearing; > > > > it comes from the right side, i.e. very narrow sized window appears and > > grows > > > > to the full width. > > > > > > > > If a website creates a notification and then update the content > immediately > > > > and the new size is bigger. Let's say, the first height was 40 and the > > second > > > > height is 48. > > > > 1. window is sized in 5x40, schedule the revealing animation to 320x40 > > > > 2. resize happens in the middle of the animation, window is resized to > > 320x48 > > > > 3. the animation continues, and sized to 320x40 > > > > > > > > So in this scenario, the final size is not the one we expected -- am I > > right? > > > > If this is wrong, sorry. > > > > > > > > Then what is the ideal behavior? > > > > An idea is: > > > > a. if animation is onging, do not change the window (at 2 above). Instead > > > > remember this size, and resize the window at the end of the animation. > > > > b. or simply set the animation goal size to the new size at 2. > > > > > > > > I suggested b. first because it's easier to implement, and that would not > be > > > > so bad. > > > > If you care about the graphics in the middle of the animation during b, > > > > consider implementing a. > > > > > > > > > For the animation caused MessagePopupCollection, I am not sure how it's > > > related here. It's okay to keep this code -- i.e. resize happens instantly, > > > and then ask the collection to relocate the notifications. This relocation > > > will cause some animations, but that's out of the scope of this class. > > > > I'm sorry, I wasn't clear enough about 3. > > > > This is what currently happening (with my patch) (numbers are not accurate) > > 1. - initial state > > notification0 [100, 250] > > notification1 [260, 350] > > > > 2. - notification0 get's enlarged. > > notification 0 [100, 300] > > notification1 [260, 350] -> [270, 360] -> [280, 370] -> .. -> (overlap for a > few > > frames) > > > > It doesn't look all that scary, but is still there. > > > > While attempting to fix it I've encountered another problem: > > overlap can happen, during the animation: > > 1. - initial > > notification0 [100, 250] > > notification1 [260, 350] > > > > 2. notification0 goes through animation: > > notification0 [115, 265] > > notification1 [260, 350] > > Overlap! > > (I do not know, if the second one is important). > > > > To fix the first problem, I've tried to grow to a bigger size with animation. > > Not very beautiful, but this a very rear case, and it can be done easily. > > (original point 3). > > > > Another thing, that can be done with the first problem, - is to enlarge to > full > > size, > > and then build complex animation. > > This requires somewhat big transformations in message_popup_collection. > > > > The second problem, can be solved, by making collection itself react to > > animation, > > not notifications individually. > > > > So my question is, what should be done, considering overlaps? > > Sorry for the confusion on my side... > > I think what we should really care about is the final status of the animation. > If the notifications do not overlap at the end, that's fine. The intermediate > graphic effect is less important. > > This means, I think overlap happens during the animation is okay, and I think > that looks not so bad -- so I think 'original point 3' would be fine. If I understood you correctly, I did what you asked for. Here is what it looks like: https://drive.google.com/open?id=0B5pZ43DhqaQDcERJSUtqdkk3YXM
LGTM
looks great, thanks, lgtm
The CQ bit was checked by dyaroshev@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/1913433004/#ps80001 (title: "review, enlarging notifications with animation")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1913433004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1913433004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dyaroshev@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1913433004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1913433004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dyaroshev@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, mukai@chromium.org, dewittj@chromium.org Link to the patchset: https://codereview.chromium.org/1913433004/#ps100001 (title: "rebase on master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1913433004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1913433004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dyaroshev@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, mukai@chromium.org, dewittj@chromium.org Link to the patchset: https://codereview.chromium.org/1913433004/#ps120001 (title: "rebase on master 2")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1913433004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1913433004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by dyaroshev@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, mukai@chromium.org, dewittj@chromium.org Link to the patchset: https://codereview.chromium.org/1913433004/#ps140001 (title: "compilation: stl variations")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1913433004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1913433004/140001
Message was sent while issue was closed.
Description was changed from ========== Supporting shrinking/enlarging for notifications Extensions have api to update text of a displayed notification. Due to such update, notification recalculates it's size. However, there was a bug in toast_contents_view, that updating preferred_size_ didn't trigger update of corresponding widget. Not updating widget lead to a visual glitch. BUG=605656 R=dewittj@chromium.com ========== to ========== Supporting shrinking/enlarging for notifications Extensions have api to update text of a displayed notification. Due to such update, notification recalculates it's size. However, there was a bug in toast_contents_view, that updating preferred_size_ didn't trigger update of corresponding widget. Not updating widget lead to a visual glitch. BUG=605656 R=dewittj@chromium.com ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Supporting shrinking/enlarging for notifications Extensions have api to update text of a displayed notification. Due to such update, notification recalculates it's size. However, there was a bug in toast_contents_view, that updating preferred_size_ didn't trigger update of corresponding widget. Not updating widget lead to a visual glitch. BUG=605656 R=dewittj@chromium.com ========== to ========== Supporting shrinking/enlarging for notifications Extensions have api to update text of a displayed notification. Due to such update, notification recalculates it's size. However, there was a bug in toast_contents_view, that updating preferred_size_ didn't trigger update of corresponding widget. Not updating widget lead to a visual glitch. BUG=605656 R=dewittj@chromium.com Committed: https://crrev.com/2f78eb16262f1442e4c2b0cc9a03689e5cc47353 Cr-Commit-Position: refs/heads/master@{#392906} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/2f78eb16262f1442e4c2b0cc9a03689e5cc47353 Cr-Commit-Position: refs/heads/master@{#392906} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
