Panels: Adding 3-step animation on minimize on Linux

chrome/browser/ui/panels/panel_slide_animation.cc

+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/panels/panel_slide_animation.h"
+
+#include "chrome/browser/ui/panels/panel.h"
+
+namespace {

[prasadt, 2011/12/06 23:21:30]

Empty line needed as per coding-style.

[Dmitry Titov, 2011/12/07 03:46:04]

Done.

+// These values are experimental and subjective.
+const int kSetBoundsAnimationMs = 180;
+const int kSetBoundsAnimationMinimizeMs = 1500;

[prasadt, 2011/12/06 23:21:30]

Empty line needed as per coding-style.

[Dmitry Titov, 2011/12/07 03:46:04]

Done.

+}
+
+PanelSlideAnimation::PanelSlideAnimation(ui::AnimationDelegate* target,
+                                         Panel* panel,
+                                         gfx::Rect initial_bounds,
+                                         gfx::Rect final_bounds)
+  : ui::SlideAnimation(target),
+    panel_(panel),
+    for_minimize_(false),
+    animation_stop_to_show_titlebar_(0) {
+  // Detect animation that happens when expansion state is set to MINIMIZED
+  // and there is relatively big portion of the panel to hide from view.
+  // Initialize animation differently in this case, using fast-pause-slow
+  // method, see below for more details.
+  int duration = kSetBoundsAnimationMs;
+  if (panel_->expansion_state() == Panel::MINIMIZED) {
+    double hidden_title_height =
+        panel_->TitleOnlyHeight() - final_bounds.height();
+    double distance_y = initial_bounds.height() - final_bounds.height();
+    animation_stop_to_show_titlebar_ = 1.0 - hidden_title_height / distance_y;
+    if (animation_stop_to_show_titlebar_ > 0.7) {  // Relatively big movement.

[prasadt, 2011/12/06 23:21:30]

I find it easier to read the other way:

distance_to_title_height_ratio = distance_y / hidden_title_height;
if (distance_to_title_height_ratio > 2.5) {
....

[Dmitry Titov, 2011/12/07 03:46:04]

Hmm. it would add another long-name variable. Why would it be easier to read?

[prasadt, 2011/12/07 17:59:31]

distance_to_title_height_ratio tells me its a ratio of the two specified
entities.

animation_stop_to_show_titlebar_, I can't tell what it is without working
through the math. animation_stop_to_distance_ratio_?

I don't have great suggestions for an alternate name, so keep what you got if
you like that best.

+      for_minimize_ = true;

[prasadt, 2011/12/06 23:21:30]

for_minimize_ is not quite what it is. How about calling it is_three_step_ which
is what it is?

[Dmitry Titov, 2011/12/07 03:46:04]

I wanted to capture "we are doing minimize animation" semantic here... The way
we do it can change (and did change few times when I was implementing it,
perhaps it will become 4 steps etc) but it still will be 'minimize' animation.

+      duration = kSetBoundsAnimationMinimizeMs;

[prasadt, 2011/12/06 23:21:30]

Same comment - I suggest calling it kSetBoundsAnimationThreeStepMs.

[Dmitry Titov, 2011/12/07 03:46:04]

Ditto. 3 steps is just current impl of it.

+    }
+  }
+  SetSlideDuration(duration);
+}
+
+PanelSlideAnimation::~PanelSlideAnimation() { }
+
+double PanelSlideAnimation::GetCurrentValue() const {
+  double progress = ui::SlideAnimation::GetCurrentValue();
+  if (!for_minimize_) {
+    // Cubic easing out.
+    float value = 1.0 - progress;
+    return 1.0 - value * value * value;
+  }
+
+  // Minimize animation:

[prasadt, 2011/12/06 23:21:30]

Minimize animation: -> Three step animation:

[dcheng, 2011/12/06 23:49:01]

Doesn't the 3-step animation only apply to minimizing? If that's the case, I
think the current naming makes sense.

[prasadt, 2011/12/06 23:58:11]

3-step implies minimizing but minimizing does not imply 3-step. 3-step is done
for minimize only when the window is big enough to warrant it.

So, sometimes you can have for_minimize_=false and yet have it be a Minimize
animation.

[dcheng, 2011/12/07 00:02:45]

I see. I don't think three step is particularly specific either (what are the
three steps? how do they work) but I see your point. Maybe we should just call
it should_use_fancy_pants_animation_. =)

(In all seriousness maybe an enum would work well here...)

[Dmitry Titov, 2011/12/07 03:46:04]

I changed comment to mention that minimize animation is implemented as 3-step
one. Really, the 3-step-ness of it is just in the following 3 lines, otherwise
this class is about catching and playing minimize movement in a different way,
not about the way it's done.

I understand what you mean by saying not every minimize is 3 step, changed the
variable name to better reflect that.

+  // 1. Quickly (0 -> 0.15) make only titlebar visible.
+  // 2. Stay a little bit (0.15->0.6) in place, just showing titlebar.
+  // 3. Slowly minimize to thin strip (0.6->1.0)
+  const double kAnimationStopAfterQuickDecrease = 0.15;
+  const double kAnimationStopAfterShowingTitlebar = 0.6;
+  double value;
+  if (progress <= kAnimationStopAfterQuickDecrease) {
+      value = progress * animation_stop_to_show_titlebar_ /
+          kAnimationStopAfterQuickDecrease;

[prasadt, 2011/12/06 23:21:30]

I think the intent comes through more clearly if you say:

animation_stop_to_show_titlebar_ * (progress / kAnimationStopAfterQuickDecrease)

[Dmitry Titov, 2011/12/07 03:46:04]

Done.

+  } else if (progress <= kAnimationStopAfterShowingTitlebar) {

[prasadt, 2011/12/06 23:21:30]

Shouldn't the if condition be:
if (progress <= (kAnimationStopAfterQuickDecrease +
kAnimationStopAfterShowingTitlebar))

The animation from title only to three pixels feels more sluggish than the
numbers suggest. I think this is why.

[Dmitry Titov, 2011/12/07 03:46:04]

Discussed offline, changed constants names to be more descriptive.

+      value = animation_stop_to_show_titlebar_;
+  } else {
+      value = animation_stop_to_show_titlebar_ +
+          (progress - kAnimationStopAfterShowingTitlebar) *
+          (1.0 - animation_stop_to_show_titlebar_) /
+          (1.0 - kAnimationStopAfterShowingTitlebar);

[prasadt, 2011/12/06 23:21:30]

The above equation doesn't seem to add up for somehow:

I believe the following is the intent:

animation_stop_to_show_titlebar_ +
(progress - animation_stop_to_show_titlebar_) *
(progress - (kAnimationStopAfterQuickDecrease + 
             kAnimationStopAfterShowingTitlebar)) / 
(1.0 - (kAnimationStopAfterQuickDecrease + 
        kAnimationStopAfterShowingTitlebar))

[Dmitry Titov, 2011/12/07 03:46:04]

Changed from 'physical' (time past * speed) to 'mathematical' (time percentage *
distance) shape, as discussed offline.

+  }
+  return value;
+}
+