SideBySide2: Reduce scrolling jank

Update the other CodeMirror3 top position immediately, rather than
waiting with a delay. This improves UI feel by reducing the visible
jank during scrolling.

In the common case of scrolling a large number of lines that are
unmodified and visible, the line heights are identical and adjusting
the Y position immediately comes up with correct results. This gives
scrolling the UI a smoother feeling, like the old SideBySide view.

While scrolling run a timer every 20 milliseconds to update the other
CodeMirror3 with more accurate position information. This is necessary
to fix up incorrect widget height estimates. The height of offscreen
line widgets used for padding and comment boxes aren't computed by CM3
until they first become visible in the document, which sometimes causes
the view to be unaligned.

The timer uses a second 20 millisecond delay (for a total of 40 ms) to
detect when scrolling ends. The full 40ms delay must elapse before
the user can switch to the other CM3 instance and begin scrolling
again. Scrolling on the same document is always available and resets
the timer for another 40ms delay.

Change-Id: I40d6271cd5103249f43e9ecaf6eb1e75181a7dbb

gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/ScrollSynchronizer.java

@@ -0,0 +1,123 @@
+// Copyright (C) 2013 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.client.diff;
+
+import com.google.gerrit.client.Gerrit;
+import com.google.gerrit.client.diff.LineMapper.LineOnOtherInfo;
+import com.google.gwt.user.client.Timer;
+
+import net.codemirror.lib.CodeMirror;
+import net.codemirror.lib.CodeMirror.Viewport;
+import net.codemirror.lib.ScrollInfo;
+
+class ScrollSynchronizer {
+  private DiffTable diffTable;
+  private LineMapper mapper;
+  private ScrollCallback active;
+
+  void init(DiffTable diffTable,
+      CodeMirror cmA, CodeMirror cmB,
+      LineMapper mapper) {
+    this.diffTable = diffTable;
+    this.mapper = mapper;
+
+    cmA.on("scroll", new ScrollCallback(cmA, cmB, DisplaySide.A));
+    cmB.on("scroll", new ScrollCallback(cmB, cmA, DisplaySide.B));
+  }
+
+  private void updateScreenHeader(CodeMirror cm) {
+    ScrollInfo si = cm.getScrollInfo();
+    if (si.getTop() == 0 && !Gerrit.isHeaderVisible()) {
+      Gerrit.setHeaderVisible(true);
+      diffTable.updateFileCommentVisibility(false);
+    } else if (si.getTop() > 0.5 * si.getClientHeight()
+        && Gerrit.isHeaderVisible()) {
+      Gerrit.setHeaderVisible(false);
+      diffTable.updateFileCommentVisibility(true);
+    }
+  }
+
+  class ScrollCallback implements Runnable {
+    private final CodeMirror src;
+    private final CodeMirror dst;
+    private final DisplaySide srcSide;
+    private final Timer fixup;
+    private int state;
+
+    ScrollCallback(CodeMirror src, CodeMirror dst, DisplaySide srcSide) {
+      this.src = src;
+      this.dst = dst;
+      this.srcSide = srcSide;
+      this.fixup = new Timer() {
+        @Override
+        public void run() {
+          if (active == ScrollCallback.this) {
+            fixup();
+          }
+        }
+      };
+    }
+
+    @Override
+    public void run() {
+      if (active == null) {
+        active = this;
+        fixup.scheduleRepeating(20);
+      }
+      if (active == this) {
+        updateScreenHeader(src);
+        dst.scrollToY(src.getScrollInfo().getTop());
+        state = 0;
+      }
+    }
+
+    private void fixup() {
+      switch (state) {
+        case 0:
+          state = 1;
+          break;
+        case 1:
+          state = 2;
+          return;
+        case 2:
+          active = null;
+          fixup.cancel();
+          return;
+      }
+
+      // Since CM doesn't always take the height of line widgets into
+      // account when calculating scrollInfo when scrolling too fast (e.g.
+      // throw scrolling), simply setting scrollTop to be the same doesn't
+      // guarantee alignment.
+      //
+      // Iterate over the viewport to find the first line that isn't part of
+      // an insertion or deletion gap, for which isAligned() will be true.
+      // We then manually examine if the lines that should be aligned are at
+      // the same height. If not, perform additional scrolling.
+      Viewport fromTo = src.getViewport();
+      for (int line = fromTo.getFrom(); line <= fromTo.getTo(); line++) {
+        LineOnOtherInfo info = mapper.lineOnOther(srcSide, line);
+        if (info.isAligned()) {
+          double sy = src.heightAtLine(line);
+          double dy = dst.heightAtLine(info.getLine());
+          if (Math.abs(dy - sy) >= 1) {
+            dst.scrollToY(dst.getScrollInfo().getTop() + (dy - sy));
+          }
+          break;
+        }
+      }
+    }
+  }
+}

gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySide2.java

@@ -61,7 +61,6 @@ import com.google.gwt.event.shared.HandlerRegistration;
 import com.google.gwt.uibinder.client.UiBinder;
 import com.google.gwt.uibinder.client.UiField;
 import com.google.gwt.user.client.DOM;
-import com.google.gwt.user.client.Timer;
 import com.google.gwt.user.client.Window;
 import com.google.gwt.user.client.rpc.AsyncCallback;
 import com.google.gwt.user.client.ui.FlowPanel;
@@ -85,7 +84,6 @@ import net.codemirror.lib.KeyMap;
 import net.codemirror.lib.LineCharacter;
 import net.codemirror.lib.LineWidget;
 import net.codemirror.lib.ModeInjector;
