Enable adding draft on active line

Modified CodeMirrorDemo to highlight active line.

Added KeyMap from 'c' to a handler that adds drafts on active line,
edits existing drafts, or replies to published comments.

Modified lineOnOther() in LineMapper to return both the line number
and whether the line on the other side is aligned with the one on
this side. Added tests to verify.

Change-Id: I732ded8d76163a6b4dc76fcd45dbd9233090ce9c
This commit is contained in:
Michael Zhou
2013-06-28 16:27:18 -07:00
committed by Shawn Pearce
parent bcb6b6f149
commit 3227f3d80c
11 changed files with 273 additions and 50 deletions

View File

@@ -20,6 +20,7 @@ import com.google.gerrit.client.changes.CommentApi;
import com.google.gerrit.client.changes.CommentInfo;
import com.google.gerrit.client.diff.DiffInfo.Region;
import com.google.gerrit.client.diff.DiffInfo.Span;
import com.google.gerrit.client.diff.LineMapper.LineOnOtherInfo;
import com.google.gerrit.client.projects.ConfigInfoCache;
import com.google.gerrit.client.rpc.CallbackGroup;
import com.google.gerrit.client.rpc.GerritCallback;
@@ -33,8 +34,6 @@ import com.google.gerrit.reviewdb.client.Project;
import com.google.gwt.core.client.JavaScriptObject;
import com.google.gwt.core.client.JsArray;
import com.google.gwt.core.client.JsArrayString;
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.logical.shared.ResizeEvent;
@@ -47,6 +46,7 @@ import com.google.gwt.user.client.rpc.AsyncCallback;
import net.codemirror.lib.CodeMirror;
import net.codemirror.lib.CodeMirror.LineClassWhere;
import net.codemirror.lib.Configuration;
import net.codemirror.lib.KeyMap;
import net.codemirror.lib.LineCharacter;
import net.codemirror.lib.LineWidget;
import net.codemirror.lib.ModeInjector;
@@ -60,6 +60,11 @@ 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);
@@ -78,6 +83,8 @@ public class CodeMirrorDemo extends Screen {
private LineMapper mapper;
private CommentLinkProcessor commentLinkProcessor;
private Map<String, PublishedBox> publishedMap;
private Map<Integer, CommentBox> lineBoxMapA;
private Map<Integer, CommentBox> lineBoxMapB;
public CodeMirrorDemo(
PatchSet.Id base,
@@ -160,14 +167,9 @@ public class CodeMirrorDemo extends Screen {
cmB.refresh();
}
Window.enableScrolling(false);
Scheduler.get().scheduleDeferred(new ScheduledCommand() {
@Override
public void execute() {
for (CommentBox box : initialBoxes) {
box.resizePaddingWidget();
}
}
});
for (CommentBox box : initialBoxes) {
box.resizePaddingWidget();
}
}
@Override
@@ -187,9 +189,6 @@ public class CodeMirrorDemo extends Screen {
cmB = null;
}
Window.enableScrolling(true);
mapper = null;
initialBoxes = null;
publishedMap = null;
}
private void display(DiffInfo diffInfo) {
@@ -198,11 +197,16 @@ public class CodeMirrorDemo extends Screen {
render(diffInfo);
initialBoxes = new ArrayList<CommentBox>();
publishedMap = new HashMap<String, PublishedBox>(published.length());
lineBoxMapA = new HashMap<Integer, CommentBox>();
lineBoxMapB = new HashMap<Integer, CommentBox>();
renderPublished();
renderDrafts();
published = null;
drafts = null;
cmA.on("cursorActivity", updateActiveLine(cmA));
cmB.on("cursorActivity", updateActiveLine(cmB));
cmA.addKeyMap(KeyMap.create().on("'c'", insertNewDraft(cmA)));
cmB.addKeyMap(KeyMap.create().on("'c'", insertNewDraft(cmB)));
// TODO: Probably need horizontal resize
resizeHandler = Window.addResizeHandler(new ResizeHandler() {
@Override
@@ -271,16 +275,37 @@ public class CodeMirrorDemo extends Screen {
}
}
DraftBox addReplyBox(CommentInfo replyTo, String initMessage, boolean doSave) {
private DraftBox addNewDraft(CodeMirror cm, int line) {
Side side = cm == cmA ? Side.PARENT : Side.REVISION;
CommentInfo info = CommentInfo.create(
path,
replyTo.side(),
replyTo.line(),
side,
line + 1,
null,
null);
return addDraftBox(info, false);
}
DraftBox addReply(CommentInfo replyTo, String initMessage, boolean doSave) {
Side side = replyTo.side();
int line = replyTo.line();
CommentInfo info = CommentInfo.create(
path,
side,
line,
replyTo.id(),
initMessage);
return addDraftBox(info, doSave);
}
private DraftBox addDraftBox(CommentInfo info, boolean doSave) {
DraftBox box = new DraftBox(this, revision, info, commentLinkProcessor,
true, !doSave);
true, doSave);
addCommentBox(info, box);
if (!doSave) {
box.setEdit(true);
}
getLineBoxMapFromSide(info.side()).put(info.line() - 1, box);
return box;
}
@@ -292,7 +317,7 @@ public class CodeMirrorDemo extends Screen {
int line = info.line() - 1; // CommentInfo is 1-based, but CM is 0-based
LineWidget boxWidget =
cm.addLineWidget(line, box.getElement(), COMMENT_BOX_CONFIG);
int lineToPad = mapper.lineOnOther(mySide, line);
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);
@@ -301,14 +326,20 @@ public class CodeMirrorDemo extends Screen {
return box;
}
void removeCommentBox(Side side, int line) {
getLineBoxMapFromSide(side).remove(line);
}
private void renderPublished() {
for (int i = 0; published != null && i < published.length(); i++) {
CommentInfo info = published.get(i);
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);
}
}
@@ -317,13 +348,15 @@ public class CodeMirrorDemo extends Screen {
CommentInfo info = drafts.get(i);
final DraftBox box =
new DraftBox(this, revision, info, commentLinkProcessor, false, false);
box.setOpen(false);
box.setEdit(false);
addCommentBox(info, box);
initialBoxes.add(box);
PublishedBox replyToBox = publishedMap.get(info.in_reply_to());
if (replyToBox != null) {
replyToBox.registerReplyBox(box);
box.registerReplyToBox(replyToBox);
}
getLineBoxMapFromSide(info.side()).put(info.line() - 1, box);
}
}
@@ -331,6 +364,10 @@ public class CodeMirrorDemo extends Screen {
return me == cmA ? cmB : cmA;
}
private Map<Integer, CommentBox> getLineBoxMapFromSide(Side side) {
return side == Side.PARENT ? lineBoxMapA : lineBoxMapB;
}
private void markEdit(CodeMirror cm, JsArrayString lines,
JsArray<Span> edits, int startLine) {
if (edits == null) {
@@ -396,6 +433,58 @@ public class CodeMirrorDemo extends Screen {
};
}
private Runnable updateActiveLine(final CodeMirror cm) {
final CodeMirror other = otherCM(cm);
return new Runnable() {
public void run() {
if (cm.hasActiveLine()) {
cm.removeLineClass(cm.getActiveLine(),
LineClassWhere.WRAP, diffTable.style.activeLine());
cm.removeLineClass(cm.getActiveLine(),
LineClassWhere.BACKGROUND, diffTable.style.activeLineBg());
}
if (other.hasActiveLine()) {
other.removeLineClass(other.getActiveLine(),
LineClassWhere.WRAP, diffTable.style.activeLine());
other.removeLineClass(other.getActiveLine(),
LineClassWhere.BACKGROUND, diffTable.style.activeLineBg());
}
int line = cm.getCursor("head").getLine();
LineOnOtherInfo info =
mapper.lineOnOther(cm == cmA ? Side.PARENT : Side.REVISION, line);
int oLine = info.getLine();
cm.setActiveLine(line);
cm.addLineClass(line, LineClassWhere.WRAP, diffTable.style.activeLine());
cm.addLineClass(line, LineClassWhere.BACKGROUND, diffTable.style.activeLineBg());
if (info.isAligned()) {
other.setActiveLine(oLine);
other.addLineClass(oLine, LineClassWhere.WRAP,
diffTable.style.activeLine());
other.addLineClass(oLine, LineClassWhere.BACKGROUND,
diffTable.style.activeLineBg());
}
}
};
}
private Runnable insertNewDraft(final CodeMirror cm) {
return new Runnable() {
public void run() {
Map<Integer, CommentBox> lineBoxMap = cm == cmA ?
lineBoxMapA : lineBoxMapB;
int line = cm.getActiveLine();
CommentBox box = lineBoxMap.get(line);
if (box == null) {
lineBoxMap.put(line, addNewDraft(cm, line));
} else if (box.isDraft()) {
((DraftBox) lineBoxMap.get(line)).setEdit(true);
} else {
((PublishedBox) box).onReply(null);
}
}
};
}
private static String getContentType(DiffInfo.FileMeta meta) {
return meta != null && meta.content_type() != null
? ModeInjector.getContentType(meta.content_type())

View File

@@ -17,6 +17,8 @@ package com.google.gerrit.client.diff;
import com.google.gerrit.client.changes.CommentInfo;
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;
@@ -50,6 +52,7 @@ abstract class CommentBox extends Composite {
private LineWidget paddingWidget;
private Element paddingWidgetEle;
private CodeMirrorDemo diffView;
private boolean draft;
@UiField(provided=true)
CommentBoxHeader header;
@@ -69,10 +72,10 @@ abstract class CommentBox extends Composite {
commentLinkProcessor = linkProcessor;
original = info;
patchSetId = id;
draft = isDraft;
header = new CommentBoxHeader(info.author(), info.updated(), isDraft);
initWidget(binder.createAndBindUi(this));
setMessageText(info.message());
setOpen(false);
}
@Override
@@ -83,7 +86,6 @@ abstract class CommentBox extends Composite {
@Override
public void onClick(ClickEvent event) {
setOpen(!isOpen());
resizePaddingWidget();
}
}, ClickEvent.getType());
res.style().ensureInjected();
@@ -109,7 +111,14 @@ abstract class CommentBox extends Composite {
}
void resizePaddingWidget() {
paddingWidgetEle.getStyle().setHeight(getOffsetHeight(), Unit.PX);
Scheduler.get().scheduleDeferred(new ScheduledCommand() {
@Override
public void execute() {
paddingWidgetEle.getStyle().setHeight(getOffsetHeight(), Unit.PX);
paddingWidget.changed();
selfWidget.changed();
}
});
}
protected void setMessageText(String message) {
@@ -136,6 +145,7 @@ abstract class CommentBox extends Composite {
removeStyleName(res.style().open());
addStyleName(res.style().close());
}
resizePaddingWidget();
}
private boolean isOpen() {
@@ -165,4 +175,8 @@ abstract class CommentBox extends Composite {
protected void updateOriginal(CommentInfo newInfo) {
original = newInfo;
}
boolean isDraft() {
return draft;
}
}

View File

@@ -36,7 +36,7 @@ class CommentBoxHeader extends Composite {
interface Binder extends UiBinder<HTMLPanel, CommentBoxHeader> {}
private static Binder uiBinder = GWT.create(Binder.class);
private boolean isDraft;
private boolean draft;
@UiField(provided=true)
AvatarImage avatar;
@@ -52,9 +52,9 @@ class CommentBoxHeader extends Composite {
CommentBoxHeader(AccountInfo author, Timestamp when, boolean isDraft) {
// TODO: Set avatar's display to none if we get a 404.
avatar = new AvatarImage(author, 26);
avatar = author == null ? new AvatarImage() : new AvatarImage(author, 26);
initWidget(uiBinder.createAndBindUi(this));
this.isDraft = isDraft;
this.draft = isDraft;
if (when != null) {
setDate(when);
}
@@ -66,7 +66,7 @@ class CommentBoxHeader extends Composite {
}
void setDate(Timestamp when) {
if (isDraft) {
if (draft) {
date.setInnerText(PatchUtil.M.draftSaved(when));
} else {
date.setInnerText(FormatUtil.shortFormatDayTime(when));

View File

@@ -35,6 +35,8 @@ class DiffTable extends Composite {
String diff();
String intraline();
String padding();
String activeLine();
String activeLineBg();
}
@UiField

View File

@@ -57,6 +57,12 @@ limitations under the License.
.a .CodeMirror-vscrollbar {
display: none !important;
}
.activeLine .CodeMirror-linenumber {
background-color: #D8EDF9;
}
.activeLineBg {
background-color: #D8EDF9;
}
</ui:style>
<g:HTMLPanel styleName='{style.difftable}'>
<table class='{style.table}'>

View File

@@ -81,8 +81,6 @@ class DraftBox extends CommentBox {
this.isNew = isNew;
editArea.setText(contentPanelMessage.getText());
setEdit(isNew && !saveOnInit);
setOpen(isNew && !saveOnInit);
if (saveOnInit) {
onSave(null);
}
@@ -120,6 +118,7 @@ class DraftBox extends CommentBox {
void setEdit(boolean edit) {
if (edit) {
setOpen(true);
removeStyleName(draftStyle.view());
addStyleName(draftStyle.edit());
editArea.setText(contentPanelMessage.getText());
@@ -139,9 +138,11 @@ class DraftBox extends CommentBox {
if (replyToBox != null) {
replyToBox.unregisterReplyBox();
}
getPaddingWidget().clear();
CommentInfo info = getOriginal();
getDiffView().removeCommentBox(info.side(), info.line() - 1);
removeFromParent();
getSelfWidget().clear();
getPaddingWidget().clear();
}
@UiHandler("edit")
@@ -162,10 +163,9 @@ class DraftBox extends CommentBox {
@Override
public void onSuccess(CommentInfo result) {
updateOriginal(result);
setEdit(false);
setMessageText(message);
setDate(result.updated());
resizePaddingWidget();
setEdit(false);
if (isNew) {
removeStyleName(draftStyle.newDraft());
isNew = false;
@@ -182,7 +182,6 @@ class DraftBox extends CommentBox {
@UiHandler("cancel")
void onCancel(ClickEvent e) {
setEdit(false);
resizePaddingWidget();
}
@UiHandler("discard")

View File

@@ -49,7 +49,7 @@ class LineMapper {
int origLineB = lineB;
lineB += numLines;
int bAheadOfA = lineB - lineA;
lineMapAtoB.add(new LineGap(lineA, lineA, bAheadOfA));
lineMapAtoB.add(new LineGap(lineA, -1, bAheadOfA));
lineMapBtoA.add(new LineGap(origLineB, lineB - 1, -bAheadOfA));
}
@@ -58,7 +58,7 @@ class LineMapper {
lineA += numLines;
int aAheadOfB = lineA - lineB;
lineMapAtoB.add(new LineGap(origLineA, lineA - 1, -aAheadOfB));
lineMapBtoA.add(new LineGap(lineB, lineB, aAheadOfB));
lineMapBtoA.add(new LineGap(lineB, -1, aAheadOfB));
}
/**
@@ -69,8 +69,9 @@ class LineMapper {
*
* A LineGap records gap information from the start of an actual gap up to
* the start of the next gap. In the following example,
* lineMapAtoB will have LineGap: {start: 1, end: 1, delta: 3}
* (end doesn't really matter here, as the binary search only looks at start)
* lineMapAtoB will have LineGap: {start: 1, end: -1, delta: 3}
* (end set to -1 to represent a dummy gap of length zero. The binary search
* only looks at start so setting it to -1 has no effect here.)
* lineMapBtoA will have LineGap: {start: 1, end: 3, delta: -3}
* These LineGaps control lines between 1 and 5.
*
@@ -97,24 +98,61 @@ class LineMapper {
* - | 6
* ...
*/
int lineOnOther(Side mySide, int line) {
LineOnOtherInfo lineOnOther(Side mySide, int line) {
List<LineGap> lineGaps = mySide == Side.PARENT ? lineMapAtoB : lineMapBtoA;
// Create a dummy LineGap for the search.
int ret = Collections.binarySearch(lineGaps, new LineGap(line));
if (ret == -1) {
return line;
return new LineOnOtherInfo(line, true);
} else {
LineGap lookup = lineGaps.get(0 <= ret ? ret : -ret - 2);
int start = lookup.start;
int end = lookup.end;
int delta = lookup.delta;
if (lookup.start <= line && line <= end) { // Line falls within gap
return end + delta;
if (start <= line && line <= end && end != -1) { // Line falls within gap
return new LineOnOtherInfo(end + delta, false);
} else { // Line after gap
return line + delta;
return new LineOnOtherInfo(line + delta, true);
}
}
}
/**
* @field line The line number on the other side.
* @field aligned Whether the two lines are at the same height when displayed.
*/
static class LineOnOtherInfo {
private int line;
private boolean aligned;
LineOnOtherInfo(int line, boolean aligned) {
this.line = line;
this.aligned = aligned;
}
int getLine() {
return line;
}
boolean isAligned() {
return aligned;
}
@Override
public boolean equals(Object obj) {
if (obj instanceof LineOnOtherInfo) {
LineOnOtherInfo other = (LineOnOtherInfo) obj;
return aligned == other.aligned && line == other.line;
}
return false;
}
@Override
public String toString() {
return line + " " + aligned;
}
}
/**
* Helper class to record line gap info and assist in calculation of line
* number on the other side.

View File

@@ -53,7 +53,7 @@ class PublishedBox extends CommentBox {
@UiHandler("reply")
void onReply(ClickEvent e) {
if (replyBox == null) {
DraftBox box = getDiffView().addReplyBox(getOriginal(), "", true);
DraftBox box = getDiffView().addReply(getOriginal(), "", false);
registerReplyBox(box);
} else {
openReplyBox();
@@ -63,7 +63,7 @@ class PublishedBox extends CommentBox {
@UiHandler("replyDone")
void onReplyDone(ClickEvent e) {
if (replyBox == null) {
DraftBox box = getDiffView().addReplyBox(getOriginal(), "Done", false);
DraftBox box = getDiffView().addReply(getOriginal(), "Done", true);
registerReplyBox(box);
} else {
openReplyBox();

View File

@@ -66,6 +66,16 @@ public class CodeMirror extends JavaScriptObject {
this.addLineClass(line, where, lineClass);
}-*/;
public final void removeLineClass(int line, LineClassWhere where,
String className) {
removeLineClassNative(line, where.name().toLowerCase(), className);
}
private final native void removeLineClassNative(int line, String where,
String lineClass) /*-{
this.removeLineClass(line, where, lineClass);
}-*/;
public final native void addWidget(LineCharacter pos, Element node,
boolean scrollIntoView) /*-{
this.addWidget(pos, node, scrollIntoView);
@@ -102,6 +112,24 @@ public class CodeMirror extends JavaScriptObject {
});
}-*/;
public final native LineCharacter getCursor(String start) /*-{
return this.getCursor(start);
}-*/;
public final native boolean hasActiveLine() /*-{
return this.state.hasOwnProperty('activeLine');
}-*/;
public final native int getActiveLine() /*-{
return this.state.activeLine;
}-*/;
public final native void setActiveLine(int line) /*-{
this.state.activeLine = line;
}-*/;
public final native void addKeyMap(KeyMap map) /*-{ this.addKeyMap(map); }-*/;
protected CodeMirror() {
}
}

View File

@@ -0,0 +1,32 @@
// 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 net.codemirror.lib;
import com.google.gwt.core.client.JavaScriptObject;
/** Object that associates a key or key combination with a handler. */
public class KeyMap extends JavaScriptObject {
public static KeyMap create() {
return createObject().cast();
}
public final native KeyMap on(String key, Runnable thunk) /*-{
this[key] = function() { $entry(thunk.@java.lang.Runnable::run()()); };
return this;
}-*/;
protected KeyMap() {
}
}

View File

@@ -16,6 +16,7 @@ package com.google.gerrit.client.diff;
import static org.junit.Assert.assertEquals;
import com.google.gerrit.client.diff.LineMapper.LineOnOtherInfo;
import com.google.gerrit.common.changes.Side;
import org.junit.Test;
@@ -51,41 +52,55 @@ public class LineMapperTest {
public void testFindInCommon() {
LineMapper mapper = new LineMapper();
mapper.appendCommon(10);
assertEquals(9, mapper.lineOnOther(Side.PARENT, 9));
assertEquals(new LineOnOtherInfo(9, true),
mapper.lineOnOther(Side.PARENT, 9));
assertEquals(new LineOnOtherInfo(9, true),
mapper.lineOnOther(Side.REVISION, 9));
}
@Test
public void testFindAfterCommon() {
LineMapper mapper = new LineMapper();
mapper.appendCommon(10);
assertEquals(10, mapper.lineOnOther(Side.PARENT, 10));
assertEquals(new LineOnOtherInfo(10, true),
mapper.lineOnOther(Side.PARENT, 10));
assertEquals(new LineOnOtherInfo(10, true),
mapper.lineOnOther(Side.REVISION, 10));
}
@Test
public void testFindInInsertGap() {
LineMapper mapper = new LineMapper();
mapper.appendInsert(10);
assertEquals(-1, mapper.lineOnOther(Side.REVISION, 9));
assertEquals(new LineOnOtherInfo(-1, false),
mapper.lineOnOther(Side.REVISION, 9));
}
@Test
public void testFindAfterInsertGap() {
LineMapper mapper = new LineMapper();
mapper.appendInsert(10);
assertEquals(0, mapper.lineOnOther(Side.REVISION, 10));
assertEquals(new LineOnOtherInfo(0, true),
mapper.lineOnOther(Side.REVISION, 10));
assertEquals(new LineOnOtherInfo(10, true),
mapper.lineOnOther(Side.PARENT, 0));
}
@Test
public void testFindInDeleteGap() {
LineMapper mapper = new LineMapper();
mapper.appendDelete(10);
assertEquals(-1, mapper.lineOnOther(Side.PARENT, 9));
assertEquals(new LineOnOtherInfo(-1, false),
mapper.lineOnOther(Side.PARENT, 9));
}
@Test
public void testFindAfterDeleteGap() {
LineMapper mapper = new LineMapper();
mapper.appendDelete(10);
assertEquals(0, mapper.lineOnOther(Side.PARENT, 10));
assertEquals(new LineOnOtherInfo(0, true),
mapper.lineOnOther(Side.PARENT, 10));
assertEquals(new LineOnOtherInfo(10, true),
mapper.lineOnOther(Side.REVISION, 0));
}
}