From 44739b11679d59e55f69b9f4fd82260d3414d9cc Mon Sep 17 00:00:00 2001 From: Michael Zhou Date: Tue, 9 Jul 2013 11:15:53 -0700 Subject: [PATCH] Refactor padding for comment boxes Added PaddingManager that takes care of calculating and adjusting paddings introduced by CommentBoxes. Change-Id: Iff58822edeeb724f649a347197ec0beec15c5a82 --- .../gerrit/client/diff/CodeMirrorDemo.java | 134 ++++++++++------- .../google/gerrit/client/diff/CommentBox.java | 66 ++++----- .../google/gerrit/client/diff/DraftBox.java | 6 +- .../gerrit/client/diff/PaddingManager.java | 138 ++++++++++++++++++ .../java/net/codemirror/lib/CodeMirror.java | 9 ++ 5 files changed, 268 insertions(+), 85 deletions(-) create mode 100644 gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/PaddingManager.java diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CodeMirrorDemo.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CodeMirrorDemo.java index 28d54d36e0..4bd43f3445 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CodeMirrorDemo.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CodeMirrorDemo.java @@ -19,6 +19,7 @@ import com.google.gerrit.client.changes.ChangeApi; import com.google.gerrit.client.changes.ChangeInfo; import com.google.gerrit.client.changes.CommentApi; import com.google.gerrit.client.changes.CommentInfo; +import com.google.gerrit.client.diff.PaddingManager.LineWidgetElementPair; import com.google.gerrit.client.diff.DiffInfo.Region; import com.google.gerrit.client.diff.DiffInfo.Span; import com.google.gerrit.client.diff.LineMapper.LineOnOtherInfo; @@ -46,6 +47,7 @@ import com.google.gwt.user.client.rpc.AsyncCallback; import net.codemirror.lib.CodeMirror; import net.codemirror.lib.CodeMirror.LineClassWhere; +import net.codemirror.lib.CodeMirror.LineHandle; import net.codemirror.lib.Configuration; import net.codemirror.lib.KeyMap; import net.codemirror.lib.LineCharacter; @@ -53,6 +55,8 @@ import net.codemirror.lib.LineWidget; import net.codemirror.lib.ModeInjector; import java.util.ArrayList; +import java.util.Collections; +import java.util.Comparator; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -61,13 +65,6 @@ public class CodeMirrorDemo extends Screen { private static final int HEADER_FOOTER = 60 + 15 * 2 + 38; private static final JsArrayString EMPTY = JavaScriptObject.createArray().cast(); - /** - * TODO: Handle ordering of line widgets. CodeMirror seems to have a bug when - * multiple line widgets are added to a line: it puts the first widget above - * the line when the line is clicked upon. - */ - private static final Configuration COMMENT_BOX_CONFIG = - Configuration.create().set("coverGutter", true); private final PatchSet.Id base; private final PatchSet.Id revision; @@ -84,8 +81,9 @@ public class CodeMirrorDemo extends Screen { private LineMapper mapper; private CommentLinkProcessor commentLinkProcessor; private Map publishedMap; - private Map lineBoxMapA; - private Map lineBoxMapB; + private Map lineActiveBoxMap; + private Map lineLastPublishedBoxMap; + private Map linePaddingManagerMap; public CodeMirrorDemo( PatchSet.Id base, @@ -201,13 +199,16 @@ public class CodeMirrorDemo extends Screen { cmB = displaySide(diffInfo.meta_b(), diffInfo.text_b(), diffTable.getCmB()); render(diffInfo); initialBoxes = new ArrayList(); + lineActiveBoxMap = new HashMap(); + lineLastPublishedBoxMap = new HashMap(); + linePaddingManagerMap = new HashMap(); if (published != null) { publishedMap = new HashMap(published.length()); + renderPublished(); + } + if (drafts != null) { + renderDrafts(); } - lineBoxMapA = new HashMap(); - lineBoxMapB = new HashMap(); - renderPublished(); - renderDrafts(); published = null; drafts = null; cmA.on("cursorActivity", updateActiveLine(cmA)); @@ -317,7 +318,8 @@ public class CodeMirrorDemo extends Screen { if (!doSave) { box.setEdit(true); } - getLineBoxMapFromSide(info.side()).put(info.line() - 1, box); + LineHandle handle = getCmFromSide(info.side()).getLineHandle(info.line() - 1); + lineActiveBoxMap.put(handle, box); return box; } @@ -327,42 +329,82 @@ public class CodeMirrorDemo extends Screen { CodeMirror cm = mySide == Side.PARENT ? cmA : cmB; CodeMirror other = otherCM(cm); int line = info.line() - 1; // CommentInfo is 1-based, but CM is 0-based - LineWidget boxWidget = - cm.addLineWidget(line, box.getElement(), COMMENT_BOX_CONFIG); + LineHandle handle = cm.getLineHandle(line); + PaddingManager manager; + if (linePaddingManagerMap.containsKey(handle)) { + manager = linePaddingManagerMap.get(handle); + } else { + // Estimated height at 21px, fixed by deferring after display + manager = new PaddingManager( + addPaddingWidget(cm, diffTable.style.padding(), line, 21, Unit.PX, 0)); + linePaddingManagerMap.put(handle, manager); + } int lineToPad = mapper.lineOnOther(mySide, line).getLine(); - // Estimated height at 21px, fixed by deferring after display - LineWidgetElementPair padding = addPaddingWidget( - other, diffTable.style.padding(), lineToPad, 21, Unit.PX); + LineHandle otherHandle = other.getLineHandle(lineToPad); + if (linePaddingManagerMap.containsKey(otherHandle)) { + PaddingManager.link(manager, linePaddingManagerMap.get(otherHandle)); + } else { + PaddingManager otherManager = new PaddingManager( + addPaddingWidget(other, diffTable.style.padding(), lineToPad, 21, Unit.PX, 0)); + linePaddingManagerMap.put(otherHandle, otherManager); + PaddingManager.link(manager, otherManager); + } + int index = manager.getCurrentCount(); + manager.insert(box, index); + Configuration config = Configuration.create() + .set("coverGutter", true) + .set("insertAt", index); + LineWidget boxWidget = cm.addLineWidget(line, box.getElement(), config); + box.setPaddingManager(manager); box.setSelfWidget(boxWidget); - box.setPadding(padding.widget, padding.element); return box; } - void removeCommentBox(Side side, int line) { - getLineBoxMapFromSide(side).remove(line); + void removeDraft(Side side, int line) { + LineHandle handle = getCmFromSide(side).getLineHandle(line); + lineActiveBoxMap.remove(handle); + if (lineLastPublishedBoxMap.containsKey(handle)) { + lineActiveBoxMap.put(handle, lineLastPublishedBoxMap.get(handle)); + } + } + + private List sortComment(JsArray unsorted) { + List sorted = new ArrayList(); + for (int i = 0; i < unsorted.length(); i++) { + sorted.add(unsorted.get(i)); + } + Collections.sort(sorted, new Comparator() { + @Override + public int compare(CommentInfo o1, CommentInfo o2) { + return o1.updated().compareTo(o2.updated()); + } + }); + return sorted; } private void renderPublished() { - for (int i = 0; published != null && i < published.length(); i++) { - CommentInfo info = published.get(i); + List sorted = sortComment(published); + for (CommentInfo info : sorted) { final PublishedBox box = new PublishedBox(this, revision, info, commentLinkProcessor); box.setOpen(false); - addCommentBox(info, box); initialBoxes.add(box); publishedMap.put(info.id(), box); - getLineBoxMapFromSide(info.side()).put(info.line() - 1, box); + int line = info.line() - 1; + LineHandle handle = getCmFromSide(info.side()).getLineHandle(line); + lineLastPublishedBoxMap.put(handle, box); + lineActiveBoxMap.put(handle, box); + addCommentBox(info, box); } } private void renderDrafts() { - for (int i = 0; drafts != null && i < drafts.length(); i++) { - CommentInfo info = drafts.get(i); + List sorted = sortComment(drafts); + for (CommentInfo info : sorted) { final DraftBox box = new DraftBox(this, revision, info, commentLinkProcessor, false, false); box.setOpen(false); box.setEdit(false); - addCommentBox(info, box); initialBoxes.add(box); if (published != null) { PublishedBox replyToBox = publishedMap.get(info.in_reply_to()); @@ -370,7 +412,9 @@ public class CodeMirrorDemo extends Screen { replyToBox.registerReplyBox(box); } } - getLineBoxMapFromSide(info.side()).put(info.line() - 1, box); + lineActiveBoxMap.put( + getCmFromSide(info.side()).getLineHandle(info.line() - 1), box); + addCommentBox(info, box); } } @@ -378,8 +422,8 @@ public class CodeMirrorDemo extends Screen { return me == cmA ? cmB : cmA; } - private Map getLineBoxMapFromSide(Side side) { - return side == Side.PARENT ? lineBoxMapA : lineBoxMapB; + private CodeMirror getCmFromSide(Side side) { + return side == Side.PARENT ? cmA : cmB; } private void markEdit(CodeMirror cm, JsArrayString lines, @@ -423,17 +467,20 @@ public class CodeMirrorDemo extends Screen { private void insertEmptyLines(CodeMirror cm, int nextLine, int cnt) { // -1 to compensate for the line we went past when this method is called. addPaddingWidget(cm, diffTable.style.padding(), nextLine - 1, - cnt, Unit.EM); + cnt, Unit.EM, null); } private LineWidgetElementPair addPaddingWidget(CodeMirror cm, String style, - int line, int height, Unit unit) { + int line, int height, Unit unit, Integer index) { Element div = DOM.createDiv(); div.setClassName(style); div.getStyle().setHeight(height, unit); Configuration config = Configuration.create() .set("coverGutter", true) .set("above", line == -1); + if (index != null) { + config = config.set("insertAt", index); + } LineWidget widget = cm.addLineWidget(line == -1 ? 0 : line, div, config); return new LineWidgetElementPair(widget, div); } @@ -484,14 +531,13 @@ public class CodeMirrorDemo extends Screen { private Runnable insertNewDraft(final CodeMirror cm) { return new Runnable() { public void run() { - Map lineBoxMap = cm == cmA ? - lineBoxMapA : lineBoxMapB; int line = cm.getActiveLine(); - CommentBox box = lineBoxMap.get(line); + LineHandle handle = cm.getLineHandle(line); + CommentBox box = lineActiveBoxMap.get(handle); if (box == null) { - lineBoxMap.put(line, addNewDraft(cm, line)); + lineActiveBoxMap.put(handle, addNewDraft(cm, line)); } else if (box.isDraft()) { - ((DraftBox) lineBoxMap.get(line)).setEdit(true); + ((DraftBox) lineActiveBoxMap.get(handle)).setEdit(true); } else { ((PublishedBox) box).onReply(null); } @@ -505,16 +551,6 @@ public class CodeMirrorDemo extends Screen { : null; } - private static class LineWidgetElementPair { - private LineWidget widget; - private Element element; - - private LineWidgetElementPair(LineWidget w, Element e) { - widget = w; - element = e; - } - } - static class EditIterator { private final JsArrayString lines; private final int startLine; diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentBox.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentBox.java index 3ba3a0f937..712711cf41 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentBox.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentBox.java @@ -19,8 +19,6 @@ import com.google.gerrit.client.ui.CommentLinkProcessor; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gwt.core.client.Scheduler; import com.google.gwt.core.client.Scheduler.ScheduledCommand; -import com.google.gwt.dom.client.Element; -import com.google.gwt.dom.client.Style.Unit; import com.google.gwt.event.dom.client.ClickEvent; import com.google.gwt.event.dom.client.ClickHandler; import com.google.gwt.event.shared.HandlerRegistration; @@ -48,11 +46,10 @@ abstract class CommentBox extends Composite { private HandlerRegistration headerClick; private CommentInfo original; private PatchSet.Id patchSetId; - private LineWidget selfWidget; - private LineWidget paddingWidget; - private Element paddingWidgetEle; + private PaddingManager widgetManager; private CodeMirrorDemo diffView; private boolean draft; + private LineWidget selfWidget; @UiField(provided=true) CommentBoxHeader header; @@ -63,7 +60,7 @@ abstract class CommentBox extends Composite { @UiField CommentBoxResources res; - protected CommentBox( + CommentBox( CodeMirrorDemo host, UiBinder binder, PatchSet.Id id, CommentInfo info, CommentLinkProcessor linkProcessor, @@ -101,27 +98,20 @@ abstract class CommentBox extends Composite { } } - void setSelfWidget(LineWidget widget) { - selfWidget = widget; - } - - void setPadding(LineWidget widget, Element element) { - paddingWidget = widget; - paddingWidgetEle = element; - } - void resizePaddingWidget() { - Scheduler.get().scheduleDeferred(new ScheduledCommand() { - @Override + Scheduler.get().scheduleDeferred(new ScheduledCommand(){ public void execute() { - paddingWidgetEle.getStyle().setHeight(getOffsetHeight(), Unit.PX); - paddingWidget.changed(); + if (selfWidget == null || widgetManager == null) { + throw new IllegalStateException( + "resizePaddingWidget() called before setting up widgets"); + } selfWidget.changed(); + widgetManager.resizePaddingWidget(); } }); } - protected void setMessageText(String message) { + void setMessageText(String message) { if (message == null) { message = ""; } else { @@ -133,11 +123,11 @@ abstract class CommentBox extends Composite { SafeHtml.set(contentPanelMessage, buf); } - protected void setDate(Timestamp when) { + void setDate(Timestamp when) { header.setDate(when); } - protected void setOpen(boolean open) { + void setOpen(boolean open) { if (open) { removeStyleName(res.style().close()); addStyleName(res.style().open()); @@ -152,31 +142,39 @@ abstract class CommentBox extends Composite { return getStyleName().contains(res.style().open()); } - protected CodeMirrorDemo getDiffView() { + CodeMirrorDemo getDiffView() { return diffView; } - protected PatchSet.Id getPatchSetId() { + PatchSet.Id getPatchSetId() { return patchSetId; } - protected CommentInfo getOriginal() { + CommentInfo getOriginal() { return original; } - protected LineWidget getSelfWidget() { - return selfWidget; - } - - protected LineWidget getPaddingWidget() { - return paddingWidget; - } - - protected void updateOriginal(CommentInfo newInfo) { + void updateOriginal(CommentInfo newInfo) { original = newInfo; } + PaddingManager getPaddingManager() { + return widgetManager; + } + boolean isDraft() { return draft; } + + void setPaddingManager(PaddingManager manager) { + widgetManager = manager; + } + + void setSelfWidget(LineWidget widget) { + selfWidget = widget; + } + + LineWidget getSelfWidget() { + return selfWidget; + } } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DraftBox.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DraftBox.java index c4906de2cb..5ec8949de6 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DraftBox.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DraftBox.java @@ -139,10 +139,12 @@ class DraftBox extends CommentBox { replyToBox.unregisterReplyBox(); } CommentInfo info = getOriginal(); - getDiffView().removeCommentBox(info.side(), info.line() - 1); + getDiffView().removeDraft(info.side(), info.line() - 1); removeFromParent(); getSelfWidget().clear(); - getPaddingWidget().clear(); + PaddingManager manager = getPaddingManager(); + manager.remove(this); + manager.resizePaddingWidget(); } @UiHandler("edit") diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/PaddingManager.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/PaddingManager.java new file mode 100644 index 0000000000..94a16602d7 --- /dev/null +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/PaddingManager.java @@ -0,0 +1,138 @@ +// 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.gwt.dom.client.Element; +import com.google.gwt.dom.client.Style.Unit; + +import net.codemirror.lib.LineWidget; + +import java.util.ArrayList; +import java.util.List; + +/** + * Manages paddings for CommentBoxes. Each line that may need to be padded owns + * a PaddingManager instance, which maintains a padding widget whose height + * changes as necessary. PaddingManager calculates padding by taking the + * difference of the sum of CommentBox heights on the two sides. + * + * Note that in the case of an insertion or deletion gap, A PaddingManager + * can map to a list of managers on the other side. The padding needed is then + * calculated from the sum of all their heights. + * + * TODO: Let PaddingManager also take care of the paddings introduced by + * insertions and deletions. + */ +class PaddingManager { + private List comments; + private LineWidgetElementPair padding; + private List others; + + PaddingManager(LineWidgetElementPair padding) { + comments = new ArrayList(); + others = new ArrayList(); + this.padding = padding; + } + + static void link(PaddingManager a, PaddingManager b) { + if (!a.others.contains(b)) { + a.others.add(b); + } + if (!b.others.contains(a)) { + b.others.add(a); + } + } + + private int getMyTotalHeight() { + int total = 0; + for (CommentBox box : comments) { + total += box.getOffsetHeight(); + } + return total; + } + + /** + * If this instance is on the insertion side, its counterpart on the other + * side will map to a group of PaddingManagers on this side, so we calculate + * the group's total height instead of an individual one's. + */ + private int getGroupTotalHeight() { + if (others.size() > 1) { + return getMyTotalHeight(); + } else { + return others.get(0).getOthersTotalHeight(); + } + } + + private int getOthersTotalHeight() { + int total = 0; + for (PaddingManager manager : others) { + total += manager.getMyTotalHeight(); + } + return total; + } + + private void setPaddingHeight(int height) { + padding.element.getStyle().setHeight(height, Unit.PX); + padding.widget.changed(); + } + + void resizePaddingWidget() { + if (others.isEmpty()) { + throw new IllegalStateException("resizePaddingWidget() called before linking"); + } + int myHeight = getGroupTotalHeight(); + int othersHeight = getOthersTotalHeight(); + int paddingNeeded = othersHeight - myHeight; + if (paddingNeeded < 0) { + for (PaddingManager manager : others.get(0).others) { + manager.setPaddingHeight(0); + } + others.get(others.size() - 1).setPaddingHeight(-paddingNeeded); + } else { + setPaddingHeight(paddingNeeded); + for (PaddingManager other : others) { + other.setPaddingHeight(0); + } + } + } + + /** This is unused now because threading info is ignored. */ + int getReplyIndex(CommentBox box) { + return comments.indexOf(box) + 1; + } + + int getCurrentCount() { + return comments.size(); + } + + void insert(CommentBox box, int index) { + comments.add(index, box); + } + + void remove(CommentBox box) { + comments.remove(box); + } + + static class LineWidgetElementPair { + private LineWidget widget; + private Element element; + + LineWidgetElementPair(LineWidget w, Element e) { + widget = w; + element = e; + } + } +} diff --git a/gerrit-gwtui/src/main/java/net/codemirror/lib/CodeMirror.java b/gerrit-gwtui/src/main/java/net/codemirror/lib/CodeMirror.java index 69d1671d91..c516a1fcbd 100644 --- a/gerrit-gwtui/src/main/java/net/codemirror/lib/CodeMirror.java +++ b/gerrit-gwtui/src/main/java/net/codemirror/lib/CodeMirror.java @@ -130,6 +130,15 @@ public class CodeMirror extends JavaScriptObject { public final native void addKeyMap(KeyMap map) /*-{ this.addKeyMap(map); }-*/; + public final native LineHandle getLineHandle(int line) /*-{ + return this.getLineHandle(line); + }-*/; + protected CodeMirror() { } + + public static class LineHandle extends JavaScriptObject { + protected LineHandle(){ + } + } }