2017-05-09 21:09:45 +02:00
|
|
|
/**
|
|
|
|
* @provides phabricator-diff-inline
|
|
|
|
* @requires javelin-dom
|
|
|
|
* @javelin
|
|
|
|
*/
|
|
|
|
|
|
|
|
JX.install('DiffInline', {
|
|
|
|
|
2017-05-16 01:26:45 +02:00
|
|
|
construct : function() {
|
2017-05-09 21:09:45 +02:00
|
|
|
},
|
|
|
|
|
|
|
|
members: {
|
|
|
|
_id: null,
|
2017-05-16 01:26:45 +02:00
|
|
|
_phid: null,
|
|
|
|
_changesetID: null,
|
2017-05-09 21:09:45 +02:00
|
|
|
_row: null,
|
2017-05-15 21:48:55 +02:00
|
|
|
_number: null,
|
|
|
|
_length: null,
|
|
|
|
_displaySide: null,
|
|
|
|
_isNewFile: null,
|
2017-05-16 01:35:06 +02:00
|
|
|
_replyToCommentPHID: null,
|
2017-05-16 22:03:45 +02:00
|
|
|
_originalText: null,
|
2017-05-20 14:39:07 +02:00
|
|
|
_snippet: null,
|
2017-05-16 00:03:30 +02:00
|
|
|
|
|
|
|
_isDeleted: false,
|
|
|
|
_isInvisible: false,
|
|
|
|
_isLoading: false,
|
2017-05-09 21:09:45 +02:00
|
|
|
|
2017-05-18 20:54:58 +02:00
|
|
|
_changeset: null,
|
|
|
|
|
2017-06-15 13:17:01 +02:00
|
|
|
_isCollapsed: false,
|
2017-05-18 20:54:58 +02:00
|
|
|
_isDraft: null,
|
2017-06-15 13:28:21 +02:00
|
|
|
_isDraftDone: null,
|
2017-05-18 20:54:58 +02:00
|
|
|
_isFixed: null,
|
2017-05-20 13:58:09 +02:00
|
|
|
_isEditing: false,
|
2017-05-20 14:33:59 +02:00
|
|
|
_isNew: false,
|
2017-06-08 04:04:23 +02:00
|
|
|
_isSynthetic: false,
|
2017-06-15 13:51:06 +02:00
|
|
|
_isHidden: false,
|
2017-05-18 20:54:58 +02:00
|
|
|
|
Make "editing" state persistent for inline comments
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.
In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.
---
Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.
On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.
Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).
To simplify this:
- Make `PhabricatorInlineCommentInterface` an abstract base class instead.
- Lift as much code out of the `Audit` and `Differential` subclasses as possible.
- Delete methods which no longer have callers, or have only trivial callers.
---
Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.
Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.
These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.
The `EditView` can not fully render its content. Move the content rendering code into the view.
---
Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.
This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.
---
Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.
Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.
This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.
---
Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".
Test Plan:
- Created comments on either side of a diff.
- Edited a comment, reloaded, saw edit stick.
- Saved comments, reloaded, saw save stick.
- Edited a comment, typed text, cancelled, "unedited" to get state back.
- Created a comment, typed text, cancelled, "unedited" to get state back.
- Deleted a comment, "undeleted" to get state back.
Weirdness / known issues:
- Drafts don't autosave yet.
- Fixed in D21187:
- When you create an empty comment then reload, you get an empty editor. This is a bit silly.
- "Cancel" does not save state, but should, once drafts autosave.
- Mostly fixed in D21188:
- "Editing" comments aren't handled specially by the overall submission flow.
- "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.
Subscribers: jmeador
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21186
2020-04-28 23:45:54 +02:00
|
|
|
_editRow: null,
|
|
|
|
_undoRow: null,
|
|
|
|
_undoType: null,
|
|
|
|
_undoText: null,
|
|
|
|
|
2020-05-04 19:52:12 +02:00
|
|
|
_draftRequest: null,
|
2020-05-04 22:32:13 +02:00
|
|
|
_skipFocus: false,
|
2020-05-04 19:52:12 +02:00
|
|
|
|
2017-05-16 01:26:45 +02:00
|
|
|
bindToRow: function(row) {
|
|
|
|
this._row = row;
|
|
|
|
|
|
|
|
var row_data = JX.Stratcom.getData(row);
|
|
|
|
row_data.inline = this;
|
2017-06-15 13:17:01 +02:00
|
|
|
this._isCollapsed = row_data.hidden || false;
|
2017-05-16 01:26:45 +02:00
|
|
|
|
|
|
|
// TODO: Get smarter about this once we do more editing, this is pretty
|
|
|
|
// hacky.
|
|
|
|
var comment = JX.DOM.find(row, 'div', 'differential-inline-comment');
|
|
|
|
var data = JX.Stratcom.getData(comment);
|
|
|
|
|
Make "editing" state persistent for inline comments
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.
In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.
---
Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.
On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.
Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).
To simplify this:
- Make `PhabricatorInlineCommentInterface` an abstract base class instead.
- Lift as much code out of the `Audit` and `Differential` subclasses as possible.
- Delete methods which no longer have callers, or have only trivial callers.
---
Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.
Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.
These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.
The `EditView` can not fully render its content. Move the content rendering code into the view.
---
Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.
This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.
---
Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.
Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.
This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.
---
Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".
Test Plan:
- Created comments on either side of a diff.
- Edited a comment, reloaded, saw edit stick.
- Saved comments, reloaded, saw save stick.
- Edited a comment, typed text, cancelled, "unedited" to get state back.
- Created a comment, typed text, cancelled, "unedited" to get state back.
- Deleted a comment, "undeleted" to get state back.
Weirdness / known issues:
- Drafts don't autosave yet.
- Fixed in D21187:
- When you create an empty comment then reload, you get an empty editor. This is a bit silly.
- "Cancel" does not save state, but should, once drafts autosave.
- Mostly fixed in D21188:
- "Editing" comments aren't handled specially by the overall submission flow.
- "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.
Subscribers: jmeador
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21186
2020-04-28 23:45:54 +02:00
|
|
|
this._readInlineState(data);
|
2017-05-16 01:26:45 +02:00
|
|
|
this._phid = data.phid;
|
|
|
|
|
Make "editing" state persistent for inline comments
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.
In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.
---
Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.
On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.
Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).
To simplify this:
- Make `PhabricatorInlineCommentInterface` an abstract base class instead.
- Lift as much code out of the `Audit` and `Differential` subclasses as possible.
- Delete methods which no longer have callers, or have only trivial callers.
---
Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.
Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.
These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.
The `EditView` can not fully render its content. Move the content rendering code into the view.
---
Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.
This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.
---
Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.
Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.
This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.
---
Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".
Test Plan:
- Created comments on either side of a diff.
- Edited a comment, reloaded, saw edit stick.
- Saved comments, reloaded, saw save stick.
- Edited a comment, typed text, cancelled, "unedited" to get state back.
- Created a comment, typed text, cancelled, "unedited" to get state back.
- Deleted a comment, "undeleted" to get state back.
Weirdness / known issues:
- Drafts don't autosave yet.
- Fixed in D21187:
- When you create an empty comment then reload, you get an empty editor. This is a bit silly.
- "Cancel" does not save state, but should, once drafts autosave.
- Mostly fixed in D21188:
- "Editing" comments aren't handled specially by the overall submission flow.
- "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.
Subscribers: jmeador
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21186
2020-04-28 23:45:54 +02:00
|
|
|
if (data.on_right) {
|
2017-05-16 01:26:45 +02:00
|
|
|
this._displaySide = 'right';
|
|
|
|
} else {
|
|
|
|
this._displaySide = 'left';
|
|
|
|
}
|
|
|
|
|
2017-05-16 23:53:21 +02:00
|
|
|
this._number = parseInt(data.number, 10);
|
|
|
|
this._length = parseInt(data.length, 10);
|
Refine unusual inline comment client interactions
Summary: Ref T13513. Refine some inline behaviors, see test plan.
Test Plan:
- Edit a comment ("A"), type text ("AB"), cancel, edit.
- Old behavior: edit and undo states (wrong, and undo does not function).
- New behavior: edit state only.
- Edit a comment ("A"), type text ("AB"), cancel. Undo ("AB"), cancel. Edit.
- Old behavior: "AB" (wrong: you never submitted this text).
- New behavior: "A".
- Create a comment, type text, cancel.
- Old behavior: counter appears in filetree (wrong, comment is undo-able but should not be counted).
- New behavior: no counter.
- Cancel editing an empty comment with no text.
- Old behavior: Something buggy -- undo, I think?
- New behavior: it just vanishes (correct behavior).
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21212
2020-05-04 18:22:23 +02:00
|
|
|
|
|
|
|
var original = '' + data.original;
|
|
|
|
if (original.length) {
|
|
|
|
this._originalText = original;
|
|
|
|
} else {
|
|
|
|
this._originalText = null;
|
|
|
|
}
|
Make "editing" state persistent for inline comments
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.
In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.
---
Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.
On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.
Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).
To simplify this:
- Make `PhabricatorInlineCommentInterface` an abstract base class instead.
- Lift as much code out of the `Audit` and `Differential` subclasses as possible.
- Delete methods which no longer have callers, or have only trivial callers.
---
Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.
Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.
These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.
The `EditView` can not fully render its content. Move the content rendering code into the view.
---
Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.
This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.
---
Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.
Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.
This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.
---
Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".
Test Plan:
- Created comments on either side of a diff.
- Edited a comment, reloaded, saw edit stick.
- Saved comments, reloaded, saw save stick.
- Edited a comment, typed text, cancelled, "unedited" to get state back.
- Created a comment, typed text, cancelled, "unedited" to get state back.
- Deleted a comment, "undeleted" to get state back.
Weirdness / known issues:
- Drafts don't autosave yet.
- Fixed in D21187:
- When you create an empty comment then reload, you get an empty editor. This is a bit silly.
- "Cancel" does not save state, but should, once drafts autosave.
- Mostly fixed in D21188:
- "Editing" comments aren't handled specially by the overall submission flow.
- "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.
Subscribers: jmeador
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21186
2020-04-28 23:45:54 +02:00
|
|
|
this._isNewFile = data.isNewFile;
|
2017-05-16 01:26:45 +02:00
|
|
|
|
2017-05-17 20:59:00 +02:00
|
|
|
this._replyToCommentPHID = data.replyToCommentPHID;
|
|
|
|
|
2017-05-18 20:54:58 +02:00
|
|
|
this._isDraft = data.isDraft;
|
|
|
|
this._isFixed = data.isFixed;
|
|
|
|
this._isGhost = data.isGhost;
|
2017-06-08 04:04:23 +02:00
|
|
|
this._isSynthetic = data.isSynthetic;
|
2017-06-15 13:28:21 +02:00
|
|
|
this._isDraftDone = data.isDraftDone;
|
2017-05-18 20:54:58 +02:00
|
|
|
|
|
|
|
this._changesetID = data.changesetID;
|
2017-05-20 14:33:59 +02:00
|
|
|
this._isNew = false;
|
2017-05-20 14:39:07 +02:00
|
|
|
this._snippet = data.snippet;
|
2017-05-18 20:54:58 +02:00
|
|
|
|
Make "editing" state persistent for inline comments
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.
In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.
---
Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.
On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.
Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).
To simplify this:
- Make `PhabricatorInlineCommentInterface` an abstract base class instead.
- Lift as much code out of the `Audit` and `Differential` subclasses as possible.
- Delete methods which no longer have callers, or have only trivial callers.
---
Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.
Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.
These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.
The `EditView` can not fully render its content. Move the content rendering code into the view.
---
Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.
This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.
---
Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.
Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.
This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.
---
Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".
Test Plan:
- Created comments on either side of a diff.
- Edited a comment, reloaded, saw edit stick.
- Saved comments, reloaded, saw save stick.
- Edited a comment, typed text, cancelled, "unedited" to get state back.
- Created a comment, typed text, cancelled, "unedited" to get state back.
- Deleted a comment, "undeleted" to get state back.
Weirdness / known issues:
- Drafts don't autosave yet.
- Fixed in D21187:
- When you create an empty comment then reload, you get an empty editor. This is a bit silly.
- "Cancel" does not save state, but should, once drafts autosave.
- Mostly fixed in D21188:
- "Editing" comments aren't handled specially by the overall submission flow.
- "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.
Subscribers: jmeador
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21186
2020-04-28 23:45:54 +02:00
|
|
|
this._isEditing = data.isEditing;
|
|
|
|
|
|
|
|
if (this._isEditing) {
|
2020-05-04 19:52:12 +02:00
|
|
|
// NOTE: The "original" shipped down in the DOM may reflect a draft
|
|
|
|
// which we're currently editing. This flow is a little clumsy, but
|
|
|
|
// reasonable until some future change moves away from "send down
|
|
|
|
// the inline, then immediately click edit".
|
2020-05-04 22:55:43 +02:00
|
|
|
this.edit(null, true);
|
Make "editing" state persistent for inline comments
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.
In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.
---
Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.
On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.
Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).
To simplify this:
- Make `PhabricatorInlineCommentInterface` an abstract base class instead.
- Lift as much code out of the `Audit` and `Differential` subclasses as possible.
- Delete methods which no longer have callers, or have only trivial callers.
---
Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.
Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.
These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.
The `EditView` can not fully render its content. Move the content rendering code into the view.
---
Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.
This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.
---
Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.
Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.
This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.
---
Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".
Test Plan:
- Created comments on either side of a diff.
- Edited a comment, reloaded, saw edit stick.
- Saved comments, reloaded, saw save stick.
- Edited a comment, typed text, cancelled, "unedited" to get state back.
- Created a comment, typed text, cancelled, "unedited" to get state back.
- Deleted a comment, "undeleted" to get state back.
Weirdness / known issues:
- Drafts don't autosave yet.
- Fixed in D21187:
- When you create an empty comment then reload, you get an empty editor. This is a bit silly.
- "Cancel" does not save state, but should, once drafts autosave.
- Mostly fixed in D21188:
- "Editing" comments aren't handled specially by the overall submission flow.
- "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.
Subscribers: jmeador
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21186
2020-04-28 23:45:54 +02:00
|
|
|
} else {
|
|
|
|
this.setInvisible(false);
|
|
|
|
}
|
2017-05-16 01:26:45 +02:00
|
|
|
|
2020-05-04 19:52:12 +02:00
|
|
|
this._startDrafts();
|
|
|
|
|
2017-05-16 01:26:45 +02:00
|
|
|
return this;
|
|
|
|
},
|
|
|
|
|
2017-05-30 23:00:02 +02:00
|
|
|
isDraft: function() {
|
|
|
|
return this._isDraft;
|
|
|
|
},
|
|
|
|
|
|
|
|
isDone: function() {
|
|
|
|
return this._isFixed;
|
|
|
|
},
|
|
|
|
|
|
|
|
isEditing: function() {
|
|
|
|
return this._isEditing;
|
|
|
|
},
|
|
|
|
|
Refine unusual inline comment client interactions
Summary: Ref T13513. Refine some inline behaviors, see test plan.
Test Plan:
- Edit a comment ("A"), type text ("AB"), cancel, edit.
- Old behavior: edit and undo states (wrong, and undo does not function).
- New behavior: edit state only.
- Edit a comment ("A"), type text ("AB"), cancel. Undo ("AB"), cancel. Edit.
- Old behavior: "AB" (wrong: you never submitted this text).
- New behavior: "A".
- Create a comment, type text, cancel.
- Old behavior: counter appears in filetree (wrong, comment is undo-able but should not be counted).
- New behavior: no counter.
- Cancel editing an empty comment with no text.
- Old behavior: Something buggy -- undo, I think?
- New behavior: it just vanishes (correct behavior).
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21212
2020-05-04 18:22:23 +02:00
|
|
|
isUndo: function() {
|
|
|
|
return !!this._undoRow;
|
|
|
|
},
|
|
|
|
|
2017-05-30 23:00:02 +02:00
|
|
|
isDeleted: function() {
|
|
|
|
return this._isDeleted;
|
|
|
|
},
|
|
|
|
|
2017-06-08 04:04:23 +02:00
|
|
|
isSynthetic: function() {
|
|
|
|
return this._isSynthetic;
|
|
|
|
},
|
|
|
|
|
2017-06-15 13:28:21 +02:00
|
|
|
isDraftDone: function() {
|
|
|
|
return this._isDraftDone;
|
|
|
|
},
|
|
|
|
|
2017-06-15 13:51:06 +02:00
|
|
|
isHidden: function() {
|
|
|
|
return this._isHidden;
|
|
|
|
},
|
|
|
|
|
|
|
|
isGhost: function() {
|
|
|
|
return this._isGhost;
|
|
|
|
},
|
|
|
|
|
2017-05-16 01:26:45 +02:00
|
|
|
bindToRange: function(data) {
|
|
|
|
this._displaySide = data.displaySide;
|
2017-05-16 23:53:21 +02:00
|
|
|
this._number = parseInt(data.number, 10);
|
|
|
|
this._length = parseInt(data.length, 10);
|
2017-05-16 01:26:45 +02:00
|
|
|
this._isNewFile = data.isNewFile;
|
|
|
|
this._changesetID = data.changesetID;
|
2017-05-20 14:33:59 +02:00
|
|
|
this._isNew = true;
|
2017-05-16 01:26:45 +02:00
|
|
|
|
2017-05-16 01:35:06 +02:00
|
|
|
// Insert the comment after any other comments which already appear on
|
|
|
|
// the same row.
|
|
|
|
var parent_row = JX.DOM.findAbove(data.target, 'tr');
|
|
|
|
var target_row = parent_row.nextSibling;
|
|
|
|
while (target_row && JX.Stratcom.hasSigil(target_row, 'inline-row')) {
|
|
|
|
target_row = target_row.nextSibling;
|
|
|
|
}
|
|
|
|
|
2017-05-16 01:26:45 +02:00
|
|
|
var row = this._newRow();
|
2017-05-16 01:35:06 +02:00
|
|
|
parent_row.parentNode.insertBefore(row, target_row);
|
2017-05-16 01:26:45 +02:00
|
|
|
|
|
|
|
this.setInvisible(true);
|
2020-05-04 19:52:12 +02:00
|
|
|
this._startDrafts();
|
2017-05-16 01:26:45 +02:00
|
|
|
|
2017-05-16 01:35:06 +02:00
|
|
|
return this;
|
|
|
|
},
|
|
|
|
|
|
|
|
bindToReply: function(inline) {
|
|
|
|
this._displaySide = inline._displaySide;
|
|
|
|
this._number = inline._number;
|
|
|
|
this._length = inline._length;
|
|
|
|
this._isNewFile = inline._isNewFile;
|
|
|
|
this._changesetID = inline._changesetID;
|
2017-05-20 14:33:59 +02:00
|
|
|
this._isNew = true;
|
2017-05-16 01:35:06 +02:00
|
|
|
|
|
|
|
this._replyToCommentPHID = inline._phid;
|
|
|
|
|
2017-05-17 20:59:00 +02:00
|
|
|
var changeset = this.getChangeset();
|
|
|
|
|
|
|
|
// We're going to figure out where in the document to position the new
|
|
|
|
// inline. Normally, it goes after any existing inline rows (so if
|
|
|
|
// several inlines reply to the same line, they appear in chronological
|
|
|
|
// order).
|
|
|
|
|
|
|
|
// However: if inlines are threaded, we want to put the new inline in
|
|
|
|
// the right place in the thread. This might be somewhere in the middle,
|
|
|
|
// so we need to do a bit more work to figure it out.
|
|
|
|
|
|
|
|
// To find the right place in the thread, we're going to look for any
|
|
|
|
// inline which is at or above the level of the comment we're replying
|
|
|
|
// to. This means we've reached a new fork of the thread, and should
|
|
|
|
// put our new inline before the comment we found.
|
|
|
|
var ancestor_map = {};
|
|
|
|
var ancestor = inline;
|
|
|
|
var reply_phid;
|
|
|
|
while (ancestor) {
|
|
|
|
reply_phid = ancestor.getReplyToCommentPHID();
|
|
|
|
if (!reply_phid) {
|
|
|
|
break;
|
|
|
|
}
|
|
|
|
ancestor_map[reply_phid] = true;
|
|
|
|
ancestor = changeset.getInlineByPHID(reply_phid);
|
|
|
|
}
|
|
|
|
|
2017-05-16 01:35:06 +02:00
|
|
|
var parent_row = inline._row;
|
2017-05-16 01:26:45 +02:00
|
|
|
var target_row = parent_row.nextSibling;
|
|
|
|
while (target_row && JX.Stratcom.hasSigil(target_row, 'inline-row')) {
|
2017-05-17 20:59:00 +02:00
|
|
|
var target = changeset.getInlineForRow(target_row);
|
|
|
|
reply_phid = target.getReplyToCommentPHID();
|
|
|
|
|
|
|
|
// If we found an inline which is replying directly to some ancestor
|
|
|
|
// of this new comment, this is where the new rows go.
|
|
|
|
if (ancestor_map.hasOwnProperty(reply_phid)) {
|
|
|
|
break;
|
|
|
|
}
|
|
|
|
|
2017-05-16 01:26:45 +02:00
|
|
|
target_row = target_row.nextSibling;
|
|
|
|
}
|
2017-05-16 01:35:06 +02:00
|
|
|
|
|
|
|
var row = this._newRow();
|
2017-05-16 01:26:45 +02:00
|
|
|
parent_row.parentNode.insertBefore(row, target_row);
|
|
|
|
|
2017-05-16 01:35:06 +02:00
|
|
|
this.setInvisible(true);
|
2020-05-04 19:52:12 +02:00
|
|
|
this._startDrafts();
|
2017-05-16 01:35:06 +02:00
|
|
|
|
2017-05-16 01:26:45 +02:00
|
|
|
return this;
|
|
|
|
},
|
|
|
|
|
2017-05-18 20:54:58 +02:00
|
|
|
setChangeset: function(changeset) {
|
|
|
|
this._changeset = changeset;
|
|
|
|
return this;
|
|
|
|
},
|
|
|
|
|
|
|
|
getChangeset: function() {
|
|
|
|
return this._changeset;
|
|
|
|
},
|
|
|
|
|
2017-05-20 13:58:09 +02:00
|
|
|
setEditing: function(editing) {
|
|
|
|
this._isEditing = editing;
|
|
|
|
return this;
|
|
|
|
},
|
|
|
|
|
2017-06-15 13:51:06 +02:00
|
|
|
setHidden: function(hidden) {
|
|
|
|
this._isHidden = hidden;
|
|
|
|
this._redraw();
|
|
|
|
return this;
|
|
|
|
},
|
|
|
|
|
2017-05-16 02:31:58 +02:00
|
|
|
canReply: function() {
|
|
|
|
if (!this._hasAction('reply')) {
|
|
|
|
return false;
|
|
|
|
}
|
|
|
|
|
|
|
|
return true;
|
|
|
|
},
|
|
|
|
|
|
|
|
canEdit: function() {
|
|
|
|
if (!this._hasAction('edit')) {
|
|
|
|
return false;
|
|
|
|
}
|
|
|
|
|
|
|
|
return true;
|
|
|
|
},
|
|
|
|
|
2017-05-16 17:03:43 +02:00
|
|
|
canDone: function() {
|
|
|
|
if (!JX.DOM.scry(this._row, 'input', 'differential-inline-done').length) {
|
|
|
|
return false;
|
|
|
|
}
|
|
|
|
|
|
|
|
return true;
|
|
|
|
},
|
|
|
|
|
2017-06-15 13:17:01 +02:00
|
|
|
canCollapse: function() {
|
2017-05-16 17:03:43 +02:00
|
|
|
if (!JX.DOM.scry(this._row, 'a', 'hide-inline').length) {
|
|
|
|
return false;
|
|
|
|
}
|
|
|
|
|
|
|
|
return true;
|
|
|
|
},
|
|
|
|
|
2017-05-17 01:52:31 +02:00
|
|
|
getRawText: function() {
|
|
|
|
return this._originalText;
|
|
|
|
},
|
|
|
|
|
2017-05-16 02:31:58 +02:00
|
|
|
_hasAction: function(action) {
|
|
|
|
var nodes = JX.DOM.scry(this._row, 'a', 'differential-inline-' + action);
|
|
|
|
return (nodes.length > 0);
|
|
|
|
},
|
|
|
|
|
2017-05-16 01:26:45 +02:00
|
|
|
_newRow: function() {
|
|
|
|
var attributes = {
|
|
|
|
sigil: 'inline-row'
|
|
|
|
};
|
|
|
|
|
2017-05-16 01:35:06 +02:00
|
|
|
var row = JX.$N('tr', attributes);
|
|
|
|
|
|
|
|
JX.Stratcom.getData(row).inline = this;
|
|
|
|
this._row = row;
|
|
|
|
|
|
|
|
this._id = null;
|
|
|
|
this._phid = null;
|
2017-06-15 13:17:01 +02:00
|
|
|
this._isCollapsed = false;
|
2017-05-16 01:35:06 +02:00
|
|
|
|
2017-05-16 22:03:45 +02:00
|
|
|
this._originalText = null;
|
|
|
|
|
2017-05-16 01:35:06 +02:00
|
|
|
return row;
|
2017-05-16 01:26:45 +02:00
|
|
|
},
|
|
|
|
|
2017-06-15 13:17:01 +02:00
|
|
|
setCollapsed: function(collapsed) {
|
|
|
|
this._isCollapsed = collapsed;
|
2017-05-09 21:09:45 +02:00
|
|
|
|
|
|
|
var op;
|
2017-06-15 13:17:01 +02:00
|
|
|
if (collapsed) {
|
2017-05-09 21:09:45 +02:00
|
|
|
op = 'hide';
|
|
|
|
} else {
|
|
|
|
op = 'show';
|
|
|
|
}
|
|
|
|
|
2017-05-15 22:08:55 +02:00
|
|
|
var inline_uri = this._getInlineURI();
|
2017-05-09 21:09:45 +02:00
|
|
|
var comment_id = this._id;
|
|
|
|
|
|
|
|
new JX.Workflow(inline_uri, {op: op, ids: comment_id})
|
|
|
|
.setHandler(JX.bag)
|
|
|
|
.start();
|
2017-05-16 16:03:28 +02:00
|
|
|
|
2017-06-15 13:17:01 +02:00
|
|
|
this._redraw();
|
2017-05-16 17:03:43 +02:00
|
|
|
this._didUpdate(true);
|
2017-05-09 21:09:45 +02:00
|
|
|
},
|
|
|
|
|
2017-06-15 13:17:01 +02:00
|
|
|
isCollapsed: function() {
|
|
|
|
return this._isCollapsed;
|
2017-05-16 02:31:58 +02:00
|
|
|
},
|
|
|
|
|
2017-05-15 22:08:55 +02:00
|
|
|
toggleDone: function() {
|
|
|
|
var uri = this._getInlineURI();
|
|
|
|
var data = {
|
|
|
|
op: 'done',
|
|
|
|
id: this._id
|
|
|
|
};
|
|
|
|
|
|
|
|
var ondone = JX.bind(this, this._ondone);
|
|
|
|
|
|
|
|
new JX.Workflow(uri, data)
|
|
|
|
.setHandler(ondone)
|
|
|
|
.start();
|
|
|
|
},
|
|
|
|
|
|
|
|
_ondone: function(response) {
|
|
|
|
var checkbox = JX.DOM.find(
|
|
|
|
this._row,
|
|
|
|
'input',
|
|
|
|
'differential-inline-done');
|
|
|
|
|
|
|
|
checkbox.checked = (response.isChecked ? 'checked' : null);
|
|
|
|
|
|
|
|
var comment = JX.DOM.findAbove(
|
|
|
|
checkbox,
|
|
|
|
'div',
|
|
|
|
'differential-inline-comment');
|
|
|
|
|
|
|
|
JX.DOM.alterClass(comment, 'inline-is-done', response.isChecked);
|
|
|
|
|
|
|
|
// NOTE: This is marking the inline as having an unsubmitted checkmark,
|
|
|
|
// as opposed to a submitted checkmark. This is different from the
|
|
|
|
// top-level "draft" state of unsubmitted comments.
|
|
|
|
JX.DOM.alterClass(comment, 'inline-state-is-draft', response.draftState);
|
|
|
|
|
2017-05-18 20:54:58 +02:00
|
|
|
this._isFixed = response.isChecked;
|
2017-06-15 13:28:21 +02:00
|
|
|
this._isDraftDone = !!response.draftState;
|
2017-05-18 20:54:58 +02:00
|
|
|
|
2017-05-15 22:08:55 +02:00
|
|
|
this._didUpdate();
|
|
|
|
},
|
|
|
|
|
2017-05-16 22:03:45 +02:00
|
|
|
create: function(text) {
|
2017-05-16 01:26:45 +02:00
|
|
|
var uri = this._getInlineURI();
|
|
|
|
var handler = JX.bind(this, this._oncreateresponse);
|
2017-05-16 22:03:45 +02:00
|
|
|
var data = this._newRequestData('new', text);
|
2017-05-16 01:26:45 +02:00
|
|
|
|
|
|
|
this.setLoading(true);
|
|
|
|
|
|
|
|
new JX.Request(uri, handler)
|
|
|
|
.setData(data)
|
|
|
|
.send();
|
|
|
|
},
|
|
|
|
|
2017-05-17 01:52:31 +02:00
|
|
|
reply: function(text) {
|
2017-05-16 01:35:06 +02:00
|
|
|
var changeset = this.getChangeset();
|
2017-05-17 01:52:31 +02:00
|
|
|
return changeset.newInlineReply(this, text);
|
2017-05-16 01:35:06 +02:00
|
|
|
},
|
|
|
|
|
2020-05-04 22:32:13 +02:00
|
|
|
edit: function(text, skip_focus) {
|
|
|
|
this._skipFocus = !!skip_focus;
|
|
|
|
|
Refine unusual inline comment client interactions
Summary: Ref T13513. Refine some inline behaviors, see test plan.
Test Plan:
- Edit a comment ("A"), type text ("AB"), cancel, edit.
- Old behavior: edit and undo states (wrong, and undo does not function).
- New behavior: edit state only.
- Edit a comment ("A"), type text ("AB"), cancel. Undo ("AB"), cancel. Edit.
- Old behavior: "AB" (wrong: you never submitted this text).
- New behavior: "A".
- Create a comment, type text, cancel.
- Old behavior: counter appears in filetree (wrong, comment is undo-able but should not be counted).
- New behavior: no counter.
- Cancel editing an empty comment with no text.
- Old behavior: Something buggy -- undo, I think?
- New behavior: it just vanishes (correct behavior).
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21212
2020-05-04 18:22:23 +02:00
|
|
|
// If you edit an inline ("A"), modify the text ("AB"), cancel, and then
|
|
|
|
// edit it again: discard the undo state ("AB"). Otherwise we end up
|
|
|
|
// with an open editor and an active "Undo" link, which is weird.
|
|
|
|
|
|
|
|
if (this._undoRow) {
|
|
|
|
JX.DOM.remove(this._undoRow);
|
|
|
|
this._undoRow = null;
|
|
|
|
|
|
|
|
this._undoType = null;
|
|
|
|
this._undoText = null;
|
|
|
|
}
|
|
|
|
|
2017-05-15 22:08:55 +02:00
|
|
|
var uri = this._getInlineURI();
|
2017-05-16 00:03:30 +02:00
|
|
|
var handler = JX.bind(this, this._oneditresponse);
|
Refine unusual inline comment client interactions
Summary: Ref T13513. Refine some inline behaviors, see test plan.
Test Plan:
- Edit a comment ("A"), type text ("AB"), cancel, edit.
- Old behavior: edit and undo states (wrong, and undo does not function).
- New behavior: edit state only.
- Edit a comment ("A"), type text ("AB"), cancel. Undo ("AB"), cancel. Edit.
- Old behavior: "AB" (wrong: you never submitted this text).
- New behavior: "A".
- Create a comment, type text, cancel.
- Old behavior: counter appears in filetree (wrong, comment is undo-able but should not be counted).
- New behavior: no counter.
- Cancel editing an empty comment with no text.
- Old behavior: Something buggy -- undo, I think?
- New behavior: it just vanishes (correct behavior).
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21212
2020-05-04 18:22:23 +02:00
|
|
|
|
2017-05-16 22:03:45 +02:00
|
|
|
var data = this._newRequestData('edit', text || null);
|
2017-05-15 21:48:55 +02:00
|
|
|
|
2017-05-16 00:03:30 +02:00
|
|
|
this.setLoading(true);
|
2017-05-15 21:48:55 +02:00
|
|
|
|
|
|
|
new JX.Request(uri, handler)
|
|
|
|
.setData(data)
|
|
|
|
.send();
|
|
|
|
},
|
|
|
|
|
2017-05-16 00:03:30 +02:00
|
|
|
delete: function(is_ref) {
|
|
|
|
var uri = this._getInlineURI();
|
|
|
|
var handler = JX.bind(this, this._ondeleteresponse);
|
|
|
|
|
|
|
|
// NOTE: This may be a direct delete (the user clicked on the inline
|
|
|
|
// itself) or a "refdelete" (the user clicked somewhere else, like the
|
|
|
|
// preview, but the inline is present on the page).
|
|
|
|
|
|
|
|
// For a "refdelete", we prompt the user to confirm that they want to
|
|
|
|
// delete the comment, because they can not undo deletions from the
|
|
|
|
// preview. We could jump the user to the inline instead, but this would
|
|
|
|
// be somewhat disruptive and make deleting several comments more
|
|
|
|
// difficult.
|
|
|
|
|
|
|
|
var op;
|
|
|
|
if (is_ref) {
|
|
|
|
op = 'refdelete';
|
|
|
|
} else {
|
|
|
|
op = 'delete';
|
|
|
|
}
|
|
|
|
|
Make "editing" state persistent for inline comments
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.
In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.
---
Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.
On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.
Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).
To simplify this:
- Make `PhabricatorInlineCommentInterface` an abstract base class instead.
- Lift as much code out of the `Audit` and `Differential` subclasses as possible.
- Delete methods which no longer have callers, or have only trivial callers.
---
Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.
Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.
These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.
The `EditView` can not fully render its content. Move the content rendering code into the view.
---
Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.
This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.
---
Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.
Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.
This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.
---
Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".
Test Plan:
- Created comments on either side of a diff.
- Edited a comment, reloaded, saw edit stick.
- Saved comments, reloaded, saw save stick.
- Edited a comment, typed text, cancelled, "unedited" to get state back.
- Created a comment, typed text, cancelled, "unedited" to get state back.
- Deleted a comment, "undeleted" to get state back.
Weirdness / known issues:
- Drafts don't autosave yet.
- Fixed in D21187:
- When you create an empty comment then reload, you get an empty editor. This is a bit silly.
- "Cancel" does not save state, but should, once drafts autosave.
- Mostly fixed in D21188:
- "Editing" comments aren't handled specially by the overall submission flow.
- "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.
Subscribers: jmeador
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21186
2020-04-28 23:45:54 +02:00
|
|
|
// If there's an existing "unedit" undo element, remove it.
|
|
|
|
if (this._undoRow) {
|
|
|
|
JX.DOM.remove(this._undoRow);
|
|
|
|
this._undoRow = null;
|
|
|
|
}
|
|
|
|
|
2017-05-16 00:03:30 +02:00
|
|
|
var data = this._newRequestData(op);
|
|
|
|
|
|
|
|
this.setLoading(true);
|
|
|
|
|
|
|
|
new JX.Workflow(uri, data)
|
|
|
|
.setHandler(handler)
|
|
|
|
.start();
|
|
|
|
},
|
|
|
|
|
2017-05-15 21:48:55 +02:00
|
|
|
getDisplaySide: function() {
|
|
|
|
return this._displaySide;
|
|
|
|
},
|
|
|
|
|
|
|
|
getLineNumber: function() {
|
|
|
|
return this._number;
|
|
|
|
},
|
|
|
|
|
|
|
|
getLineLength: function() {
|
|
|
|
return this._length;
|
|
|
|
},
|
|
|
|
|
|
|
|
isNewFile: function() {
|
|
|
|
return this._isNewFile;
|
|
|
|
},
|
|
|
|
|
2017-05-16 00:03:30 +02:00
|
|
|
getID: function() {
|
|
|
|
return this._id;
|
|
|
|
},
|
|
|
|
|
2017-05-17 20:59:00 +02:00
|
|
|
getPHID: function() {
|
|
|
|
return this._phid;
|
|
|
|
},
|
|
|
|
|
2017-05-16 01:26:45 +02:00
|
|
|
getChangesetID: function() {
|
|
|
|
return this._changesetID;
|
|
|
|
},
|
|
|
|
|
2017-05-16 01:35:06 +02:00
|
|
|
getReplyToCommentPHID: function() {
|
|
|
|
return this._replyToCommentPHID;
|
|
|
|
},
|
|
|
|
|
2017-05-16 00:03:30 +02:00
|
|
|
setDeleted: function(deleted) {
|
|
|
|
this._isDeleted = deleted;
|
|
|
|
this._redraw();
|
|
|
|
return this;
|
|
|
|
},
|
|
|
|
|
|
|
|
setInvisible: function(invisible) {
|
|
|
|
this._isInvisible = invisible;
|
|
|
|
this._redraw();
|
|
|
|
return this;
|
|
|
|
},
|
|
|
|
|
|
|
|
setLoading: function(loading) {
|
|
|
|
this._isLoading = loading;
|
|
|
|
this._redraw();
|
|
|
|
return this;
|
|
|
|
},
|
|
|
|
|
2017-05-16 22:03:45 +02:00
|
|
|
_newRequestData: function(operation, text) {
|
2017-05-15 21:48:55 +02:00
|
|
|
return {
|
2017-05-16 00:03:30 +02:00
|
|
|
op: operation,
|
2017-05-15 21:48:55 +02:00
|
|
|
id: this._id,
|
|
|
|
on_right: ((this.getDisplaySide() == 'right') ? 1 : 0),
|
Make "renderer", "engine", and "encoding" sticky across reloads in Differential and Diffusion
Summary:
Ref T13455. Update the other "view state" properties to work like "highlight" now works.
Some complexity here arises from these concerns:
- In "View Standalone", we render the changeset inline. This is useful for debugging/development, and desirable to retain.
- In all other cases, we render the changeset with AJAX.
So the client needs to be able to learn about the "state" properties of the changeset on two different flows. Prior to this change, each pathway had a fair amount of unique code.
Then, some bookkeeping issues:
- At inital rendering time, we may not know which renderer will be selected: it may be based on the client viewport dimensions.
- Prior to this change, the client didn't separate "value of the property for the changeset as rendered" and "desired value of the property".
Test Plan:
- Viewed changes in Differential, Diffusion, and in standalone mode.
- Toggled renderer, character sets, and document engine (this one isn't terribly useful). Reloaded, saw them stick.
- Started typing a comment, cancelled it, hit the undo UI.
Maniphest Tasks: T13455
Differential Revision: https://secure.phabricator.com/D21138
2020-04-18 00:39:47 +02:00
|
|
|
renderer: this.getChangeset().getRendererKey(),
|
2017-05-15 21:48:55 +02:00
|
|
|
number: this.getLineNumber(),
|
|
|
|
length: this.getLineLength(),
|
|
|
|
is_new: this.isNewFile(),
|
2017-05-16 01:26:45 +02:00
|
|
|
changesetID: this.getChangesetID(),
|
2017-05-16 01:35:06 +02:00
|
|
|
replyToCommentPHID: this.getReplyToCommentPHID() || '',
|
2017-05-16 22:03:45 +02:00
|
|
|
text: text || ''
|
2017-05-15 21:48:55 +02:00
|
|
|
};
|
|
|
|
},
|
|
|
|
|
|
|
|
_oneditresponse: function(response) {
|
Make "editing" state persistent for inline comments
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.
In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.
---
Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.
On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.
Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).
To simplify this:
- Make `PhabricatorInlineCommentInterface` an abstract base class instead.
- Lift as much code out of the `Audit` and `Differential` subclasses as possible.
- Delete methods which no longer have callers, or have only trivial callers.
---
Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.
Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.
These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.
The `EditView` can not fully render its content. Move the content rendering code into the view.
---
Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.
This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.
---
Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.
Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.
This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.
---
Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".
Test Plan:
- Created comments on either side of a diff.
- Edited a comment, reloaded, saw edit stick.
- Saved comments, reloaded, saw save stick.
- Edited a comment, typed text, cancelled, "unedited" to get state back.
- Created a comment, typed text, cancelled, "unedited" to get state back.
- Deleted a comment, "undeleted" to get state back.
Weirdness / known issues:
- Drafts don't autosave yet.
- Fixed in D21187:
- When you create an empty comment then reload, you get an empty editor. This is a bit silly.
- "Cancel" does not save state, but should, once drafts autosave.
- Mostly fixed in D21188:
- "Editing" comments aren't handled specially by the overall submission flow.
- "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.
Subscribers: jmeador
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21186
2020-04-28 23:45:54 +02:00
|
|
|
var rows = JX.$H(response.view).getNode();
|
2017-05-15 21:48:55 +02:00
|
|
|
|
Make "editing" state persistent for inline comments
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.
In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.
---
Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.
On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.
Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).
To simplify this:
- Make `PhabricatorInlineCommentInterface` an abstract base class instead.
- Lift as much code out of the `Audit` and `Differential` subclasses as possible.
- Delete methods which no longer have callers, or have only trivial callers.
---
Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.
Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.
These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.
The `EditView` can not fully render its content. Move the content rendering code into the view.
---
Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.
This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.
---
Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.
Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.
This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.
---
Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".
Test Plan:
- Created comments on either side of a diff.
- Edited a comment, reloaded, saw edit stick.
- Saved comments, reloaded, saw save stick.
- Edited a comment, typed text, cancelled, "unedited" to get state back.
- Created a comment, typed text, cancelled, "unedited" to get state back.
- Deleted a comment, "undeleted" to get state back.
Weirdness / known issues:
- Drafts don't autosave yet.
- Fixed in D21187:
- When you create an empty comment then reload, you get an empty editor. This is a bit silly.
- "Cancel" does not save state, but should, once drafts autosave.
- Mostly fixed in D21188:
- "Editing" comments aren't handled specially by the overall submission flow.
- "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.
Subscribers: jmeador
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21186
2020-04-28 23:45:54 +02:00
|
|
|
this._readInlineState(response.inline);
|
2017-05-15 21:48:55 +02:00
|
|
|
this._drawEditRows(rows);
|
|
|
|
|
2017-05-16 00:03:30 +02:00
|
|
|
this.setLoading(false);
|
|
|
|
this.setInvisible(true);
|
|
|
|
},
|
|
|
|
|
2017-05-16 01:26:45 +02:00
|
|
|
_oncreateresponse: function(response) {
|
Make "editing" state persistent for inline comments
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.
In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.
---
Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.
On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.
Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).
To simplify this:
- Make `PhabricatorInlineCommentInterface` an abstract base class instead.
- Lift as much code out of the `Audit` and `Differential` subclasses as possible.
- Delete methods which no longer have callers, or have only trivial callers.
---
Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.
Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.
These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.
The `EditView` can not fully render its content. Move the content rendering code into the view.
---
Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.
This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.
---
Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.
Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.
This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.
---
Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".
Test Plan:
- Created comments on either side of a diff.
- Edited a comment, reloaded, saw edit stick.
- Saved comments, reloaded, saw save stick.
- Edited a comment, typed text, cancelled, "unedited" to get state back.
- Created a comment, typed text, cancelled, "unedited" to get state back.
- Deleted a comment, "undeleted" to get state back.
Weirdness / known issues:
- Drafts don't autosave yet.
- Fixed in D21187:
- When you create an empty comment then reload, you get an empty editor. This is a bit silly.
- "Cancel" does not save state, but should, once drafts autosave.
- Mostly fixed in D21188:
- "Editing" comments aren't handled specially by the overall submission flow.
- "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.
Subscribers: jmeador
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21186
2020-04-28 23:45:54 +02:00
|
|
|
var rows = JX.$H(response.view).getNode();
|
2017-05-16 01:26:45 +02:00
|
|
|
|
Make "editing" state persistent for inline comments
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.
In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.
---
Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.
On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.
Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).
To simplify this:
- Make `PhabricatorInlineCommentInterface` an abstract base class instead.
- Lift as much code out of the `Audit` and `Differential` subclasses as possible.
- Delete methods which no longer have callers, or have only trivial callers.
---
Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.
Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.
These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.
The `EditView` can not fully render its content. Move the content rendering code into the view.
---
Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.
This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.
---
Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.
Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.
This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.
---
Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".
Test Plan:
- Created comments on either side of a diff.
- Edited a comment, reloaded, saw edit stick.
- Saved comments, reloaded, saw save stick.
- Edited a comment, typed text, cancelled, "unedited" to get state back.
- Created a comment, typed text, cancelled, "unedited" to get state back.
- Deleted a comment, "undeleted" to get state back.
Weirdness / known issues:
- Drafts don't autosave yet.
- Fixed in D21187:
- When you create an empty comment then reload, you get an empty editor. This is a bit silly.
- "Cancel" does not save state, but should, once drafts autosave.
- Mostly fixed in D21188:
- "Editing" comments aren't handled specially by the overall submission flow.
- "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.
Subscribers: jmeador
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21186
2020-04-28 23:45:54 +02:00
|
|
|
this._readInlineState(response.inline);
|
2017-05-16 01:26:45 +02:00
|
|
|
this._drawEditRows(rows);
|
|
|
|
},
|
|
|
|
|
Make "editing" state persistent for inline comments
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.
In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.
---
Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.
On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.
Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).
To simplify this:
- Make `PhabricatorInlineCommentInterface` an abstract base class instead.
- Lift as much code out of the `Audit` and `Differential` subclasses as possible.
- Delete methods which no longer have callers, or have only trivial callers.
---
Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.
Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.
These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.
The `EditView` can not fully render its content. Move the content rendering code into the view.
---
Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.
This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.
---
Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.
Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.
This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.
---
Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".
Test Plan:
- Created comments on either side of a diff.
- Edited a comment, reloaded, saw edit stick.
- Saved comments, reloaded, saw save stick.
- Edited a comment, typed text, cancelled, "unedited" to get state back.
- Created a comment, typed text, cancelled, "unedited" to get state back.
- Deleted a comment, "undeleted" to get state back.
Weirdness / known issues:
- Drafts don't autosave yet.
- Fixed in D21187:
- When you create an empty comment then reload, you get an empty editor. This is a bit silly.
- "Cancel" does not save state, but should, once drafts autosave.
- Mostly fixed in D21188:
- "Editing" comments aren't handled specially by the overall submission flow.
- "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.
Subscribers: jmeador
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21186
2020-04-28 23:45:54 +02:00
|
|
|
_readInlineState: function(state) {
|
|
|
|
this._id = state.id;
|
|
|
|
},
|
|
|
|
|
2017-05-16 00:03:30 +02:00
|
|
|
_ondeleteresponse: function() {
|
2017-05-16 22:03:45 +02:00
|
|
|
this._drawUndeleteRows();
|
2017-05-16 00:03:30 +02:00
|
|
|
|
|
|
|
this.setLoading(false);
|
|
|
|
this.setDeleted(true);
|
|
|
|
|
|
|
|
this._didUpdate();
|
|
|
|
},
|
|
|
|
|
2017-05-16 22:03:45 +02:00
|
|
|
_drawUndeleteRows: function() {
|
Make "editing" state persistent for inline comments
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.
In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.
---
Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.
On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.
Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).
To simplify this:
- Make `PhabricatorInlineCommentInterface` an abstract base class instead.
- Lift as much code out of the `Audit` and `Differential` subclasses as possible.
- Delete methods which no longer have callers, or have only trivial callers.
---
Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.
Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.
These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.
The `EditView` can not fully render its content. Move the content rendering code into the view.
---
Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.
This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.
---
Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.
Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.
This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.
---
Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".
Test Plan:
- Created comments on either side of a diff.
- Edited a comment, reloaded, saw edit stick.
- Saved comments, reloaded, saw save stick.
- Edited a comment, typed text, cancelled, "unedited" to get state back.
- Created a comment, typed text, cancelled, "unedited" to get state back.
- Deleted a comment, "undeleted" to get state back.
Weirdness / known issues:
- Drafts don't autosave yet.
- Fixed in D21187:
- When you create an empty comment then reload, you get an empty editor. This is a bit silly.
- "Cancel" does not save state, but should, once drafts autosave.
- Mostly fixed in D21188:
- "Editing" comments aren't handled specially by the overall submission flow.
- "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.
Subscribers: jmeador
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21186
2020-04-28 23:45:54 +02:00
|
|
|
this._undoType = 'undelete';
|
|
|
|
this._undoText = null;
|
|
|
|
|
2017-05-16 22:03:45 +02:00
|
|
|
return this._drawUndoRows('undelete', this._row);
|
|
|
|
},
|
|
|
|
|
|
|
|
_drawUneditRows: function(text) {
|
Make "editing" state persistent for inline comments
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.
In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.
---
Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.
On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.
Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).
To simplify this:
- Make `PhabricatorInlineCommentInterface` an abstract base class instead.
- Lift as much code out of the `Audit` and `Differential` subclasses as possible.
- Delete methods which no longer have callers, or have only trivial callers.
---
Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.
Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.
These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.
The `EditView` can not fully render its content. Move the content rendering code into the view.
---
Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.
This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.
---
Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.
Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.
This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.
---
Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".
Test Plan:
- Created comments on either side of a diff.
- Edited a comment, reloaded, saw edit stick.
- Saved comments, reloaded, saw save stick.
- Edited a comment, typed text, cancelled, "unedited" to get state back.
- Created a comment, typed text, cancelled, "unedited" to get state back.
- Deleted a comment, "undeleted" to get state back.
Weirdness / known issues:
- Drafts don't autosave yet.
- Fixed in D21187:
- When you create an empty comment then reload, you get an empty editor. This is a bit silly.
- "Cancel" does not save state, but should, once drafts autosave.
- Mostly fixed in D21188:
- "Editing" comments aren't handled specially by the overall submission flow.
- "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.
Subscribers: jmeador
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21186
2020-04-28 23:45:54 +02:00
|
|
|
this._undoType = 'unedit';
|
|
|
|
this._undoText = text;
|
|
|
|
|
2017-05-16 22:03:45 +02:00
|
|
|
return this._drawUndoRows('unedit', null, text);
|
|
|
|
},
|
|
|
|
|
|
|
|
_drawUndoRows: function(mode, cursor, text) {
|
2017-05-16 00:03:30 +02:00
|
|
|
var templates = this.getChangeset().getUndoTemplates();
|
|
|
|
|
|
|
|
var template;
|
|
|
|
if (this.getDisplaySide() == 'right') {
|
|
|
|
template = templates.r;
|
|
|
|
} else {
|
|
|
|
template = templates.l;
|
|
|
|
}
|
|
|
|
template = JX.$H(template).getNode();
|
|
|
|
|
2017-05-16 22:03:45 +02:00
|
|
|
this._undoRow = this._drawRows(template, cursor, mode, text);
|
2017-05-15 21:48:55 +02:00
|
|
|
},
|
|
|
|
|
2017-05-20 13:58:09 +02:00
|
|
|
_drawContentRows: function(rows) {
|
|
|
|
return this._drawRows(rows, null, 'content');
|
|
|
|
},
|
|
|
|
|
2017-05-15 21:48:55 +02:00
|
|
|
_drawEditRows: function(rows) {
|
2017-05-20 13:58:09 +02:00
|
|
|
this.setEditing(true);
|
Make "editing" state persistent for inline comments
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.
In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.
---
Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.
On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.
Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).
To simplify this:
- Make `PhabricatorInlineCommentInterface` an abstract base class instead.
- Lift as much code out of the `Audit` and `Differential` subclasses as possible.
- Delete methods which no longer have callers, or have only trivial callers.
---
Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.
Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.
These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.
The `EditView` can not fully render its content. Move the content rendering code into the view.
---
Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.
This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.
---
Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.
Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.
This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.
---
Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".
Test Plan:
- Created comments on either side of a diff.
- Edited a comment, reloaded, saw edit stick.
- Saved comments, reloaded, saw save stick.
- Edited a comment, typed text, cancelled, "unedited" to get state back.
- Created a comment, typed text, cancelled, "unedited" to get state back.
- Deleted a comment, "undeleted" to get state back.
Weirdness / known issues:
- Drafts don't autosave yet.
- Fixed in D21187:
- When you create an empty comment then reload, you get an empty editor. This is a bit silly.
- "Cancel" does not save state, but should, once drafts autosave.
- Mostly fixed in D21188:
- "Editing" comments aren't handled specially by the overall submission flow.
- "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.
Subscribers: jmeador
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21186
2020-04-28 23:45:54 +02:00
|
|
|
this._editRow = this._drawRows(rows, null, 'edit');
|
2017-05-16 00:03:30 +02:00
|
|
|
},
|
|
|
|
|
2017-05-16 22:03:45 +02:00
|
|
|
_drawRows: function(rows, cursor, type, text) {
|
2017-05-15 21:48:55 +02:00
|
|
|
var first_row = JX.DOM.scry(rows, 'tr')[0];
|
|
|
|
var row = first_row;
|
2017-05-16 23:53:21 +02:00
|
|
|
var anchor = cursor || this._row;
|
2017-05-16 00:03:30 +02:00
|
|
|
cursor = cursor || this._row.nextSibling;
|
2017-05-15 21:48:55 +02:00
|
|
|
|
Make "editing" state persistent for inline comments
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.
In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.
---
Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.
On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.
Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).
To simplify this:
- Make `PhabricatorInlineCommentInterface` an abstract base class instead.
- Lift as much code out of the `Audit` and `Differential` subclasses as possible.
- Delete methods which no longer have callers, or have only trivial callers.
---
Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.
Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.
These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.
The `EditView` can not fully render its content. Move the content rendering code into the view.
---
Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.
This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.
---
Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.
Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.
This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.
---
Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".
Test Plan:
- Created comments on either side of a diff.
- Edited a comment, reloaded, saw edit stick.
- Saved comments, reloaded, saw save stick.
- Edited a comment, typed text, cancelled, "unedited" to get state back.
- Created a comment, typed text, cancelled, "unedited" to get state back.
- Deleted a comment, "undeleted" to get state back.
Weirdness / known issues:
- Drafts don't autosave yet.
- Fixed in D21187:
- When you create an empty comment then reload, you get an empty editor. This is a bit silly.
- "Cancel" does not save state, but should, once drafts autosave.
- Mostly fixed in D21188:
- "Editing" comments aren't handled specially by the overall submission flow.
- "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.
Subscribers: jmeador
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21186
2020-04-28 23:45:54 +02:00
|
|
|
|
|
|
|
var result_row;
|
2017-05-16 00:03:30 +02:00
|
|
|
var next_row;
|
2017-05-15 21:48:55 +02:00
|
|
|
while (row) {
|
2017-05-16 00:03:30 +02:00
|
|
|
// Grab this first, since it's going to change once we insert the row
|
|
|
|
// into the document.
|
|
|
|
next_row = row.nextSibling;
|
|
|
|
|
2017-05-16 23:53:21 +02:00
|
|
|
// Bind edit and undo rows to this DiffInline object so that
|
|
|
|
// interactions like hovering work properly.
|
|
|
|
JX.Stratcom.getData(row).inline = this;
|
|
|
|
|
|
|
|
anchor.parentNode.insertBefore(row, cursor);
|
2017-05-15 21:48:55 +02:00
|
|
|
cursor = row;
|
|
|
|
|
Make "editing" state persistent for inline comments
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.
In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.
---
Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.
On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.
Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).
To simplify this:
- Make `PhabricatorInlineCommentInterface` an abstract base class instead.
- Lift as much code out of the `Audit` and `Differential` subclasses as possible.
- Delete methods which no longer have callers, or have only trivial callers.
---
Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.
Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.
These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.
The `EditView` can not fully render its content. Move the content rendering code into the view.
---
Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.
This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.
---
Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.
Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.
This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.
---
Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".
Test Plan:
- Created comments on either side of a diff.
- Edited a comment, reloaded, saw edit stick.
- Saved comments, reloaded, saw save stick.
- Edited a comment, typed text, cancelled, "unedited" to get state back.
- Created a comment, typed text, cancelled, "unedited" to get state back.
- Deleted a comment, "undeleted" to get state back.
Weirdness / known issues:
- Drafts don't autosave yet.
- Fixed in D21187:
- When you create an empty comment then reload, you get an empty editor. This is a bit silly.
- "Cancel" does not save state, but should, once drafts autosave.
- Mostly fixed in D21188:
- "Editing" comments aren't handled specially by the overall submission flow.
- "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.
Subscribers: jmeador
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21186
2020-04-28 23:45:54 +02:00
|
|
|
if (!result_row) {
|
|
|
|
result_row = row;
|
2017-05-16 00:03:30 +02:00
|
|
|
}
|
|
|
|
|
2020-05-04 22:32:13 +02:00
|
|
|
if (!this._skipFocus) {
|
|
|
|
// If the row has a textarea, focus it. This allows the user to start
|
|
|
|
// typing a comment immediately after a "new", "edit", or "reply"
|
|
|
|
// action.
|
|
|
|
|
|
|
|
// (When simulating an "edit" on page load, we don't do this.)
|
|
|
|
|
|
|
|
var textareas = JX.DOM.scry(
|
|
|
|
row,
|
|
|
|
'textarea',
|
|
|
|
'differential-inline-comment-edit-textarea');
|
|
|
|
if (textareas.length) {
|
|
|
|
var area = textareas[0];
|
|
|
|
area.focus();
|
|
|
|
|
|
|
|
var length = area.value.length;
|
|
|
|
JX.TextAreaUtils.setSelectionRange(area, length, length);
|
|
|
|
}
|
2017-05-16 01:26:45 +02:00
|
|
|
}
|
|
|
|
|
2017-05-16 00:03:30 +02:00
|
|
|
row = next_row;
|
2017-05-15 21:48:55 +02:00
|
|
|
}
|
|
|
|
|
2017-05-16 16:11:43 +02:00
|
|
|
JX.Stratcom.invoke('resize');
|
|
|
|
|
Make "editing" state persistent for inline comments
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.
In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.
---
Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.
On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.
Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).
To simplify this:
- Make `PhabricatorInlineCommentInterface` an abstract base class instead.
- Lift as much code out of the `Audit` and `Differential` subclasses as possible.
- Delete methods which no longer have callers, or have only trivial callers.
---
Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.
Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.
These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.
The `EditView` can not fully render its content. Move the content rendering code into the view.
---
Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.
This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.
---
Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.
Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.
This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.
---
Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".
Test Plan:
- Created comments on either side of a diff.
- Edited a comment, reloaded, saw edit stick.
- Saved comments, reloaded, saw save stick.
- Edited a comment, typed text, cancelled, "unedited" to get state back.
- Created a comment, typed text, cancelled, "unedited" to get state back.
- Deleted a comment, "undeleted" to get state back.
Weirdness / known issues:
- Drafts don't autosave yet.
- Fixed in D21187:
- When you create an empty comment then reload, you get an empty editor. This is a bit silly.
- "Cancel" does not save state, but should, once drafts autosave.
- Mostly fixed in D21188:
- "Editing" comments aren't handled specially by the overall submission flow.
- "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.
Subscribers: jmeador
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21186
2020-04-28 23:45:54 +02:00
|
|
|
return result_row;
|
2017-05-15 21:48:55 +02:00
|
|
|
},
|
|
|
|
|
Make "editing" state persistent for inline comments
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.
In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.
---
Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.
On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.
Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).
To simplify this:
- Make `PhabricatorInlineCommentInterface` an abstract base class instead.
- Lift as much code out of the `Audit` and `Differential` subclasses as possible.
- Delete methods which no longer have callers, or have only trivial callers.
---
Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.
Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.
These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.
The `EditView` can not fully render its content. Move the content rendering code into the view.
---
Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.
This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.
---
Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.
Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.
This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.
---
Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".
Test Plan:
- Created comments on either side of a diff.
- Edited a comment, reloaded, saw edit stick.
- Saved comments, reloaded, saw save stick.
- Edited a comment, typed text, cancelled, "unedited" to get state back.
- Created a comment, typed text, cancelled, "unedited" to get state back.
- Deleted a comment, "undeleted" to get state back.
Weirdness / known issues:
- Drafts don't autosave yet.
- Fixed in D21187:
- When you create an empty comment then reload, you get an empty editor. This is a bit silly.
- "Cancel" does not save state, but should, once drafts autosave.
- Mostly fixed in D21188:
- "Editing" comments aren't handled specially by the overall submission flow.
- "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.
Subscribers: jmeador
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21186
2020-04-28 23:45:54 +02:00
|
|
|
save: function(form) {
|
|
|
|
var handler = JX.bind(this, this._onsubmitresponse);
|
2017-05-15 21:48:55 +02:00
|
|
|
|
2017-05-16 00:03:30 +02:00
|
|
|
this.setLoading(true);
|
|
|
|
|
Make "editing" state persistent for inline comments
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.
In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.
---
Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.
On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.
Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).
To simplify this:
- Make `PhabricatorInlineCommentInterface` an abstract base class instead.
- Lift as much code out of the `Audit` and `Differential` subclasses as possible.
- Delete methods which no longer have callers, or have only trivial callers.
---
Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.
Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.
These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.
The `EditView` can not fully render its content. Move the content rendering code into the view.
---
Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.
This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.
---
Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.
Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.
This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.
---
Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".
Test Plan:
- Created comments on either side of a diff.
- Edited a comment, reloaded, saw edit stick.
- Saved comments, reloaded, saw save stick.
- Edited a comment, typed text, cancelled, "unedited" to get state back.
- Created a comment, typed text, cancelled, "unedited" to get state back.
- Deleted a comment, "undeleted" to get state back.
Weirdness / known issues:
- Drafts don't autosave yet.
- Fixed in D21187:
- When you create an empty comment then reload, you get an empty editor. This is a bit silly.
- "Cancel" does not save state, but should, once drafts autosave.
- Mostly fixed in D21188:
- "Editing" comments aren't handled specially by the overall submission flow.
- "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.
Subscribers: jmeador
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21186
2020-04-28 23:45:54 +02:00
|
|
|
JX.Workflow.newFromForm(form)
|
2017-05-15 21:48:55 +02:00
|
|
|
.setHandler(handler)
|
|
|
|
.start();
|
2017-05-16 00:03:30 +02:00
|
|
|
},
|
|
|
|
|
Make "editing" state persistent for inline comments
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.
In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.
---
Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.
On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.
Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).
To simplify this:
- Make `PhabricatorInlineCommentInterface` an abstract base class instead.
- Lift as much code out of the `Audit` and `Differential` subclasses as possible.
- Delete methods which no longer have callers, or have only trivial callers.
---
Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.
Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.
These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.
The `EditView` can not fully render its content. Move the content rendering code into the view.
---
Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.
This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.
---
Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.
Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.
This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.
---
Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".
Test Plan:
- Created comments on either side of a diff.
- Edited a comment, reloaded, saw edit stick.
- Saved comments, reloaded, saw save stick.
- Edited a comment, typed text, cancelled, "unedited" to get state back.
- Created a comment, typed text, cancelled, "unedited" to get state back.
- Deleted a comment, "undeleted" to get state back.
Weirdness / known issues:
- Drafts don't autosave yet.
- Fixed in D21187:
- When you create an empty comment then reload, you get an empty editor. This is a bit silly.
- "Cancel" does not save state, but should, once drafts autosave.
- Mostly fixed in D21188:
- "Editing" comments aren't handled specially by the overall submission flow.
- "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.
Subscribers: jmeador
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21186
2020-04-28 23:45:54 +02:00
|
|
|
undo: function() {
|
2017-05-16 00:03:30 +02:00
|
|
|
|
Make "editing" state persistent for inline comments
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.
In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.
---
Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.
On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.
Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).
To simplify this:
- Make `PhabricatorInlineCommentInterface` an abstract base class instead.
- Lift as much code out of the `Audit` and `Differential` subclasses as possible.
- Delete methods which no longer have callers, or have only trivial callers.
---
Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.
Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.
These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.
The `EditView` can not fully render its content. Move the content rendering code into the view.
---
Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.
This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.
---
Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.
Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.
This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.
---
Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".
Test Plan:
- Created comments on either side of a diff.
- Edited a comment, reloaded, saw edit stick.
- Saved comments, reloaded, saw save stick.
- Edited a comment, typed text, cancelled, "unedited" to get state back.
- Created a comment, typed text, cancelled, "unedited" to get state back.
- Deleted a comment, "undeleted" to get state back.
Weirdness / known issues:
- Drafts don't autosave yet.
- Fixed in D21187:
- When you create an empty comment then reload, you get an empty editor. This is a bit silly.
- "Cancel" does not save state, but should, once drafts autosave.
- Mostly fixed in D21188:
- "Editing" comments aren't handled specially by the overall submission flow.
- "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.
Subscribers: jmeador
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21186
2020-04-28 23:45:54 +02:00
|
|
|
JX.DOM.remove(this._undoRow);
|
|
|
|
this._undoRow = null;
|
2017-05-15 21:48:55 +02:00
|
|
|
|
Make "editing" state persistent for inline comments
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.
In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.
---
Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.
On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.
Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).
To simplify this:
- Make `PhabricatorInlineCommentInterface` an abstract base class instead.
- Lift as much code out of the `Audit` and `Differential` subclasses as possible.
- Delete methods which no longer have callers, or have only trivial callers.
---
Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.
Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.
These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.
The `EditView` can not fully render its content. Move the content rendering code into the view.
---
Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.
This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.
---
Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.
Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.
This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.
---
Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".
Test Plan:
- Created comments on either side of a diff.
- Edited a comment, reloaded, saw edit stick.
- Saved comments, reloaded, saw save stick.
- Edited a comment, typed text, cancelled, "unedited" to get state back.
- Created a comment, typed text, cancelled, "unedited" to get state back.
- Deleted a comment, "undeleted" to get state back.
Weirdness / known issues:
- Drafts don't autosave yet.
- Fixed in D21187:
- When you create an empty comment then reload, you get an empty editor. This is a bit silly.
- "Cancel" does not save state, but should, once drafts autosave.
- Mostly fixed in D21188:
- "Editing" comments aren't handled specially by the overall submission flow.
- "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.
Subscribers: jmeador
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21186
2020-04-28 23:45:54 +02:00
|
|
|
if (this._undoType === 'undelete') {
|
2017-05-16 22:03:45 +02:00
|
|
|
var uri = this._getInlineURI();
|
|
|
|
var data = this._newRequestData('undelete');
|
|
|
|
var handler = JX.bind(this, this._onundelete);
|
2017-05-16 00:03:30 +02:00
|
|
|
|
2017-05-16 22:03:45 +02:00
|
|
|
this.setDeleted(false);
|
|
|
|
this.setLoading(true);
|
2017-05-16 00:03:30 +02:00
|
|
|
|
2017-05-16 22:03:45 +02:00
|
|
|
new JX.Request(uri, handler)
|
|
|
|
.setData(data)
|
|
|
|
.send();
|
|
|
|
}
|
|
|
|
|
Make "editing" state persistent for inline comments
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.
In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.
---
Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.
On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.
Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).
To simplify this:
- Make `PhabricatorInlineCommentInterface` an abstract base class instead.
- Lift as much code out of the `Audit` and `Differential` subclasses as possible.
- Delete methods which no longer have callers, or have only trivial callers.
---
Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.
Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.
These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.
The `EditView` can not fully render its content. Move the content rendering code into the view.
---
Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.
This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.
---
Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.
Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.
This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.
---
Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".
Test Plan:
- Created comments on either side of a diff.
- Edited a comment, reloaded, saw edit stick.
- Saved comments, reloaded, saw save stick.
- Edited a comment, typed text, cancelled, "unedited" to get state back.
- Created a comment, typed text, cancelled, "unedited" to get state back.
- Deleted a comment, "undeleted" to get state back.
Weirdness / known issues:
- Drafts don't autosave yet.
- Fixed in D21187:
- When you create an empty comment then reload, you get an empty editor. This is a bit silly.
- "Cancel" does not save state, but should, once drafts autosave.
- Mostly fixed in D21188:
- "Editing" comments aren't handled specially by the overall submission flow.
- "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.
Subscribers: jmeador
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21186
2020-04-28 23:45:54 +02:00
|
|
|
if (this._undoType === 'unedit') {
|
|
|
|
this.edit(this._undoText);
|
2017-05-16 22:03:45 +02:00
|
|
|
}
|
Make "editing" state persistent for inline comments
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.
In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.
---
Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.
On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.
Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).
To simplify this:
- Make `PhabricatorInlineCommentInterface` an abstract base class instead.
- Lift as much code out of the `Audit` and `Differential` subclasses as possible.
- Delete methods which no longer have callers, or have only trivial callers.
---
Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.
Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.
These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.
The `EditView` can not fully render its content. Move the content rendering code into the view.
---
Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.
This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.
---
Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.
Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.
This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.
---
Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".
Test Plan:
- Created comments on either side of a diff.
- Edited a comment, reloaded, saw edit stick.
- Saved comments, reloaded, saw save stick.
- Edited a comment, typed text, cancelled, "unedited" to get state back.
- Created a comment, typed text, cancelled, "unedited" to get state back.
- Deleted a comment, "undeleted" to get state back.
Weirdness / known issues:
- Drafts don't autosave yet.
- Fixed in D21187:
- When you create an empty comment then reload, you get an empty editor. This is a bit silly.
- "Cancel" does not save state, but should, once drafts autosave.
- Mostly fixed in D21188:
- "Editing" comments aren't handled specially by the overall submission flow.
- "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.
Subscribers: jmeador
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21186
2020-04-28 23:45:54 +02:00
|
|
|
|
2017-05-16 00:03:30 +02:00
|
|
|
},
|
|
|
|
|
|
|
|
_onundelete: function() {
|
|
|
|
this.setLoading(false);
|
|
|
|
this._didUpdate();
|
2017-05-15 21:48:55 +02:00
|
|
|
},
|
|
|
|
|
Make "editing" state persistent for inline comments
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.
In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.
---
Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.
On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.
Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).
To simplify this:
- Make `PhabricatorInlineCommentInterface` an abstract base class instead.
- Lift as much code out of the `Audit` and `Differential` subclasses as possible.
- Delete methods which no longer have callers, or have only trivial callers.
---
Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.
Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.
These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.
The `EditView` can not fully render its content. Move the content rendering code into the view.
---
Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.
This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.
---
Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.
Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.
This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.
---
Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".
Test Plan:
- Created comments on either side of a diff.
- Edited a comment, reloaded, saw edit stick.
- Saved comments, reloaded, saw save stick.
- Edited a comment, typed text, cancelled, "unedited" to get state back.
- Created a comment, typed text, cancelled, "unedited" to get state back.
- Deleted a comment, "undeleted" to get state back.
Weirdness / known issues:
- Drafts don't autosave yet.
- Fixed in D21187:
- When you create an empty comment then reload, you get an empty editor. This is a bit silly.
- "Cancel" does not save state, but should, once drafts autosave.
- Mostly fixed in D21188:
- "Editing" comments aren't handled specially by the overall submission flow.
- "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.
Subscribers: jmeador
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21186
2020-04-28 23:45:54 +02:00
|
|
|
cancel: function() {
|
|
|
|
var text = this._readText(this._editRow);
|
|
|
|
|
|
|
|
JX.DOM.remove(this._editRow);
|
|
|
|
this._editRow = null;
|
2017-05-15 21:48:55 +02:00
|
|
|
|
2017-05-16 22:03:45 +02:00
|
|
|
if (text && text.length && (text != this._originalText)) {
|
|
|
|
this._drawUneditRows(text);
|
|
|
|
}
|
2017-05-15 21:48:55 +02:00
|
|
|
|
2017-05-20 13:58:09 +02:00
|
|
|
this.setEditing(false);
|
Refine unusual inline comment client interactions
Summary: Ref T13513. Refine some inline behaviors, see test plan.
Test Plan:
- Edit a comment ("A"), type text ("AB"), cancel, edit.
- Old behavior: edit and undo states (wrong, and undo does not function).
- New behavior: edit state only.
- Edit a comment ("A"), type text ("AB"), cancel. Undo ("AB"), cancel. Edit.
- Old behavior: "AB" (wrong: you never submitted this text).
- New behavior: "A".
- Create a comment, type text, cancel.
- Old behavior: counter appears in filetree (wrong, comment is undo-able but should not be counted).
- New behavior: no counter.
- Cancel editing an empty comment with no text.
- Old behavior: Something buggy -- undo, I think?
- New behavior: it just vanishes (correct behavior).
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21212
2020-05-04 18:22:23 +02:00
|
|
|
|
|
|
|
// If this was an empty box and we typed some text and then hit cancel,
|
|
|
|
// don't show the empty concrete inline.
|
|
|
|
if (!this._originalText) {
|
|
|
|
this.setInvisible(true);
|
|
|
|
} else {
|
|
|
|
this.setInvisible(false);
|
|
|
|
}
|
|
|
|
|
|
|
|
// If you "undo" to restore text ("AB") and then "Cancel", we put you
|
|
|
|
// back in the original text state ("A"). We also send the original
|
|
|
|
// text ("A") to the server as the current persistent state.
|
2017-05-16 23:53:21 +02:00
|
|
|
|
When a user clicks "Cancel" on an inline comment to leave the "Editing" state, save the state change
Summary:
Ref T13513. Now that the "currently being edited" state of inlines is saved on the server side, clear the flag when the user clicks "Cancel" to leave the "editing" state on the client.
This also serves to delete empty comments.
Test Plan:
- Clicked a line number to create a new comment. Then:
- Clicked "Cancel". Reloaded page, saw no more comment.
- Typed text, saved. Reloaded page, saw non-editing draft. Clicked "Edit", reloaded page, saw editing draft. Clicked "Cancel", reloaded page, saw non-editing draft.
- Typed text, saved. Clicked "Edit", deleted text, saved. Reloaded page, saw no more comment.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21187
2020-04-29 20:25:43 +02:00
|
|
|
var uri = this._getInlineURI();
|
Refine unusual inline comment client interactions
Summary: Ref T13513. Refine some inline behaviors, see test plan.
Test Plan:
- Edit a comment ("A"), type text ("AB"), cancel, edit.
- Old behavior: edit and undo states (wrong, and undo does not function).
- New behavior: edit state only.
- Edit a comment ("A"), type text ("AB"), cancel. Undo ("AB"), cancel. Edit.
- Old behavior: "AB" (wrong: you never submitted this text).
- New behavior: "A".
- Create a comment, type text, cancel.
- Old behavior: counter appears in filetree (wrong, comment is undo-able but should not be counted).
- New behavior: no counter.
- Cancel editing an empty comment with no text.
- Old behavior: Something buggy -- undo, I think?
- New behavior: it just vanishes (correct behavior).
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21212
2020-05-04 18:22:23 +02:00
|
|
|
var data = this._newRequestData('cancel', this._originalText);
|
When a user clicks "Cancel" on an inline comment to leave the "Editing" state, save the state change
Summary:
Ref T13513. Now that the "currently being edited" state of inlines is saved on the server side, clear the flag when the user clicks "Cancel" to leave the "editing" state on the client.
This also serves to delete empty comments.
Test Plan:
- Clicked a line number to create a new comment. Then:
- Clicked "Cancel". Reloaded page, saw no more comment.
- Typed text, saved. Reloaded page, saw non-editing draft. Clicked "Edit", reloaded page, saw editing draft. Clicked "Cancel", reloaded page, saw non-editing draft.
- Typed text, saved. Clicked "Edit", deleted text, saved. Reloaded page, saw no more comment.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21187
2020-04-29 20:25:43 +02:00
|
|
|
var handler = JX.bind(this, this._onCancelResponse);
|
|
|
|
|
|
|
|
this.setLoading(true);
|
|
|
|
|
|
|
|
new JX.Request(uri, handler)
|
|
|
|
.setData(data)
|
|
|
|
.send();
|
|
|
|
|
2017-05-16 23:53:21 +02:00
|
|
|
this._didUpdate(true);
|
2017-05-15 21:48:55 +02:00
|
|
|
},
|
|
|
|
|
When a user clicks "Cancel" on an inline comment to leave the "Editing" state, save the state change
Summary:
Ref T13513. Now that the "currently being edited" state of inlines is saved on the server side, clear the flag when the user clicks "Cancel" to leave the "editing" state on the client.
This also serves to delete empty comments.
Test Plan:
- Clicked a line number to create a new comment. Then:
- Clicked "Cancel". Reloaded page, saw no more comment.
- Typed text, saved. Reloaded page, saw non-editing draft. Clicked "Edit", reloaded page, saw editing draft. Clicked "Cancel", reloaded page, saw non-editing draft.
- Typed text, saved. Clicked "Edit", deleted text, saved. Reloaded page, saw no more comment.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21187
2020-04-29 20:25:43 +02:00
|
|
|
_onCancelResponse: function(response) {
|
|
|
|
this.setLoading(false);
|
Refine unusual inline comment client interactions
Summary: Ref T13513. Refine some inline behaviors, see test plan.
Test Plan:
- Edit a comment ("A"), type text ("AB"), cancel, edit.
- Old behavior: edit and undo states (wrong, and undo does not function).
- New behavior: edit state only.
- Edit a comment ("A"), type text ("AB"), cancel. Undo ("AB"), cancel. Edit.
- Old behavior: "AB" (wrong: you never submitted this text).
- New behavior: "A".
- Create a comment, type text, cancel.
- Old behavior: counter appears in filetree (wrong, comment is undo-able but should not be counted).
- New behavior: no counter.
- Cancel editing an empty comment with no text.
- Old behavior: Something buggy -- undo, I think?
- New behavior: it just vanishes (correct behavior).
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21212
2020-05-04 18:22:23 +02:00
|
|
|
|
|
|
|
// If the comment was empty when we started editing it (there's no
|
|
|
|
// original text) and empty when we finished editing it (there's no
|
|
|
|
// undo row), just delete the comment.
|
|
|
|
if (!this._originalText && !this.isUndo()) {
|
|
|
|
this.setDeleted(true);
|
|
|
|
|
|
|
|
JX.DOM.remove(this._row);
|
|
|
|
this._row = null;
|
|
|
|
|
|
|
|
this._didUpdate();
|
|
|
|
}
|
When a user clicks "Cancel" on an inline comment to leave the "Editing" state, save the state change
Summary:
Ref T13513. Now that the "currently being edited" state of inlines is saved on the server side, clear the flag when the user clicks "Cancel" to leave the "editing" state on the client.
This also serves to delete empty comments.
Test Plan:
- Clicked a line number to create a new comment. Then:
- Clicked "Cancel". Reloaded page, saw no more comment.
- Typed text, saved. Reloaded page, saw non-editing draft. Clicked "Edit", reloaded page, saw editing draft. Clicked "Cancel", reloaded page, saw non-editing draft.
- Typed text, saved. Clicked "Edit", deleted text, saved. Reloaded page, saw no more comment.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21187
2020-04-29 20:25:43 +02:00
|
|
|
},
|
|
|
|
|
2017-05-16 22:03:45 +02:00
|
|
|
_readText: function(row) {
|
|
|
|
var textarea;
|
|
|
|
try {
|
|
|
|
textarea = JX.DOM.find(
|
|
|
|
row,
|
|
|
|
'textarea',
|
|
|
|
'differential-inline-comment-edit-textarea');
|
|
|
|
} catch (ex) {
|
|
|
|
return null;
|
|
|
|
}
|
|
|
|
|
|
|
|
return textarea.value;
|
|
|
|
},
|
|
|
|
|
Make "editing" state persistent for inline comments
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.
In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.
---
Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.
On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.
Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).
To simplify this:
- Make `PhabricatorInlineCommentInterface` an abstract base class instead.
- Lift as much code out of the `Audit` and `Differential` subclasses as possible.
- Delete methods which no longer have callers, or have only trivial callers.
---
Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.
Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.
These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.
The `EditView` can not fully render its content. Move the content rendering code into the view.
---
Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.
This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.
---
Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.
Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.
This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.
---
Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".
Test Plan:
- Created comments on either side of a diff.
- Edited a comment, reloaded, saw edit stick.
- Saved comments, reloaded, saw save stick.
- Edited a comment, typed text, cancelled, "unedited" to get state back.
- Created a comment, typed text, cancelled, "unedited" to get state back.
- Deleted a comment, "undeleted" to get state back.
Weirdness / known issues:
- Drafts don't autosave yet.
- Fixed in D21187:
- When you create an empty comment then reload, you get an empty editor. This is a bit silly.
- "Cancel" does not save state, but should, once drafts autosave.
- Mostly fixed in D21188:
- "Editing" comments aren't handled specially by the overall submission flow.
- "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.
Subscribers: jmeador
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21186
2020-04-28 23:45:54 +02:00
|
|
|
_onsubmitresponse: function(response) {
|
|
|
|
if (this._editRow) {
|
|
|
|
JX.DOM.remove(this._editRow);
|
|
|
|
this._editRow = null;
|
|
|
|
}
|
2017-05-15 21:48:55 +02:00
|
|
|
|
2017-05-16 01:26:45 +02:00
|
|
|
this.setLoading(false);
|
2017-05-16 00:03:30 +02:00
|
|
|
this.setInvisible(false);
|
2017-05-20 13:58:09 +02:00
|
|
|
this.setEditing(false);
|
2017-05-15 21:48:55 +02:00
|
|
|
|
|
|
|
this._onupdate(response);
|
|
|
|
},
|
|
|
|
|
|
|
|
_onupdate: function(response) {
|
|
|
|
var new_row;
|
Make "editing" state persistent for inline comments
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.
In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.
---
Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.
On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.
Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).
To simplify this:
- Make `PhabricatorInlineCommentInterface` an abstract base class instead.
- Lift as much code out of the `Audit` and `Differential` subclasses as possible.
- Delete methods which no longer have callers, or have only trivial callers.
---
Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.
Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.
These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.
The `EditView` can not fully render its content. Move the content rendering code into the view.
---
Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.
This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.
---
Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.
Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.
This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.
---
Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".
Test Plan:
- Created comments on either side of a diff.
- Edited a comment, reloaded, saw edit stick.
- Saved comments, reloaded, saw save stick.
- Edited a comment, typed text, cancelled, "unedited" to get state back.
- Created a comment, typed text, cancelled, "unedited" to get state back.
- Deleted a comment, "undeleted" to get state back.
Weirdness / known issues:
- Drafts don't autosave yet.
- Fixed in D21187:
- When you create an empty comment then reload, you get an empty editor. This is a bit silly.
- "Cancel" does not save state, but should, once drafts autosave.
- Mostly fixed in D21188:
- "Editing" comments aren't handled specially by the overall submission flow.
- "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.
Subscribers: jmeador
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21186
2020-04-28 23:45:54 +02:00
|
|
|
if (response.view) {
|
|
|
|
new_row = this._drawContentRows(JX.$H(response.view).getNode());
|
2017-05-15 21:48:55 +02:00
|
|
|
}
|
|
|
|
|
|
|
|
// TODO: Save the old row so the action it's undo-able if it was a
|
|
|
|
// delete.
|
|
|
|
var remove_old = true;
|
|
|
|
if (remove_old) {
|
|
|
|
JX.DOM.remove(this._row);
|
|
|
|
}
|
|
|
|
|
2017-07-21 17:08:24 +02:00
|
|
|
// If you delete the content on a comment and save it, it acts like a
|
|
|
|
// delete: the server does not return a new row.
|
|
|
|
if (new_row) {
|
|
|
|
this.bindToRow(new_row);
|
|
|
|
} else {
|
|
|
|
this.setDeleted(true);
|
|
|
|
this._row = null;
|
|
|
|
}
|
2017-05-15 21:48:55 +02:00
|
|
|
|
|
|
|
this._didUpdate();
|
|
|
|
},
|
|
|
|
|
2017-05-16 17:03:43 +02:00
|
|
|
_didUpdate: function(local_only) {
|
2017-05-15 21:48:55 +02:00
|
|
|
// After making changes to inline comments, refresh the transaction
|
|
|
|
// preview at the bottom of the page.
|
2017-05-16 17:03:43 +02:00
|
|
|
if (!local_only) {
|
|
|
|
this.getChangeset().getChangesetList().redrawPreview();
|
|
|
|
}
|
2017-05-16 16:11:43 +02:00
|
|
|
|
2017-05-16 16:24:20 +02:00
|
|
|
this.getChangeset().getChangesetList().redrawCursor();
|
2017-05-16 23:53:21 +02:00
|
|
|
this.getChangeset().getChangesetList().resetHover();
|
2017-05-16 16:24:20 +02:00
|
|
|
|
2017-05-16 23:53:21 +02:00
|
|
|
// Emit a resize event so that UI elements like the keyboard focus
|
2017-05-16 16:11:43 +02:00
|
|
|
// reticle can redraw properly.
|
|
|
|
JX.Stratcom.invoke('resize');
|
2017-05-16 00:03:30 +02:00
|
|
|
},
|
2017-05-15 21:48:55 +02:00
|
|
|
|
2017-05-16 00:03:30 +02:00
|
|
|
_redraw: function() {
|
2017-06-15 13:51:06 +02:00
|
|
|
var is_invisible =
|
|
|
|
(this._isInvisible || this._isDeleted || this._isHidden);
|
2017-06-15 13:17:01 +02:00
|
|
|
var is_loading = this._isLoading;
|
2017-06-15 13:51:06 +02:00
|
|
|
var is_collapsed = (this._isCollapsed && !this._isHidden);
|
2017-05-16 00:03:30 +02:00
|
|
|
|
|
|
|
var row = this._row;
|
|
|
|
JX.DOM.alterClass(row, 'differential-inline-hidden', is_invisible);
|
|
|
|
JX.DOM.alterClass(row, 'differential-inline-loading', is_loading);
|
2017-06-15 13:17:01 +02:00
|
|
|
JX.DOM.alterClass(row, 'inline-hidden', is_collapsed);
|
2017-05-15 21:48:55 +02:00
|
|
|
},
|
|
|
|
|
2017-05-15 22:08:55 +02:00
|
|
|
_getInlineURI: function() {
|
2017-05-09 21:09:45 +02:00
|
|
|
var changeset = this.getChangeset();
|
2017-05-15 22:08:55 +02:00
|
|
|
var list = changeset.getChangesetList();
|
|
|
|
return list.getInlineURI();
|
2020-05-04 19:52:12 +02:00
|
|
|
},
|
|
|
|
|
|
|
|
_startDrafts: function() {
|
|
|
|
if (this._draftRequest) {
|
|
|
|
return;
|
|
|
|
}
|
|
|
|
|
|
|
|
var onresponse = JX.bind(this, this._onDraftResponse);
|
|
|
|
var draft = JX.bind(this, this._getDraftState);
|
|
|
|
|
|
|
|
var uri = this._getInlineURI();
|
|
|
|
var request = new JX.PhabricatorShapedRequest(uri, onresponse, draft);
|
|
|
|
|
|
|
|
// The main transaction code uses a 500ms delay on desktop and a
|
|
|
|
// 10s delay on mobile. Perhaps this should be standardized.
|
|
|
|
request.setRateLimit(2000);
|
|
|
|
|
|
|
|
this._draftRequest = request;
|
|
|
|
|
|
|
|
request.start();
|
|
|
|
},
|
|
|
|
|
|
|
|
_onDraftResponse: function() {
|
|
|
|
// For now, do nothing.
|
|
|
|
},
|
|
|
|
|
|
|
|
_getDraftState: function() {
|
|
|
|
if (this.isDeleted()) {
|
|
|
|
return null;
|
|
|
|
}
|
|
|
|
|
|
|
|
if (!this.isEditing()) {
|
|
|
|
return null;
|
|
|
|
}
|
|
|
|
|
|
|
|
var text = this._readText(this._editRow);
|
|
|
|
if (text === null) {
|
|
|
|
return null;
|
|
|
|
}
|
|
|
|
|
|
|
|
return {
|
|
|
|
op: 'draft',
|
|
|
|
id: this.getID(),
|
|
|
|
text: text
|
|
|
|
};
|
|
|
|
},
|
|
|
|
|
|
|
|
triggerDraft: function() {
|
|
|
|
if (this._draftRequest) {
|
|
|
|
this._draftRequest.trigger();
|
|
|
|
}
|
2017-05-09 21:09:45 +02:00
|
|
|
}
|
2020-05-04 19:52:12 +02:00
|
|
|
|
2017-05-09 21:09:45 +02:00
|
|
|
}
|
|
|
|
|
|
|
|
});
|