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,
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
_originalState: null,
|
2017-05-20 14:39:07 +02:00
|
|
|
_snippet: null,
|
2020-05-13 13:59:04 +02:00
|
|
|
_menuItems: null,
|
2020-05-12 20:37:55 +02:00
|
|
|
_documentEngineKey: 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,
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
_undoState: 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
|
|
|
|
2020-05-04 19:52:12 +02:00
|
|
|
_draftRequest: null,
|
2020-05-04 22:32:13 +02:00
|
|
|
_skipFocus: false,
|
2020-05-13 13:59:04 +02:00
|
|
|
_menu: null,
|
2020-05-04 19:52:12 +02:00
|
|
|
|
2020-05-14 00:27:11 +02:00
|
|
|
_startOffset: null,
|
|
|
|
_endOffset: null,
|
2020-05-14 21:49:29 +02:00
|
|
|
_isSelected: false,
|
2020-05-19 23:50:47 +02:00
|
|
|
_canSuggestEdit: false,
|
2020-05-14 00:27:11 +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
|
|
|
|
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;
|
2020-05-13 13:59:04 +02:00
|
|
|
this._menuItems = data.menuItems;
|
2020-05-12 20:37:55 +02:00
|
|
|
this._documentEngineKey = data.documentEngineKey;
|
2020-05-14 19:53:03 +02:00
|
|
|
|
2020-05-14 00:27:11 +02:00
|
|
|
this._startOffset = data.startOffset;
|
|
|
|
this._endOffset = data.endOffset;
|
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;
|
|
|
|
},
|
|
|
|
|
2020-05-14 00:27:11 +02:00
|
|
|
getStartOffset: function() {
|
|
|
|
return this._startOffset;
|
|
|
|
},
|
|
|
|
|
|
|
|
getEndOffset: function() {
|
|
|
|
return this._endOffset;
|
|
|
|
},
|
|
|
|
|
2020-05-14 21:49:29 +02:00
|
|
|
setIsSelected: function(is_selected) {
|
|
|
|
this._isSelected = is_selected;
|
|
|
|
|
|
|
|
if (this._row) {
|
|
|
|
JX.DOM.alterClass(
|
|
|
|
this._row,
|
|
|
|
'inline-comment-selected',
|
|
|
|
this._isSelected);
|
|
|
|
}
|
|
|
|
|
|
|
|
return this;
|
|
|
|
},
|
|
|
|
|
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;
|
2020-05-14 21:49:29 +02:00
|
|
|
|
|
|
|
if (data.hasOwnProperty('startOffset')) {
|
|
|
|
this._startOffset = data.startOffset;
|
|
|
|
} else {
|
|
|
|
this._startOffset = null;
|
|
|
|
}
|
|
|
|
|
|
|
|
if (data.hasOwnProperty('endOffset')) {
|
|
|
|
this._endOffset = data.endOffset;
|
|
|
|
} else {
|
|
|
|
this._endOffset = null;
|
|
|
|
}
|
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;
|
2020-05-12 20:37:55 +02:00
|
|
|
this._documentEngineKey = inline._documentEngineKey;
|
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() {
|
2020-05-13 13:59:04 +02:00
|
|
|
return this._hasMenuAction('reply');
|
2017-05-16 02:31:58 +02:00
|
|
|
},
|
|
|
|
|
|
|
|
canEdit: function() {
|
2020-05-13 13:59:04 +02:00
|
|
|
return this._hasMenuAction('edit');
|
2017-05-16 02:31:58 +02:00
|
|
|
},
|
|
|
|
|
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() {
|
2020-05-13 13:59:04 +02:00
|
|
|
return this._hasMenuAction('collapse');
|
2017-05-16 17:03:43 +02:00
|
|
|
},
|
|
|
|
|
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
|
|
|
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
this._originalState = null;
|
2017-05-16 22:03:45 +02:00
|
|
|
|
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) {
|
2020-05-13 13:59:04 +02:00
|
|
|
this._closeMenu();
|
|
|
|
|
2017-06-15 13:17:01 +02:00
|
|
|
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();
|
|
|
|
},
|
|
|
|
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
create: function(content_state) {
|
2020-05-12 20:37:55 +02:00
|
|
|
var changeset = this.getChangeset();
|
|
|
|
if (!this._documentEngineKey) {
|
|
|
|
this._documentEngineKey = changeset.getResponseDocumentEngineKey();
|
|
|
|
}
|
|
|
|
|
2017-05-16 01:26:45 +02:00
|
|
|
var uri = this._getInlineURI();
|
|
|
|
var handler = JX.bind(this, this._oncreateresponse);
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
var data = this._newRequestData('new', content_state);
|
2017-05-16 01:26:45 +02:00
|
|
|
|
|
|
|
this.setLoading(true);
|
|
|
|
|
|
|
|
new JX.Request(uri, handler)
|
|
|
|
.setData(data)
|
|
|
|
.send();
|
|
|
|
},
|
|
|
|
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
_getContentState: function() {
|
|
|
|
var state;
|
|
|
|
|
|
|
|
if (this._editRow) {
|
|
|
|
state = this._readFormState(this._editRow);
|
|
|
|
} else {
|
|
|
|
state = this._originalState;
|
|
|
|
}
|
|
|
|
|
|
|
|
return state;
|
|
|
|
},
|
|
|
|
|
2020-05-13 13:59:04 +02:00
|
|
|
reply: function(with_quote) {
|
|
|
|
this._closeMenu();
|
|
|
|
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
var content_state = this._newContentState();
|
2020-05-13 13:59:04 +02:00
|
|
|
if (with_quote) {
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
var text = this._getContentState().text;
|
2020-05-13 13:59:04 +02:00
|
|
|
text = '> ' + text.replace(/\n/g, '\n> ') + '\n\n';
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
content_state.text = text;
|
2020-05-13 13:59:04 +02:00
|
|
|
}
|
|
|
|
|
2017-05-16 01:35:06 +02:00
|
|
|
var changeset = this.getChangeset();
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
return changeset.newInlineReply(this, content_state);
|
2017-05-16 01:35:06 +02:00
|
|
|
},
|
|
|
|
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
edit: function(content_state, skip_focus) {
|
2020-05-13 13:59:04 +02:00
|
|
|
this._closeMenu();
|
|
|
|
|
2020-05-04 22:32:13 +02:00
|
|
|
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
|
|
|
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
var data = this._newRequestData('edit', content_state);
|
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';
|
|
|
|
}
|
|
|
|
|
|
|
|
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;
|
|
|
|
},
|
|
|
|
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
_newRequestData: function(operation, content_state) {
|
2020-05-14 00:27:11 +02:00
|
|
|
var data = {
|
2017-05-16 00:03:30 +02:00
|
|
|
op: operation,
|
2020-05-14 00:27:11 +02:00
|
|
|
is_new: this.isNewFile(),
|
2017-05-15 21:48:55 +02:00
|
|
|
on_right: ((this.getDisplaySide() == 'right') ? 1 : 0),
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
renderer: this.getChangeset().getRendererKey()
|
2017-05-15 21:48:55 +02:00
|
|
|
};
|
2020-05-14 00:27:11 +02:00
|
|
|
|
|
|
|
if (operation === 'new') {
|
|
|
|
var create_data = {
|
|
|
|
changesetID: this.getChangesetID(),
|
|
|
|
documentEngineKey: this._documentEngineKey,
|
|
|
|
replyToCommentPHID: this.getReplyToCommentPHID(),
|
|
|
|
startOffset: this._startOffset,
|
|
|
|
endOffset: this._endOffset,
|
|
|
|
number: this.getLineNumber(),
|
|
|
|
length: this.getLineLength()
|
|
|
|
};
|
|
|
|
|
|
|
|
JX.copy(data, create_data);
|
|
|
|
} else {
|
|
|
|
var edit_data = {
|
|
|
|
id: this._id
|
|
|
|
};
|
|
|
|
|
|
|
|
JX.copy(data, edit_data);
|
|
|
|
}
|
|
|
|
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
if (content_state) {
|
|
|
|
data.hasContentState = 1;
|
|
|
|
JX.copy(data, content_state);
|
|
|
|
}
|
|
|
|
|
2020-05-14 00:27:11 +02:00
|
|
|
return data;
|
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;
|
2020-05-19 23:50:47 +02:00
|
|
|
this._originalState = state.contentState;
|
|
|
|
this._canSuggestEdit = state.canSuggestEdit;
|
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
|
|
|
_ondeleteresponse: function() {
|
2020-05-08 16:59:18 +02:00
|
|
|
// If there's an existing "unedit" undo element, remove it.
|
|
|
|
if (this._undoRow) {
|
|
|
|
JX.DOM.remove(this._undoRow);
|
|
|
|
this._undoRow = null;
|
|
|
|
}
|
|
|
|
|
|
|
|
// If there's an existing editor, remove it. This happens when you
|
|
|
|
// delete a comment from the comment preview area. In this case, we
|
|
|
|
// read and preserve the text so "Undo" restores it.
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
var state = null;
|
2020-05-08 16:59:18 +02:00
|
|
|
if (this._editRow) {
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
state = this._readFormState(this._editRow);
|
2020-05-08 16:59:18 +02:00
|
|
|
JX.DOM.remove(this._editRow);
|
|
|
|
this._editRow = null;
|
|
|
|
}
|
|
|
|
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
this._drawUndeleteRows(state);
|
2017-05-16 00:03:30 +02:00
|
|
|
|
|
|
|
this.setLoading(false);
|
|
|
|
this.setDeleted(true);
|
|
|
|
|
|
|
|
this._didUpdate();
|
|
|
|
},
|
|
|
|
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
_drawUndeleteRows: function(content_state) {
|
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';
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
this._undoState = content_state || 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
|
|
|
|
2017-05-16 22:03:45 +02:00
|
|
|
return this._drawUndoRows('undelete', this._row);
|
|
|
|
},
|
|
|
|
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
_drawUneditRows: function(content_state) {
|
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';
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
this._undoState = content_state;
|
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
|
|
|
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
return this._drawUndoRows('unedit', null);
|
2017-05-16 22:03:45 +02:00
|
|
|
},
|
|
|
|
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
_drawUndoRows: function(mode, cursor) {
|
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();
|
|
|
|
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
this._undoRow = this._drawRows(template, cursor, mode);
|
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');
|
2020-05-19 23:50:47 +02:00
|
|
|
|
|
|
|
this._drawSuggestionState(this._editRow);
|
|
|
|
this.setHasSuggestion(this._originalState.hasSuggestion);
|
2017-05-16 00:03:30 +02:00
|
|
|
},
|
|
|
|
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
_drawRows: function(rows, cursor, type) {
|
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',
|
2020-05-19 23:50:32 +02:00
|
|
|
'inline-content-text');
|
2020-05-04 22:32:13 +02:00
|
|
|
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
|
|
|
},
|
|
|
|
|
2020-05-19 23:50:47 +02:00
|
|
|
_drawSuggestionState: function(row) {
|
|
|
|
if (this._canSuggestEdit) {
|
|
|
|
var button = this._getSuggestionButton();
|
|
|
|
var node = button.getNode();
|
|
|
|
|
|
|
|
// As a side effect of form submission, the button may become
|
|
|
|
// visually disabled. Re-enable it. This is a bit hacky.
|
|
|
|
JX.DOM.alterClass(node, 'disabled', false);
|
|
|
|
node.disabled = false;
|
|
|
|
|
|
|
|
var container = JX.DOM.find(row, 'div', 'inline-edit-buttons');
|
|
|
|
container.appendChild(node);
|
|
|
|
}
|
|
|
|
},
|
|
|
|
|
|
|
|
_getSuggestionButton: function() {
|
|
|
|
if (!this._suggestionButton) {
|
|
|
|
var button = new JX.PHUIXButtonView()
|
|
|
|
.setIcon('fa-pencil-square-o')
|
|
|
|
.setColor('grey');
|
|
|
|
|
|
|
|
var node = button.getNode();
|
|
|
|
JX.DOM.alterClass(node, 'inline-button-left', true);
|
|
|
|
|
|
|
|
var onclick = JX.bind(this, this._onSuggestEdit);
|
|
|
|
JX.DOM.listen(node, 'click', null, onclick);
|
|
|
|
|
|
|
|
this._suggestionButton = button;
|
|
|
|
}
|
|
|
|
|
|
|
|
return this._suggestionButton;
|
|
|
|
},
|
|
|
|
|
|
|
|
_onSuggestEdit: function(e) {
|
|
|
|
e.kill();
|
|
|
|
|
|
|
|
this.setHasSuggestion(!this.getHasSuggestion());
|
|
|
|
|
|
|
|
// The first time the user actually clicks the button and enables
|
|
|
|
// suggestions for a given editor state, fill the input with the
|
|
|
|
// underlying text if there isn't any text yet.
|
|
|
|
if (this.getHasSuggestion()) {
|
|
|
|
if (this._editRow) {
|
|
|
|
var node = this._getSuggestionNode(this._editRow);
|
|
|
|
if (node) {
|
|
|
|
if (!node.value.length) {
|
|
|
|
var data = JX.Stratcom.getData(node);
|
|
|
|
if (!data.hasSetDefault) {
|
|
|
|
data.hasSetDefault = true;
|
|
|
|
node.value = data.defaultText;
|
|
|
|
node.rows = Math.max(3, node.value.split('\n').length);
|
|
|
|
}
|
|
|
|
}
|
|
|
|
}
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
|
|
|
// Save the "hasSuggestion" part of the content state.
|
|
|
|
this.triggerDraft();
|
|
|
|
},
|
|
|
|
|
|
|
|
setHasSuggestion: function(has_suggestion) {
|
|
|
|
this._hasSuggestion = has_suggestion;
|
|
|
|
|
|
|
|
var button = this._getSuggestionButton();
|
|
|
|
var pht = this.getChangeset().getChangesetList().getTranslations();
|
|
|
|
if (has_suggestion) {
|
|
|
|
button
|
|
|
|
.setIcon('fa-times')
|
|
|
|
.setText(pht('Discard Edit'));
|
|
|
|
} else {
|
|
|
|
button
|
|
|
|
.setIcon('fa-plus')
|
|
|
|
.setText(pht('Suggest Edit'));
|
|
|
|
}
|
|
|
|
|
|
|
|
if (this._editRow) {
|
|
|
|
JX.DOM.alterClass(this._editRow, 'has-suggestion', has_suggestion);
|
|
|
|
}
|
|
|
|
},
|
|
|
|
|
|
|
|
getHasSuggestion: function() {
|
|
|
|
return this._hasSuggestion;
|
|
|
|
},
|
|
|
|
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
save: 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
|
|
|
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 inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
var uri = this._getInlineURI();
|
|
|
|
var data = this._newRequestData('save', this._getContentState());
|
|
|
|
|
|
|
|
new JX.Request(uri, handler)
|
|
|
|
.setData(data)
|
|
|
|
.send();
|
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() {
|
|
|
|
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 inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
if (this._undoState !== null) {
|
|
|
|
this.edit(this._undoState);
|
2017-05-16 22:03:45 +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() {
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
var state = this._readFormState(this._editRow);
|
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._editRow);
|
|
|
|
this._editRow = null;
|
2017-05-15 21:48:55 +02:00
|
|
|
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
var is_empty = this._isVoidContentState(state);
|
|
|
|
var is_same = this._isSameContentState(state, this._originalState);
|
|
|
|
if (!is_empty && !is_same) {
|
|
|
|
this._drawUneditRows(state);
|
2017-05-16 22:03:45 +02:00
|
|
|
}
|
2017-05-15 21:48:55 +02:00
|
|
|
|
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.
|
2020-05-23 00:27:52 +02:00
|
|
|
if (this._isVoidContentState(this._originalState)) {
|
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
|
|
|
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();
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
var data = this._newRequestData('cancel', this._originalState);
|
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) {
|
2020-05-14 20:17:51 +02:00
|
|
|
this.setEditing(false);
|
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
|
|
|
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.
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
if (this._isVoidContentState(this._originalState) && !this.isUndo()) {
|
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
|
|
|
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
|
|
|
},
|
|
|
|
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
_readFormState: function(row) {
|
2020-05-19 23:50:32 +02:00
|
|
|
var state = this._newContentState();
|
|
|
|
|
|
|
|
var node;
|
|
|
|
|
2017-05-16 22:03:45 +02:00
|
|
|
try {
|
2020-05-19 23:50:32 +02:00
|
|
|
node = JX.DOM.find(row, 'textarea', 'inline-content-text');
|
|
|
|
state.text = node.value;
|
2017-05-16 22:03:45 +02:00
|
|
|
} catch (ex) {
|
2020-05-19 23:50:32 +02:00
|
|
|
// Ignore.
|
2017-05-16 22:03:45 +02:00
|
|
|
}
|
|
|
|
|
2020-05-19 23:50:47 +02:00
|
|
|
node = this._getSuggestionNode(row);
|
|
|
|
if (node) {
|
2020-05-19 23:50:32 +02:00
|
|
|
state.suggestionText = node.value;
|
|
|
|
}
|
|
|
|
|
2020-05-19 23:50:47 +02:00
|
|
|
state.hasSuggestion = this.getHasSuggestion();
|
|
|
|
|
2020-05-19 23:50:32 +02:00
|
|
|
return state;
|
2017-05-16 22:03:45 +02:00
|
|
|
},
|
|
|
|
|
2020-05-19 23:50:47 +02:00
|
|
|
_getSuggestionNode: function(row) {
|
|
|
|
try {
|
|
|
|
return JX.DOM.find(row, 'textarea', 'inline-content-suggestion');
|
|
|
|
} catch (ex) {
|
|
|
|
return 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
|
|
|
_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;
|
|
|
|
}
|
|
|
|
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
var state = this._readFormState(this._editRow);
|
|
|
|
if (this._isVoidContentState(state)) {
|
2020-05-04 19:52:12 +02:00
|
|
|
return null;
|
|
|
|
}
|
|
|
|
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
var draft_data = {
|
2020-05-04 19:52:12 +02:00
|
|
|
op: 'draft',
|
|
|
|
id: this.getID(),
|
|
|
|
};
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
|
|
|
|
JX.copy(draft_data, state);
|
|
|
|
|
|
|
|
return draft_data;
|
2020-05-04 19:52:12 +02:00
|
|
|
},
|
|
|
|
|
|
|
|
triggerDraft: function() {
|
|
|
|
if (this._draftRequest) {
|
|
|
|
this._draftRequest.trigger();
|
|
|
|
}
|
2020-05-13 13:59:04 +02:00
|
|
|
},
|
|
|
|
|
|
|
|
activateMenu: function(button, e) {
|
|
|
|
// If we already have a menu for this button, let the menu handle the
|
|
|
|
// event.
|
|
|
|
var data = JX.Stratcom.getData(button);
|
|
|
|
if (data.menu) {
|
|
|
|
return;
|
|
|
|
}
|
|
|
|
|
|
|
|
e.prevent();
|
|
|
|
|
|
|
|
var menu = new JX.PHUIXDropdownMenu(button)
|
|
|
|
.setWidth(240);
|
|
|
|
|
|
|
|
var list = new JX.PHUIXActionListView();
|
|
|
|
var items = this._newMenuItems(menu);
|
|
|
|
for (var ii = 0; ii < items.length; ii++) {
|
|
|
|
list.addItem(items[ii]);
|
|
|
|
}
|
|
|
|
|
|
|
|
menu.setContent(list.getNode());
|
|
|
|
|
|
|
|
data.menu = menu;
|
|
|
|
this._menu = menu;
|
|
|
|
|
|
|
|
menu.listen('open', JX.bind(this, function() {
|
|
|
|
var changeset_list = this.getChangeset().getChangesetList();
|
|
|
|
changeset_list.selectInline(this, true);
|
|
|
|
}));
|
|
|
|
|
|
|
|
menu.open();
|
|
|
|
},
|
|
|
|
|
|
|
|
_newMenuItems: function(menu) {
|
|
|
|
var items = [];
|
|
|
|
|
|
|
|
for (var ii = 0; ii < this._menuItems.length; ii++) {
|
|
|
|
var spec = this._menuItems[ii];
|
|
|
|
|
2020-05-13 18:24:33 +02:00
|
|
|
var onmenu = JX.bind(this, this._onMenuItem, menu, spec.action, spec);
|
2020-05-13 13:59:04 +02:00
|
|
|
|
|
|
|
var item = new JX.PHUIXActionView()
|
|
|
|
.setIcon(spec.icon)
|
|
|
|
.setName(spec.label)
|
|
|
|
.setHandler(onmenu);
|
|
|
|
|
|
|
|
if (spec.key) {
|
|
|
|
item.setKeyCommand(spec.key);
|
|
|
|
}
|
|
|
|
|
|
|
|
items.push(item);
|
|
|
|
}
|
|
|
|
|
|
|
|
return items;
|
|
|
|
},
|
|
|
|
|
2020-05-13 18:24:33 +02:00
|
|
|
_onMenuItem: function(menu, action, spec, e) {
|
2020-05-13 13:59:04 +02:00
|
|
|
e.prevent();
|
|
|
|
menu.close();
|
|
|
|
|
|
|
|
switch (action) {
|
|
|
|
case 'reply':
|
|
|
|
this.reply();
|
|
|
|
break;
|
|
|
|
case 'quote':
|
|
|
|
this.reply(true);
|
|
|
|
break;
|
|
|
|
case 'collapse':
|
|
|
|
this.setCollapsed(true);
|
|
|
|
break;
|
|
|
|
case 'delete':
|
|
|
|
this.delete();
|
|
|
|
break;
|
|
|
|
case 'edit':
|
|
|
|
this.edit();
|
|
|
|
break;
|
2020-05-13 18:24:33 +02:00
|
|
|
case 'raw':
|
|
|
|
new JX.Workflow(spec.uri)
|
|
|
|
.start();
|
|
|
|
break;
|
2020-05-13 13:59:04 +02:00
|
|
|
}
|
|
|
|
|
|
|
|
},
|
|
|
|
|
|
|
|
_hasMenuAction: function(action) {
|
|
|
|
for (var ii = 0; ii < this._menuItems.length; ii++) {
|
|
|
|
var spec = this._menuItems[ii];
|
|
|
|
if (spec.action === action) {
|
|
|
|
return true;
|
|
|
|
}
|
|
|
|
}
|
|
|
|
return false;
|
|
|
|
},
|
|
|
|
|
|
|
|
_closeMenu: function() {
|
|
|
|
if (this._menu) {
|
|
|
|
this._menu.close();
|
|
|
|
}
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
},
|
|
|
|
|
2020-05-19 23:50:32 +02:00
|
|
|
_newContentState: function() {
|
|
|
|
return {
|
|
|
|
text: '',
|
|
|
|
suggestionText: '',
|
2020-05-19 23:50:47 +02:00
|
|
|
hasSuggestion: false
|
2020-05-19 23:50:32 +02:00
|
|
|
};
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
},
|
|
|
|
|
2020-05-19 23:50:32 +02:00
|
|
|
_isVoidContentState: function(state) {
|
|
|
|
return (!state.text.length && !state.suggestionText.length);
|
Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
2020-05-19 22:46:58 +02:00
|
|
|
},
|
|
|
|
|
2020-05-19 23:50:32 +02:00
|
|
|
_isSameContentState: function(u, v) {
|
|
|
|
return (
|
2020-05-19 23:50:47 +02:00
|
|
|
((u === null) === (v === null)) &&
|
2020-05-19 23:50:32 +02:00
|
|
|
(u.text === v.text) &&
|
|
|
|
(u.suggestionText === v.suggestionText) &&
|
|
|
|
(u.hasSuggestion === v.hasSuggestion));
|
2017-05-09 21:09:45 +02:00
|
|
|
}
|
|
|
|
}
|
|
|
|
|
|
|
|
});
|