From 5bf4addf76d8fe7bff67401116acd03783bbd33f Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Fri, 11 Oct 2013 20:00:23 -0700 Subject: [PATCH] SideBySide2: Synchronize horizontal scrolling Disable CM3 line wrapping and rely on synchronized horizontal scrolling instead. This fixes alignment problems caused by long lines that exist only one of two sides. When line wrapping, the view did not pad the other side to account for the extra line height used by the wrapped line, skewing everything after it. Since horizontal scrolling is now functional change the widths of the views to be related to the width of the browser window. Keep a maximum width to prevent the a large gap between the sides on very wide monitors. Change-Id: I3a3b10fefdacd7246e4856365cb91d934f0346e2 --- .../google/gerrit/client/diff/DiffTable.ui.xml | 16 ++++++++++------ .../com/google/gerrit/client/diff/Header.ui.xml | 5 ++--- .../gerrit/client/diff/ScrollSynchronizer.java | 8 ++++---- .../google/gerrit/client/diff/SideBySide2.java | 17 +++++++++++------ 4 files changed, 27 insertions(+), 19 deletions(-) diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.ui.xml b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.ui.xml index 4c8579c7ed..18d497ce3d 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.ui.xml +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.ui.xml @@ -22,13 +22,15 @@ limitations under the License. @external .CodeMirror-linenumber, .CodeMirror-vscrollbar .CodeMirror-scroll; @external .cm-keymap-fat-cursor, CodeMirror-cursor; @external .cm-searching, .cm-trailingspace, .cm-tab; - .difftable .CodeMirror-lines { - padding: 0; - } + + .difftable { max-width: 1484px; } + .difftable .CodeMirror-lines { padding: 0; } .difftable .CodeMirror pre { padding: 0; padding-bottom: 0.11em; overflow: hidden; + border-right: 0; + width: auto; } .difftable .CodeMirror pre span { padding-bottom: 0.11em; @@ -41,13 +43,15 @@ limitations under the License. width: 10px; } .table { + width: 100%; table-layout: fixed; border-spacing: 0; } - .a, .b { padding: 0; } - .a { width: 702px; min-width: 702px; } - .b { width: 718px; min-width: 718px; } + .a, .b { + padding: 0; + width: 50%; + } /* Hide left side scrollbar, right side controls both views. */ .a .CodeMirror-scroll { padding-right: 0; } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/Header.ui.xml b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/Header.ui.xml index 53402fc086..2814a6312d 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/Header.ui.xml +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/Header.ui.xml @@ -21,8 +21,7 @@ limitations under the License. .header { position: relative; - width: 1420px; - min-width: 1420px; + max-width: 1484px; } .reviewed input { margin: 0; @@ -34,7 +33,7 @@ limitations under the License. .navigation { position: absolute; top: 0; - right: 0; + right: 15px; font-family: Arial Unicode MS, sans-serif; } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/ScrollSynchronizer.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/ScrollSynchronizer.java index bff8990c3c..f5ae14980b 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/ScrollSynchronizer.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/ScrollSynchronizer.java @@ -37,8 +37,7 @@ class ScrollSynchronizer { cmB.on("scroll", new ScrollCallback(cmB, cmA, DisplaySide.B)); } - private void updateScreenHeader(CodeMirror cm) { - ScrollInfo si = cm.getScrollInfo(); + private void updateScreenHeader(ScrollInfo si) { if (si.getTop() == 0 && !Gerrit.isHeaderVisible()) { Gerrit.setHeaderVisible(true); diffTable.updateFileCommentVisibility(false); @@ -77,8 +76,9 @@ class ScrollSynchronizer { fixup.scheduleRepeating(20); } if (active == this) { - updateScreenHeader(src); - dst.scrollToY(src.getScrollInfo().getTop()); + ScrollInfo si = src.getScrollInfo(); + updateScreenHeader(si); + dst.scrollTo(si.getLeft(), si.getTop()); state = 0; } } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySide2.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySide2.java index 505b6443e0..80660fa99d 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySide2.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySide2.java @@ -159,8 +159,8 @@ public class SideBySide2 extends Screen { // TODO: Re-implement necessary GlobalKey bindings. addDomHandler(GlobalKey.STOP_PROPAGATION, KeyPressEvent.getType()); keysNavigation = new KeyCommandSet(Gerrit.C.sectionNavigation()); - add(header = new Header(keysNavigation, base, revision, path)); - add(diffTable = new DiffTable(this, base, revision, path)); + header = new Header(keysNavigation, base, revision, path); + diffTable = new DiffTable(this, base, revision, path); add(uiBinder.createAndBindUi(this)); } @@ -248,8 +248,8 @@ public class SideBySide2 extends Screen { } })); resizeCodeMirror(); - Window.enableScrolling(false); + cmA.setOption("viewportMargin", 10); cmB.setOption("viewportMargin", 10); cmB.setCursor(LineCharacter.create(0)); @@ -267,8 +267,13 @@ public class SideBySide2 extends Screen { resizeHandler.removeHandler(); resizeHandler = null; } - cmA.getWrapperElement().removeFromParent(); - cmB.getWrapperElement().removeFromParent(); + if (cmA != null) { + cmA.getWrapperElement().removeFromParent(); + } + if (cmB != null) { + cmB.getWrapperElement().removeFromParent(); + } + Window.enableScrolling(true); Gerrit.setHeaderVisible(true); } @@ -463,7 +468,7 @@ public class SideBySide2 extends Screen { .set("lineNumbers", true) .set("tabSize", pref.getTabSize()) .set("mode", getContentType(meta)) - .set("lineWrapping", true) + .set("lineWrapping", false) .set("styleSelectedText", true) .set("showTrailingSpace", pref.isShowWhitespaceErrors()) .set("keyMap", "vim_ro")