Add "Undo" for editing Differential inline comments
Summary:
When a user hits 'cancel' on a 'new', 'edit', or 'reply' operation, add a little
"Changes discarded. __Undo__" insert so they can get their change back. No undo
for delete since there's an explicit prompt. Once this lands we can make
'escape' work again to close dialogs.
This change started feeling really good when I was merging all the duplicate
code and making things more consistent, but by the time I started writing client
rendering it felt gross. I'm not really thrilled with it but I guess it's a step
forward? The feature seems pretty OK in practice. Let me know how much barfing
this causes and I can try to remedy the most acute concerns.
This also fixes a bug where replies always (?) appear on the 'new' side of the
diff (I think?).
Test Plan:
Applied 'new', 'edit', 'delete' and 'reply' operations, pressed 'cancel' and
'okay' in each case, with and without changing text where relevant. All
behaviors seem to conform with expectations, except that canceling out of 'edit'
without changing the text gives you an option to undo when it shouldn't really.
There's no super easy way to get at the original text right now.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: simpkins, aran, epriestley
Differential Revision: 406
2011-06-08 01:11:10 +02:00
|
|
|
/**
|
|
|
|
* @provides differential-inline-comment-editor
|
|
|
|
* @requires javelin-dom
|
|
|
|
* javelin-util
|
|
|
|
* javelin-stratcom
|
|
|
|
* javelin-install
|
2012-02-29 23:28:48 +01:00
|
|
|
* javelin-request
|
|
|
|
* javelin-workflow
|
Add "Undo" for editing Differential inline comments
Summary:
When a user hits 'cancel' on a 'new', 'edit', or 'reply' operation, add a little
"Changes discarded. __Undo__" insert so they can get their change back. No undo
for delete since there's an explicit prompt. Once this lands we can make
'escape' work again to close dialogs.
This change started feeling really good when I was merging all the duplicate
code and making things more consistent, but by the time I started writing client
rendering it felt gross. I'm not really thrilled with it but I guess it's a step
forward? The feature seems pretty OK in practice. Let me know how much barfing
this causes and I can try to remedy the most acute concerns.
This also fixes a bug where replies always (?) appear on the 'new' side of the
diff (I think?).
Test Plan:
Applied 'new', 'edit', 'delete' and 'reply' operations, pressed 'cancel' and
'okay' in each case, with and without changing text where relevant. All
behaviors seem to conform with expectations, except that canceling out of 'edit'
without changing the text gives you an option to undo when it shouldn't really.
There's no super easy way to get at the original text right now.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: simpkins, aran, epriestley
Differential Revision: 406
2011-06-08 01:11:10 +02:00
|
|
|
*/
|
|
|
|
|
|
|
|
JX.install('DifferentialInlineCommentEditor', {
|
|
|
|
|
|
|
|
construct : function(uri) {
|
|
|
|
this._uri = uri;
|
|
|
|
},
|
|
|
|
|
|
|
|
events : ['done'],
|
|
|
|
|
|
|
|
members : {
|
|
|
|
_uri : null,
|
|
|
|
_undoText : null,
|
|
|
|
_skipOverInlineCommentRows : function(node) {
|
|
|
|
// TODO: Move this semantic information out of class names.
|
|
|
|
while (node && node.className.indexOf('inline') !== -1) {
|
|
|
|
node = node.nextSibling;
|
|
|
|
}
|
|
|
|
return node;
|
|
|
|
},
|
|
|
|
_buildRequestData : function() {
|
|
|
|
return {
|
|
|
|
op : this.getOperation(),
|
|
|
|
on_right : this.getOnRight(),
|
|
|
|
id : this.getID(),
|
|
|
|
number : this.getLineNumber(),
|
|
|
|
is_new : this.getIsNew(),
|
|
|
|
length : this.getLength(),
|
|
|
|
changeset : this.getChangeset(),
|
2015-03-05 23:11:51 +01:00
|
|
|
text : this.getText() || '',
|
|
|
|
renderer: this.getRenderer()
|
Add "Undo" for editing Differential inline comments
Summary:
When a user hits 'cancel' on a 'new', 'edit', or 'reply' operation, add a little
"Changes discarded. __Undo__" insert so they can get their change back. No undo
for delete since there's an explicit prompt. Once this lands we can make
'escape' work again to close dialogs.
This change started feeling really good when I was merging all the duplicate
code and making things more consistent, but by the time I started writing client
rendering it felt gross. I'm not really thrilled with it but I guess it's a step
forward? The feature seems pretty OK in practice. Let me know how much barfing
this causes and I can try to remedy the most acute concerns.
This also fixes a bug where replies always (?) appear on the 'new' side of the
diff (I think?).
Test Plan:
Applied 'new', 'edit', 'delete' and 'reply' operations, pressed 'cancel' and
'okay' in each case, with and without changing text where relevant. All
behaviors seem to conform with expectations, except that canceling out of 'edit'
without changing the text gives you an option to undo when it shouldn't really.
There's no super easy way to get at the original text right now.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: simpkins, aran, epriestley
Differential Revision: 406
2011-06-08 01:11:10 +02:00
|
|
|
};
|
|
|
|
},
|
|
|
|
_draw : function(content, exact_row) {
|
|
|
|
var row = this.getRow();
|
2011-06-10 16:36:42 +02:00
|
|
|
var table = this.getTable();
|
Add "Undo" for editing Differential inline comments
Summary:
When a user hits 'cancel' on a 'new', 'edit', or 'reply' operation, add a little
"Changes discarded. __Undo__" insert so they can get their change back. No undo
for delete since there's an explicit prompt. Once this lands we can make
'escape' work again to close dialogs.
This change started feeling really good when I was merging all the duplicate
code and making things more consistent, but by the time I started writing client
rendering it felt gross. I'm not really thrilled with it but I guess it's a step
forward? The feature seems pretty OK in practice. Let me know how much barfing
this causes and I can try to remedy the most acute concerns.
This also fixes a bug where replies always (?) appear on the 'new' side of the
diff (I think?).
Test Plan:
Applied 'new', 'edit', 'delete' and 'reply' operations, pressed 'cancel' and
'okay' in each case, with and without changing text where relevant. All
behaviors seem to conform with expectations, except that canceling out of 'edit'
without changing the text gives you an option to undo when it shouldn't really.
There's no super easy way to get at the original text right now.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: simpkins, aran, epriestley
Differential Revision: 406
2011-06-08 01:11:10 +02:00
|
|
|
var target = exact_row ? row : this._skipOverInlineCommentRows(row);
|
|
|
|
|
Make "Show Context" persist rendering, whitespace, encoding, etc
Summary:
Ref T2009. Currently, we do not persist view parameters when making context rendering requests.
The big one is the renderer (1up vs 2up). This makes context on unified diffs come in with too many columns.
However, it impacts other parameters too. For example, at HEAD, if you change highlighting to "rainbow" and then load more context, the context uses the original highlighter instead of the rainbow highlighter.
This moves context loads into ChangesetViewManager, which maintains view parameters and can provide them correctly.
- This removes "ref"; it is no longer required, as the ChangesetViewManager tracks it.
- This removes URI management from `behavior-show-more`; it is no longer required, since the ChangesetViewManager knows how to render.
- This removes "whitespace" since this is handled properly by the view manager.
Test Plan:
- Used "Show Top" / "Show All" / "Show Bottom" in 1-up and 2-up views.
- Changed file highlighting to rainbow, loaded stuff, saw rainbow stick.
- Used "Show Entire File" in 1-up and 2-up views.
- Saw loading chrome.
- No loading chrome normally.
- Made inlines, verified `copyRows()` code runs.
- Poked around Diffusion -- it is missing some parameter handling, but works OK.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D11977
2015-03-05 23:03:00 +01:00
|
|
|
function copyRows(dst, src, before) {
|
|
|
|
var rows = JX.DOM.scry(src, 'tr');
|
|
|
|
for (var ii = 0; ii < rows.length; ii++) {
|
|
|
|
|
|
|
|
// Find the table this <tr /> belongs to. If it's a sub-table, like a
|
|
|
|
// table in an inline comment, don't copy it.
|
|
|
|
if (JX.DOM.findAbove(rows[ii], 'table') !== src) {
|
|
|
|
continue;
|
|
|
|
}
|
|
|
|
|
|
|
|
if (before) {
|
|
|
|
dst.insertBefore(rows[ii], before);
|
|
|
|
} else {
|
|
|
|
dst.appendChild(rows[ii]);
|
|
|
|
}
|
|
|
|
}
|
|
|
|
return rows;
|
|
|
|
}
|
|
|
|
|
Add "Undo" for editing Differential inline comments
Summary:
When a user hits 'cancel' on a 'new', 'edit', or 'reply' operation, add a little
"Changes discarded. __Undo__" insert so they can get their change back. No undo
for delete since there's an explicit prompt. Once this lands we can make
'escape' work again to close dialogs.
This change started feeling really good when I was merging all the duplicate
code and making things more consistent, but by the time I started writing client
rendering it felt gross. I'm not really thrilled with it but I guess it's a step
forward? The feature seems pretty OK in practice. Let me know how much barfing
this causes and I can try to remedy the most acute concerns.
This also fixes a bug where replies always (?) appear on the 'new' side of the
diff (I think?).
Test Plan:
Applied 'new', 'edit', 'delete' and 'reply' operations, pressed 'cancel' and
'okay' in each case, with and without changing text where relevant. All
behaviors seem to conform with expectations, except that canceling out of 'edit'
without changing the text gives you an option to undo when it shouldn't really.
There's no super easy way to get at the original text right now.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: simpkins, aran, epriestley
Differential Revision: 406
2011-06-08 01:11:10 +02:00
|
|
|
return copyRows(table, content, target);
|
|
|
|
},
|
|
|
|
_removeUndoLink : function() {
|
|
|
|
var rows = JX.DifferentialInlineCommentEditor._undoRows;
|
|
|
|
if (rows) {
|
|
|
|
for (var ii = 0; ii < rows.length; ii++) {
|
|
|
|
JX.DOM.remove(rows[ii]);
|
|
|
|
}
|
|
|
|
}
|
|
|
|
},
|
|
|
|
_undo : function() {
|
|
|
|
this._removeUndoLink();
|
|
|
|
|
|
|
|
this.setText(this._undoText);
|
|
|
|
this.start();
|
|
|
|
},
|
|
|
|
_registerUndoListener : function() {
|
|
|
|
if (!JX.DifferentialInlineCommentEditor._activeEditor) {
|
|
|
|
JX.Stratcom.listen(
|
|
|
|
'click',
|
|
|
|
'differential-inline-comment-undo',
|
|
|
|
function(e) {
|
|
|
|
JX.DifferentialInlineCommentEditor._activeEditor._undo();
|
|
|
|
e.kill();
|
|
|
|
});
|
|
|
|
}
|
|
|
|
JX.DifferentialInlineCommentEditor._activeEditor = this;
|
|
|
|
},
|
2012-02-29 23:28:48 +01:00
|
|
|
_setRowState : function(state) {
|
|
|
|
var is_hidden = (state == 'hidden');
|
|
|
|
var is_loading = (state == 'loading');
|
|
|
|
var row = this.getRow();
|
|
|
|
JX.DOM.alterClass(row, 'differential-inline-hidden', is_hidden);
|
|
|
|
JX.DOM.alterClass(row, 'differential-inline-loading', is_loading);
|
|
|
|
},
|
|
|
|
_didContinueWorkflow : function(response) {
|
Don't mangle inline comments with tables in them in Differential
Summary:
Fixes T3814. Broadly, remarkup tables in inline comments did not work properly. I ran into several messes here and cleaned up some of them:
- Some of this code is doing `JX.$N('div', {}, JX.$H(response.markup))`, to turn an HTML response into a node, passing that around, and then doing junk with it. This is super old and gross.
- The slightly more modern pattern is `JX.$H(response.markup).getFragment().firstChild`, but this is kind of yuck too and not as safe as it could be.
- Introduce `JX.$H(response.markup).getNode()`, which actually expresses intent here. We have a bunch of `getFragment().firstChild` callsites which should switch to this, but I didn't clean those up yet because I don't want to test them all.
- Switch the `JX.$N('div', {}, JX.$H(response.markup))`-style callsites to `JX.$H(response.markup).getNode()`.
- `copyRows()` is too aggressive in finding `<tr />` tags. This actually causes the bug in T3814. We only want to find these tags at top level, not all tags. Don't copy `<tr />` tags which belong to some deeper table.
- Once this is fixed, there's another bug with mousing over the cells in tables in inline comments. We select the nearest `<td />`, but that's the cell in the remarkup table. Instead, select the correct `<td />`.
- At this point, these last two callsites were looking ugly. I provided `findAbove()` to clean them up.
Test Plan: Created, edited, deleted, moused over, and reloaded a revision with inline comments including remarkup tables. Used "Show more context" links.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3814
Differential Revision: https://secure.phabricator.com/D6924
2013-09-11 00:31:32 +02:00
|
|
|
var drawn = this._draw(JX.$H(response).getNode());
|
2012-02-29 23:28:48 +01:00
|
|
|
|
|
|
|
var op = this.getOperation();
|
|
|
|
if (op == 'edit') {
|
|
|
|
this._setRowState('hidden');
|
|
|
|
}
|
|
|
|
|
|
|
|
JX.DOM.find(
|
|
|
|
drawn[0],
|
|
|
|
'textarea',
|
|
|
|
'differential-inline-comment-edit-textarea').focus();
|
|
|
|
|
|
|
|
var oncancel = JX.bind(this, function(e) {
|
|
|
|
e.kill();
|
|
|
|
|
|
|
|
this._didCancelWorkflow();
|
|
|
|
|
|
|
|
if (op == 'edit') {
|
|
|
|
this._setRowState('visible');
|
|
|
|
}
|
|
|
|
|
|
|
|
JX.DOM.remove(drawn[0]);
|
|
|
|
});
|
|
|
|
JX.DOM.listen(drawn[0], 'click', 'inline-edit-cancel', oncancel);
|
|
|
|
|
|
|
|
var onsubmit = JX.bind(this, function(e) {
|
|
|
|
e.kill();
|
|
|
|
|
|
|
|
JX.Workflow.newFromForm(e.getTarget())
|
|
|
|
.setHandler(JX.bind(this, function(response) {
|
|
|
|
JX.DOM.remove(drawn[0]);
|
|
|
|
if (op == 'edit') {
|
|
|
|
this._setRowState('visible');
|
|
|
|
}
|
|
|
|
this._didCompleteWorkflow(response);
|
|
|
|
}))
|
|
|
|
.start();
|
|
|
|
|
|
|
|
JX.DOM.alterClass(drawn[0], 'differential-inline-loading', true);
|
|
|
|
});
|
2013-01-28 23:11:32 +01:00
|
|
|
JX.DOM.listen(
|
|
|
|
drawn[0],
|
|
|
|
['submit', 'didSyntheticSubmit'],
|
|
|
|
'inline-edit-form',
|
|
|
|
onsubmit);
|
2012-02-29 23:28:48 +01:00
|
|
|
},
|
Add "Undo" for editing Differential inline comments
Summary:
When a user hits 'cancel' on a 'new', 'edit', or 'reply' operation, add a little
"Changes discarded. __Undo__" insert so they can get their change back. No undo
for delete since there's an explicit prompt. Once this lands we can make
'escape' work again to close dialogs.
This change started feeling really good when I was merging all the duplicate
code and making things more consistent, but by the time I started writing client
rendering it felt gross. I'm not really thrilled with it but I guess it's a step
forward? The feature seems pretty OK in practice. Let me know how much barfing
this causes and I can try to remedy the most acute concerns.
This also fixes a bug where replies always (?) appear on the 'new' side of the
diff (I think?).
Test Plan:
Applied 'new', 'edit', 'delete' and 'reply' operations, pressed 'cancel' and
'okay' in each case, with and without changing text where relevant. All
behaviors seem to conform with expectations, except that canceling out of 'edit'
without changing the text gives you an option to undo when it shouldn't really.
There's no super easy way to get at the original text right now.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: simpkins, aran, epriestley
Differential Revision: 406
2011-06-08 01:11:10 +02:00
|
|
|
_didCompleteWorkflow : function(response) {
|
|
|
|
var op = this.getOperation();
|
|
|
|
|
|
|
|
// We don't get any markup back if the user deletes a comment, or saves
|
|
|
|
// an empty comment (which effects a delete).
|
|
|
|
if (response.markup) {
|
Don't mangle inline comments with tables in them in Differential
Summary:
Fixes T3814. Broadly, remarkup tables in inline comments did not work properly. I ran into several messes here and cleaned up some of them:
- Some of this code is doing `JX.$N('div', {}, JX.$H(response.markup))`, to turn an HTML response into a node, passing that around, and then doing junk with it. This is super old and gross.
- The slightly more modern pattern is `JX.$H(response.markup).getFragment().firstChild`, but this is kind of yuck too and not as safe as it could be.
- Introduce `JX.$H(response.markup).getNode()`, which actually expresses intent here. We have a bunch of `getFragment().firstChild` callsites which should switch to this, but I didn't clean those up yet because I don't want to test them all.
- Switch the `JX.$N('div', {}, JX.$H(response.markup))`-style callsites to `JX.$H(response.markup).getNode()`.
- `copyRows()` is too aggressive in finding `<tr />` tags. This actually causes the bug in T3814. We only want to find these tags at top level, not all tags. Don't copy `<tr />` tags which belong to some deeper table.
- Once this is fixed, there's another bug with mousing over the cells in tables in inline comments. We select the nearest `<td />`, but that's the cell in the remarkup table. Instead, select the correct `<td />`.
- At this point, these last two callsites were looking ugly. I provided `findAbove()` to clean them up.
Test Plan: Created, edited, deleted, moused over, and reloaded a revision with inline comments including remarkup tables. Used "Show more context" links.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3814
Differential Revision: https://secure.phabricator.com/D6924
2013-09-11 00:31:32 +02:00
|
|
|
this._draw(JX.$H(response.markup).getNode());
|
Add "Undo" for editing Differential inline comments
Summary:
When a user hits 'cancel' on a 'new', 'edit', or 'reply' operation, add a little
"Changes discarded. __Undo__" insert so they can get their change back. No undo
for delete since there's an explicit prompt. Once this lands we can make
'escape' work again to close dialogs.
This change started feeling really good when I was merging all the duplicate
code and making things more consistent, but by the time I started writing client
rendering it felt gross. I'm not really thrilled with it but I guess it's a step
forward? The feature seems pretty OK in practice. Let me know how much barfing
this causes and I can try to remedy the most acute concerns.
This also fixes a bug where replies always (?) appear on the 'new' side of the
diff (I think?).
Test Plan:
Applied 'new', 'edit', 'delete' and 'reply' operations, pressed 'cancel' and
'okay' in each case, with and without changing text where relevant. All
behaviors seem to conform with expectations, except that canceling out of 'edit'
without changing the text gives you an option to undo when it shouldn't really.
There's no super easy way to get at the original text right now.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: simpkins, aran, epriestley
Differential Revision: 406
2011-06-08 01:11:10 +02:00
|
|
|
}
|
|
|
|
|
|
|
|
// These operations remove the old row (edit adds a new row first).
|
|
|
|
var remove_old = (op == 'edit' || op == 'delete');
|
|
|
|
if (remove_old) {
|
|
|
|
JX.DOM.remove(this.getRow());
|
2012-08-02 21:24:23 +02:00
|
|
|
var other_rows = this.getOtherRows();
|
|
|
|
for(var i = 0; i < other_rows.length; ++i) {
|
|
|
|
JX.DOM.remove(other_rows[i]);
|
|
|
|
}
|
Add "Undo" for editing Differential inline comments
Summary:
When a user hits 'cancel' on a 'new', 'edit', or 'reply' operation, add a little
"Changes discarded. __Undo__" insert so they can get their change back. No undo
for delete since there's an explicit prompt. Once this lands we can make
'escape' work again to close dialogs.
This change started feeling really good when I was merging all the duplicate
code and making things more consistent, but by the time I started writing client
rendering it felt gross. I'm not really thrilled with it but I guess it's a step
forward? The feature seems pretty OK in practice. Let me know how much barfing
this causes and I can try to remedy the most acute concerns.
This also fixes a bug where replies always (?) appear on the 'new' side of the
diff (I think?).
Test Plan:
Applied 'new', 'edit', 'delete' and 'reply' operations, pressed 'cancel' and
'okay' in each case, with and without changing text where relevant. All
behaviors seem to conform with expectations, except that canceling out of 'edit'
without changing the text gives you an option to undo when it shouldn't really.
There's no super easy way to get at the original text right now.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: simpkins, aran, epriestley
Differential Revision: 406
2011-06-08 01:11:10 +02:00
|
|
|
}
|
|
|
|
|
|
|
|
// Once the user saves something, get rid of the 'undo' option. A
|
|
|
|
// particular case where we need this is saving a delete, when we might
|
|
|
|
// otherwise leave around an 'undo' for an earlier edit to the same
|
|
|
|
// comment.
|
|
|
|
this._removeUndoLink();
|
|
|
|
|
|
|
|
JX.Stratcom.invoke('differential-inline-comment-update');
|
|
|
|
this.invoke('done');
|
|
|
|
},
|
|
|
|
_didCancelWorkflow : function() {
|
|
|
|
this.invoke('done');
|
|
|
|
|
|
|
|
var op = this.getOperation();
|
|
|
|
if (op == 'delete') {
|
|
|
|
// No undo for delete, we prompt the user explicitly.
|
|
|
|
return;
|
|
|
|
}
|
|
|
|
|
2013-05-19 02:04:22 +02:00
|
|
|
var textarea;
|
Add "Undo" for editing Differential inline comments
Summary:
When a user hits 'cancel' on a 'new', 'edit', or 'reply' operation, add a little
"Changes discarded. __Undo__" insert so they can get their change back. No undo
for delete since there's an explicit prompt. Once this lands we can make
'escape' work again to close dialogs.
This change started feeling really good when I was merging all the duplicate
code and making things more consistent, but by the time I started writing client
rendering it felt gross. I'm not really thrilled with it but I guess it's a step
forward? The feature seems pretty OK in practice. Let me know how much barfing
this causes and I can try to remedy the most acute concerns.
This also fixes a bug where replies always (?) appear on the 'new' side of the
diff (I think?).
Test Plan:
Applied 'new', 'edit', 'delete' and 'reply' operations, pressed 'cancel' and
'okay' in each case, with and without changing text where relevant. All
behaviors seem to conform with expectations, except that canceling out of 'edit'
without changing the text gives you an option to undo when it shouldn't really.
There's no super easy way to get at the original text right now.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: simpkins, aran, epriestley
Differential Revision: 406
2011-06-08 01:11:10 +02:00
|
|
|
try {
|
2013-05-19 02:04:22 +02:00
|
|
|
textarea = JX.DOM.find(
|
Add "Undo" for editing Differential inline comments
Summary:
When a user hits 'cancel' on a 'new', 'edit', or 'reply' operation, add a little
"Changes discarded. __Undo__" insert so they can get their change back. No undo
for delete since there's an explicit prompt. Once this lands we can make
'escape' work again to close dialogs.
This change started feeling really good when I was merging all the duplicate
code and making things more consistent, but by the time I started writing client
rendering it felt gross. I'm not really thrilled with it but I guess it's a step
forward? The feature seems pretty OK in practice. Let me know how much barfing
this causes and I can try to remedy the most acute concerns.
This also fixes a bug where replies always (?) appear on the 'new' side of the
diff (I think?).
Test Plan:
Applied 'new', 'edit', 'delete' and 'reply' operations, pressed 'cancel' and
'okay' in each case, with and without changing text where relevant. All
behaviors seem to conform with expectations, except that canceling out of 'edit'
without changing the text gives you an option to undo when it shouldn't really.
There's no super easy way to get at the original text right now.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: simpkins, aran, epriestley
Differential Revision: 406
2011-06-08 01:11:10 +02:00
|
|
|
document.body, // TODO: use getDialogRootNode() when available
|
|
|
|
'textarea',
|
|
|
|
'differential-inline-comment-edit-textarea');
|
|
|
|
} catch (ex) {
|
|
|
|
// The close handler is called whenever the dialog closes, even if the
|
|
|
|
// user closed it by completing the workflow with "Save". The
|
|
|
|
// JX.Workflow API should probably be refined to allow programmatic
|
|
|
|
// distinction of close caused by 'cancel' vs 'submit'. Testing for
|
|
|
|
// presence of the textarea serves as a proxy for detecting a 'cancel'.
|
|
|
|
return;
|
|
|
|
}
|
|
|
|
|
|
|
|
var text = textarea.value;
|
|
|
|
|
|
|
|
// If the user hasn't edited the text (i.e., no change from original for
|
2012-03-01 07:21:40 +01:00
|
|
|
// 'edit' or no text at all), don't offer them an undo.
|
2013-05-19 02:04:22 +02:00
|
|
|
if (text == this.getOriginalText() || text === '') {
|
Add "Undo" for editing Differential inline comments
Summary:
When a user hits 'cancel' on a 'new', 'edit', or 'reply' operation, add a little
"Changes discarded. __Undo__" insert so they can get their change back. No undo
for delete since there's an explicit prompt. Once this lands we can make
'escape' work again to close dialogs.
This change started feeling really good when I was merging all the duplicate
code and making things more consistent, but by the time I started writing client
rendering it felt gross. I'm not really thrilled with it but I guess it's a step
forward? The feature seems pretty OK in practice. Let me know how much barfing
this causes and I can try to remedy the most acute concerns.
This also fixes a bug where replies always (?) appear on the 'new' side of the
diff (I think?).
Test Plan:
Applied 'new', 'edit', 'delete' and 'reply' operations, pressed 'cancel' and
'okay' in each case, with and without changing text where relevant. All
behaviors seem to conform with expectations, except that canceling out of 'edit'
without changing the text gives you an option to undo when it shouldn't really.
There's no super easy way to get at the original text right now.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: simpkins, aran, epriestley
Differential Revision: 406
2011-06-08 01:11:10 +02:00
|
|
|
return;
|
|
|
|
}
|
|
|
|
|
|
|
|
// Save the text so we can 'undo' back to it.
|
|
|
|
this._undoText = text;
|
|
|
|
|
2013-05-19 02:04:22 +02:00
|
|
|
var templates = this.getTemplates();
|
|
|
|
var template = this.getOnRight() ? templates.r : templates.l;
|
Don't mangle inline comments with tables in them in Differential
Summary:
Fixes T3814. Broadly, remarkup tables in inline comments did not work properly. I ran into several messes here and cleaned up some of them:
- Some of this code is doing `JX.$N('div', {}, JX.$H(response.markup))`, to turn an HTML response into a node, passing that around, and then doing junk with it. This is super old and gross.
- The slightly more modern pattern is `JX.$H(response.markup).getFragment().firstChild`, but this is kind of yuck too and not as safe as it could be.
- Introduce `JX.$H(response.markup).getNode()`, which actually expresses intent here. We have a bunch of `getFragment().firstChild` callsites which should switch to this, but I didn't clean those up yet because I don't want to test them all.
- Switch the `JX.$N('div', {}, JX.$H(response.markup))`-style callsites to `JX.$H(response.markup).getNode()`.
- `copyRows()` is too aggressive in finding `<tr />` tags. This actually causes the bug in T3814. We only want to find these tags at top level, not all tags. Don't copy `<tr />` tags which belong to some deeper table.
- Once this is fixed, there's another bug with mousing over the cells in tables in inline comments. We select the nearest `<td />`, but that's the cell in the remarkup table. Instead, select the correct `<td />`.
- At this point, these last two callsites were looking ugly. I provided `findAbove()` to clean them up.
Test Plan: Created, edited, deleted, moused over, and reloaded a revision with inline comments including remarkup tables. Used "Show more context" links.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3814
Differential Revision: https://secure.phabricator.com/D6924
2013-09-11 00:31:32 +02:00
|
|
|
template = JX.$H(template).getNode();
|
Add "Undo" for editing Differential inline comments
Summary:
When a user hits 'cancel' on a 'new', 'edit', or 'reply' operation, add a little
"Changes discarded. __Undo__" insert so they can get their change back. No undo
for delete since there's an explicit prompt. Once this lands we can make
'escape' work again to close dialogs.
This change started feeling really good when I was merging all the duplicate
code and making things more consistent, but by the time I started writing client
rendering it felt gross. I'm not really thrilled with it but I guess it's a step
forward? The feature seems pretty OK in practice. Let me know how much barfing
this causes and I can try to remedy the most acute concerns.
This also fixes a bug where replies always (?) appear on the 'new' side of the
diff (I think?).
Test Plan:
Applied 'new', 'edit', 'delete' and 'reply' operations, pressed 'cancel' and
'okay' in each case, with and without changing text where relevant. All
behaviors seem to conform with expectations, except that canceling out of 'edit'
without changing the text gives you an option to undo when it shouldn't really.
There's no super easy way to get at the original text right now.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: simpkins, aran, epriestley
Differential Revision: 406
2011-06-08 01:11:10 +02:00
|
|
|
|
|
|
|
// NOTE: Operation order matters here; we can't remove anything until
|
|
|
|
// after we draw the new rows because _draw uses the old rows to figure
|
|
|
|
// out where to place the comment.
|
|
|
|
|
|
|
|
// We use 'exact_row' to put the "undo" text directly above the affected
|
|
|
|
// comment.
|
|
|
|
var exact_row = true;
|
|
|
|
var rows = this._draw(template, exact_row);
|
|
|
|
|
|
|
|
this._removeUndoLink();
|
|
|
|
|
|
|
|
JX.DifferentialInlineCommentEditor._undoRows = rows;
|
|
|
|
},
|
|
|
|
|
|
|
|
start : function() {
|
|
|
|
this._registerUndoListener();
|
|
|
|
|
|
|
|
var data = this._buildRequestData();
|
|
|
|
|
2012-02-29 23:28:48 +01:00
|
|
|
var op = this.getOperation();
|
|
|
|
|
|
|
|
|
|
|
|
if (op == 'delete') {
|
|
|
|
this._setRowState('loading');
|
|
|
|
var oncomplete = JX.bind(this, this._didCompleteWorkflow);
|
|
|
|
var onclose = JX.bind(this, function() {
|
|
|
|
this._setRowState('visible');
|
|
|
|
this._didCancelWorkflow();
|
|
|
|
});
|
|
|
|
|
|
|
|
new JX.Workflow(this._uri, data)
|
|
|
|
.setHandler(oncomplete)
|
|
|
|
.setCloseHandler(onclose)
|
|
|
|
.start();
|
|
|
|
} else {
|
|
|
|
var handler = JX.bind(this, this._didContinueWorkflow);
|
|
|
|
|
|
|
|
if (op == 'edit') {
|
|
|
|
this._setRowState('loading');
|
|
|
|
}
|
|
|
|
|
|
|
|
new JX.Request(this._uri, handler)
|
|
|
|
.setData(data)
|
|
|
|
.send();
|
|
|
|
}
|
Add "Undo" for editing Differential inline comments
Summary:
When a user hits 'cancel' on a 'new', 'edit', or 'reply' operation, add a little
"Changes discarded. __Undo__" insert so they can get their change back. No undo
for delete since there's an explicit prompt. Once this lands we can make
'escape' work again to close dialogs.
This change started feeling really good when I was merging all the duplicate
code and making things more consistent, but by the time I started writing client
rendering it felt gross. I'm not really thrilled with it but I guess it's a step
forward? The feature seems pretty OK in practice. Let me know how much barfing
this causes and I can try to remedy the most acute concerns.
This also fixes a bug where replies always (?) appear on the 'new' side of the
diff (I think?).
Test Plan:
Applied 'new', 'edit', 'delete' and 'reply' operations, pressed 'cancel' and
'okay' in each case, with and without changing text where relevant. All
behaviors seem to conform with expectations, except that canceling out of 'edit'
without changing the text gives you an option to undo when it shouldn't really.
There's no super easy way to get at the original text right now.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: simpkins, aran, epriestley
Differential Revision: 406
2011-06-08 01:11:10 +02:00
|
|
|
|
|
|
|
return this;
|
|
|
|
}
|
|
|
|
},
|
|
|
|
|
|
|
|
statics : {
|
|
|
|
/**
|
|
|
|
* Global refernece to the 'undo' rows currently rendered in the document.
|
|
|
|
*/
|
|
|
|
_undoRows : null,
|
|
|
|
|
|
|
|
/**
|
|
|
|
* Global listener for the 'undo' click associated with the currently
|
|
|
|
* displayed 'undo' link. When an editor is start()ed, it becomes the active
|
|
|
|
* editor.
|
|
|
|
*/
|
|
|
|
_activeEditor : null
|
|
|
|
},
|
|
|
|
|
|
|
|
properties : {
|
|
|
|
operation : null,
|
|
|
|
row : null,
|
2012-08-02 21:24:23 +02:00
|
|
|
otherRows: [],
|
2011-06-10 16:36:42 +02:00
|
|
|
table : null,
|
Add "Undo" for editing Differential inline comments
Summary:
When a user hits 'cancel' on a 'new', 'edit', or 'reply' operation, add a little
"Changes discarded. __Undo__" insert so they can get their change back. No undo
for delete since there's an explicit prompt. Once this lands we can make
'escape' work again to close dialogs.
This change started feeling really good when I was merging all the duplicate
code and making things more consistent, but by the time I started writing client
rendering it felt gross. I'm not really thrilled with it but I guess it's a step
forward? The feature seems pretty OK in practice. Let me know how much barfing
this causes and I can try to remedy the most acute concerns.
This also fixes a bug where replies always (?) appear on the 'new' side of the
diff (I think?).
Test Plan:
Applied 'new', 'edit', 'delete' and 'reply' operations, pressed 'cancel' and
'okay' in each case, with and without changing text where relevant. All
behaviors seem to conform with expectations, except that canceling out of 'edit'
without changing the text gives you an option to undo when it shouldn't really.
There's no super easy way to get at the original text right now.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: simpkins, aran, epriestley
Differential Revision: 406
2011-06-08 01:11:10 +02:00
|
|
|
onRight : null,
|
|
|
|
ID : null,
|
|
|
|
lineNumber : null,
|
|
|
|
changeset : null,
|
|
|
|
length : null,
|
|
|
|
isNew : null,
|
|
|
|
text : null,
|
|
|
|
templates : null,
|
2015-03-05 23:11:51 +01:00
|
|
|
originalText : null,
|
|
|
|
renderer: null
|
Add "Undo" for editing Differential inline comments
Summary:
When a user hits 'cancel' on a 'new', 'edit', or 'reply' operation, add a little
"Changes discarded. __Undo__" insert so they can get their change back. No undo
for delete since there's an explicit prompt. Once this lands we can make
'escape' work again to close dialogs.
This change started feeling really good when I was merging all the duplicate
code and making things more consistent, but by the time I started writing client
rendering it felt gross. I'm not really thrilled with it but I guess it's a step
forward? The feature seems pretty OK in practice. Let me know how much barfing
this causes and I can try to remedy the most acute concerns.
This also fixes a bug where replies always (?) appear on the 'new' side of the
diff (I think?).
Test Plan:
Applied 'new', 'edit', 'delete' and 'reply' operations, pressed 'cancel' and
'okay' in each case, with and without changing text where relevant. All
behaviors seem to conform with expectations, except that canceling out of 'edit'
without changing the text gives you an option to undo when it shouldn't really.
There's no super easy way to get at the original text right now.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: simpkins, aran, epriestley
Differential Revision: 406
2011-06-08 01:11:10 +02:00
|
|
|
}
|
|
|
|
|
|
|
|
});
|