-import net.codemirror.lib.ScrollInfo;
 import net.codemirror.lib.TextMarker.FromTo;
 
 import java.util.ArrayList;
@@ -118,8 +116,7 @@ public class SideBySide2 extends Screen {
   private CodeMirror cmA;
   private CodeMirror cmB;
   private CodeMirror lastFocused;
-  private Timer scrollTimerA;
+  private ScrollSynchronizer scrollingGlue;
-  private Timer scrollTimerB;
   private HandlerRegistration resizeHandler;
   private JsArray<CommentInfo> publishedBase;
   private JsArray<CommentInfo> publishedRevision;
@@ -286,7 +283,6 @@ public class SideBySide2 extends Screen {
   private void registerCmEvents(final CodeMirror cm) {
     cm.on("cursorActivity", updateActiveLine(cm));
     cm.on("gutterClick", onGutterClick(cm));
-    cm.on("scroll", doScroll(cm));
     cm.on("renderLine", resizeLinePadding(getSideFromCm(cm)));
     cm.on("viewportChange", adjustGutters(cm));
     cm.on("focus", new Runnable() {
@@ -416,18 +412,6 @@ public class SideBySide2 extends Screen {
   private void display(DiffInfo diffInfo) {
     cmA = displaySide(diffInfo.meta_a(), diffInfo.text_a(), diffTable.cmA);
     cmB = displaySide(diffInfo.meta_b(), diffInfo.text_b(), diffTable.cmB);
-    scrollTimerA = new Timer() {
-      @Override
-      public void run() {
-        fixScroll(cmA);
-      }
-    };
-    scrollTimerB = new Timer() {
-      @Override
-      public void run() {
-        fixScroll(cmB);
-      }
-    };
 
     skips = new ArrayList<SkippedLine>();
     linePaddingOnOtherSideMap = new HashMap<LineHandle, LinePaddingWidgetWrapper>();
@@ -454,6 +438,10 @@ public class SideBySide2 extends Screen {
     renderSkips();
     registerCmEvents(cmA);
     registerCmEvents(cmB);
+
+    scrollingGlue = GWT.create(ScrollSynchronizer.class);
+    scrollingGlue.init(diffTable, cmA, cmB, mapper);
+
     resizeHandler = Window.addResizeHandler(new ResizeHandler() {
       @Override
       public void onResize(ResizeEvent event) {
@@ -919,63 +907,6 @@ public class SideBySide2 extends Screen {
     }
   }
 
-  private Runnable doScroll(final CodeMirror cm) {
-    return new Runnable() {
-      public void run() {
-        // Hack to prevent feedback loop.
-        if (cm.getScrollSetAt() + 5 > System.currentTimeMillis()) {
-          return;
-        }
-        if (cm == cmA) {
-          scrollTimerB.cancel();
-          scrollTimerA.schedule(5);
-        } else {
-          scrollTimerA.cancel();
-          scrollTimerB.schedule(5);
-        }
-      }
-    };
-  }
-
-  private void fixScroll(CodeMirror cm) {
-    ScrollInfo si = cm.getScrollInfo();
-    if (si.getTop() == 0 && !Gerrit.isHeaderVisible()) {
-      Gerrit.setHeaderVisible(true);
-      diffTable.updateFileCommentVisibility(false);
-    } else if (si.getTop() > 0.5 * si.getClientHeight()
-        && Gerrit.isHeaderVisible()) {
-      Gerrit.setHeaderVisible(false);
-      diffTable.updateFileCommentVisibility(true);
-    }
-    CodeMirror other = otherCm(cm);
-    Viewport fromTo = cm.getViewport();
-    int line = fromTo.getFrom();
-    for (; line <= fromTo.getTo(); line++) {
-      LineOnOtherInfo info = mapper.lineOnOther(getSideFromCm(cm), line);
-      /**
-       * Since CM doesn't always take the height of line widgets into
-       * account when calculating scrollInfo when scrolling too fast
-       * (e.g. throw-scrolling), simply setting scrollTop to be the same
-       * doesn't guarantee alignment.
-       *
-       * Iterate over the viewport to find the first line that isn't part of an
-       * insertion or deletion gap, for which isAligned() will be true. We then
-       * manually examine if the lines that should be aligned are at the same
-       * height. If not, perform additional scrolling.
-       */
-      if (info.isAligned()) {
-        double myHeight = cm.heightAtLine(line);
-        double otherHeight = other.heightAtLine(info.getLine());
-        if (myHeight != otherHeight) {
-          other.scrollToY(
-              other.getScrollInfo().getTop() + otherHeight - myHeight);
-          other.setScrollSetAt(System.currentTimeMillis());
-        }
-        break;
-      }
-    }
-  }
-
   private Runnable adjustGutters(final CodeMirror cm) {
     return new Runnable() {
       @Override

gerrit-gwtui/src/main/java/net/codemirror/lib/CodeMirror.java

@@ -166,14 +166,6 @@ public class CodeMirror extends JavaScriptObject {
     this.state.oldViewportSize = lines;
   }-*/;
 
-  public final native double getScrollSetAt() /*-{
-    return this.state.scrollSetAt || 0;
-  }-*/;
-
-  public final native void setScrollSetAt(double when) /*-{
-    this.state.scrollSetAt = when;
-  }-*/;
-
   public final native void on(String event, Runnable thunk) /*-{
     this.on(event, $entry(function() {
       thunk.@java.lang.Runnable::run()();