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
Summary:
Ref T13513. Give selected inlines a selection state and visual cues which are similar to the changeset selection state.
Also fix a couple of minor issues with select interactions and offset comments.
Test Plan: Selected inlines, saw obvious visual cues.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21256
Summary: Ref T13513. This fixes a bug where clicking a line number, then clicking "Cancel" causes the paths panel to briefly update with an extra inline comment counted on the file.
Test Plan:
- Clicked a line number.
- Typed some text.
- Clicked "Cancel".
- Before patch: paths panel flashes "1".
- After patch: paths panel stays stable.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21253
Summary:
Ref T13513. Currently, when creating an inline by selecting a line range, slightly careless handling leads to an inline with "0" offsets (by passing "undefined" to the server). This causes the block to highlight every line except the last one as fully bright, which is incorrect.
An inline with "0" offsets and an inline with no offsets are different. Be more careful about passing offsets around and rendering them.
Test Plan:
- Used the line numbers to add an inline to lines 4-8 of a change.
- Hovered the inline.
- Saw all four lines marked as "dull"-highlighted (previously: three bright lines, one dull line).
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21252
Summary:
Ref T13513. When a user selects a text range and uses "New Inline Comment" to create a comment directly from a range, store the offset information alongside the comment.
When hovering the comment, highlight the original range.
Test Plan: {F7480926, size=full}
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21250
Summary: Ref T11401. Ref T13513. This paves the way for more comment actions, particularly an edit-after-submit action.
Test Plan: Took all actions from menus, via mouse and via keyboard (where applicable).
Maniphest Tasks: T13513, T11401
Differential Revision: https://secure.phabricator.com/D21244
Summary:
Ref T13513. As part of inline metadata, save the document engine the change is being rendered with.
This will allow other parts of the UI to detect that an inline was created on a Jupyter notebook but is being rendered on raw source, or whatever else.
The immediate goal is to fix nonsensical inline snippet rendering in email on Jupyter notebooks.
Test Plan:
- Created inlines and replies on normal soure code, saw no document engine annotated in the database.
- Created inlines and replies on a Jupyter notebook rendered in Jupyter mode, saw "jupyter" annotations in the database.
- Swapped document engines between Jupyter and Source, etc.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21240
Summary: Ref T13513. Currently, if you're editing a comment, "delete" doesn't put the comment into the correct state. This action is normally only reachable from comment previews, since an editing inline has no "delete" button.
Test Plan:
- Started editing an inline, clicked "Delete", got a deletion.
- Created an inline, typed text,
- Deleted a normal comment via preview.
- Deleted a normal comment via the on-inline action.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21238
Summary:
Ref T13513. Overloading "original text" to get "edit-on-load" comments into the right state has some undesirable side effects.
Instead, provide the text when the editor opens. This fixes a cancel interaction.
Test Plan:
- Create an inline, type text, don't save.
- Reload page.
- Cancel.
- Before: cancelled into empty state.
- After: cancelled into deleted+undo state.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21219
Summary:
Ref T13513. This is a bit clumsy, but the cleanest way to implement "isEditing" inlines today is to send them down as normal inlines and then simulate clicking "edit" on them.
When we do, don't focus the resulting editor: focusing it makes the page scroll around and highlight things in essentially random order as the editors load in.
Test Plan: Reloaded a page with some open editors, wasn't scrolled to them.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21217
Summary:
Ref T13513. As users type text into inline comments, save the comment state as a draft on the server.
This has some rough edges, particularly around previews, but mostly works. See T13513 for notes.
Test Plan: Started an inline, typed some text, waited a second, reloaded the page, saw an editing inline with the saved text.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21216
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
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
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
Summary:
Ref T13455. Update the other "view state" properties to work like "highlight" now works.
Some complexity here arises from these concerns:
- In "View Standalone", we render the changeset inline. This is useful for debugging/development, and desirable to retain.
- In all other cases, we render the changeset with AJAX.
So the client needs to be able to learn about the "state" properties of the changeset on two different flows. Prior to this change, each pathway had a fair amount of unique code.
Then, some bookkeeping issues:
- At inital rendering time, we may not know which renderer will be selected: it may be based on the client viewport dimensions.
- Prior to this change, the client didn't separate "value of the property for the changeset as rendered" and "desired value of the property".
Test Plan:
- Viewed changes in Differential, Diffusion, and in standalone mode.
- Toggled renderer, character sets, and document engine (this one isn't terribly useful). Reloaded, saw them stick.
- Started typing a comment, cancelled it, hit the undo UI.
Maniphest Tasks: T13455
Differential Revision: https://secure.phabricator.com/D21138
Summary: Ref T12733. In this case, the server returns no new `row`, but we would incorrectly try to bind to one.
Test Plan:
- Created a new comment.
- Edited it.
- Deleted all the text.
- Saved changes.
- Before: Header continues to show phantom "1 Unsubmitted Comment", browser error log reflects one error.
- After: Header reflects comment being deleted, error log is quiet.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D18262
Summary:
Fixes T8909. Ref T12733.
UI attempts to follow the mock, but is a bit rough since PHUIXButtonView without text in this menu gets weird spacing, we don't have circular buttons yet, and PHUIXActionView without an icon also gets odd spacing.
Test Plan: {F5003125}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733, T8909
Differential Revision: https://secure.phabricator.com/D18128
Summary:
Ref T12733. When a revision has unsubmitted checkmarks:
- Color the banner yellow.
- Show them in the "X unsubmitted" count.
- Make the "X unsubmitted" button cycle between all drafts (written but unpublished comments) and "draft done" (checked but unsubmitted "Done" checkbox comments).
Test Plan:
- Checked a "Done" box, saw "1 unsubmitted" and yellow banner.
- Clicked "5 unsubmitted" repeatedly, saw it cycle through all unsubmitted comments and checkboxes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D18127
Summary:
Ref T12733. This paves the way for a separate "hide" operation which completely hides things.
(I didn't extend this to the server side because that would require schema changes and the new "hide" state is client-only.)
Test Plan: Collapsed and expanded inlines, viewed tooltips.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D18126
Summary:
Fixes T12806. Ref T12733.
- Don't count synthetic (lint) comments as anything.
- When you begin writing an inline then cancel it, don't count it as anything.
- When we would show "0 / X", just show "X".
Test Plan:
- Viewed a diff with synthetic comments, no button.
- Wrote, then cancelled an inline. No "X comments".
- Clicked / unlicked "Done", saw "X" -> "1 / X".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12806, T12733
Differential Revision: https://secure.phabricator.com/D18103
Summary: Ref T12733. This adds classes for unsubmitted/unsaved changes, and lays some groundwork for additional buttons.
Test Plan:
- Added, edited, deleted comments.
- Saw bar background color update appropriately.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D18045
Summary: Ref T12733. Completely removes the objectives UI.
Test Plan:
- Grepped for `objective`, etc.
- Browsed revisions, no JS errors / broken stuff.
- (If I missed anything, it's likely to turn up in followup changes.)
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D18043
Summary: Ref T12733. When an inline is selected, make it stand out so you can see where you are in the document more clearly.
Test Plan: {F4968509}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D17982
Summary: Ref T12733. Show a "reply" icon for replies, and make them stack directly under their parent.
Test Plan: {F4968500}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D17981
Summary: Ref T12733. Shows a comment snippet when hovering inlines in the objective list.
Test Plan: {F4968490}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D17980
Summary: Ref T12733. Currently, creating a new inline and then canceling it leaves a marker in the objective list. Instead, remove the marker.
Test Plan:
- Created an empty inline, cancelled. Created a non-empty inline, cancelled. No objective marker in either case.
- Created a new normal inline, objective marker.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D17979
Summary:
Ref T12733. When creating a new comment, the objective could appear to far up in the scrollbar because we were anchoring it to an invisible row.
Instead, anchor to the "edit" row while editing.
Test Plan: Created a new comment at the very top of a file, saw "File, Star" icons instead of "Star, File".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D17978
Summary:
Ref T12733.
- While editing a comment, show a pink star ({icon star, color=pink}) with a tooltip.
- Slight UI tweaks, including draft comments getting an indigo pencil ({icon pencil, color=indigo}).
Test Plan: {F4968470}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D17977
Summary:
Add important objectives (like waygates and quest markers) to the minimap.
This also probably fixes @cspeckmim's bug with the {key @} keyboard shortcut.
Test Plan:
(This is probably easier to undestand if you `arc patch` + click around.)
{F4966037}
Reviewers: chad, amckinley
Reviewed By: chad
Subscribers: cspeckmim
Differential Revision: https://secure.phabricator.com/D17955
Summary:
Fixes T10563. If you have a thread like this:
```
> A
> B
> C
```
...and you reply to "B", we should put the new inline below "B".
We currently do when you reload the page, but the initial edit goes at the bottom always (below "C").
Test Plan: {F4963015}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10563
Differential Revision: https://secure.phabricator.com/D17938
Summary:
Ref T11401. Fixes T5232. Ref T12616.
Partly, this moves more code over to the new stuff.
This also allows "r" to work if you have code selected (not just comments). If you "reply" to code, you start a new comment.
You can "R" a comment to quote it. This just starts a new comment normally if you "R" a block of code. This is sort of a power-user version of "quote" since it seems like it probably doesn't really make sense to put it in the UI ever (maybe).
With the new click-to-select, you can click + "R" to reply-with-quote.
Test Plan: Used "r" and "R" to reply to comments and code.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616, T11401, T5232
Differential Revision: https://secure.phabricator.com/D17920
Summary:
Fixes T8047. Ref T12616. Fixes T9270. This moves the "hover" part of the hover/drag behavior to the new code, leaving the "drag" part for a followup change.
The new hover UI behaves properly with Quicksand (T8047) and the filetree (T9270).
Test Plan: Hovered over inlines, saw lines select properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616, T9270, T8047
Differential Revision: https://secure.phabricator.com/D17919
Summary: Ref T12616. The ability to do {nav Edit > Cancel > Undo} to get your text back on inlines got dropped during the conversion. Restore it.
Test Plan:
Created, replied, and edited inlines, typed text, then cancelled. Was able to undo.
Also undid normal deletion.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17916
Summary:
Fixes T8130. Allows selected comments to be shown/hidden (with "q") or marked done/not-done (with "w").
(These key selections are because "qwer" are right next to each other on QWERTY keyboards, and now mean "hide, done, edit, reply".)
Also, allow "N" and "P" to do next/previous inline, including hidden inlines. This makes "q" to hide/show a little more powerful and a little easier to undo.
Test Plan: Used "q", "w", "N" and "P" to navigate and interact with comments.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8130
Differential Revision: https://secure.phabricator.com/D17906
Summary:
Ref T12634. Fixes T8131. Currently, most edit operations (edit, reply, collapse, mark done) lose the keyboard cursor state.
Instead, bind the state more tighlty to the inline object itself (instead of the rows which happen to be in the document), and then do a bit of recalculation to try to keep it selected across edits.
Test Plan:
- Used "n" to select an inline.
- Clicked "Done" checkbox.
- Pressed "n".
- Went to the next inline (previously: lost position in document).
- Behavior is also better for: edit, reply, collapse/expand.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12634, T8131
Differential Revision: https://secure.phabricator.com/D17905
Summary:
Ref T12634. Fixes T12633. These events allow the keyboard reticle to resize properly.
(I expect to possibly hide/disable the reticle in the future during edits, but at least make the behavior sensible for now.)
Test Plan:
- Used "n" to select a block.
- Clicked a line number in that block to start a new inline comment.
- Saw reticle resize properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12634, T12633
Differential Revision: https://secure.phabricator.com/D17904
Summary: Ref T12634. Fixes T10049. Toggling an inline currently leaves the reticle oddly-positioned.
Test Plan:
- Selected a comment with the keyboard.
- Collapsed it.
- Saw reticle behave reasonably.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12634, T10049
Differential Revision: https://secure.phabricator.com/D17903
Summary: Ref T12616. This makes "edit" and "reply" work again.
Test Plan:
Used "e" and "r" to edit and reply.
Also used them in bogus ways and got useful UI feedback.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17895
Summary:
Ref T12616. This moves "reply" to the new stuff and deletes DifferentialInlineEditor, which no longer does anything.
(This breaks some keyboard shortcuts, but I'll rebase D17859 shortly.)
Test Plan: Replied to inlines; things seemed to work properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17894
Summary:
Ref T12616. This makes creating inlines use the new code.
Creation and editing is now slightly more consistent in how it uses nodes. This will simplify the next change (replies), which I ran into some trouble with in an earlier iteration.
Note that this (and other changes in the series) allow you to create and edit multiple inlines simultaneously. This is mostly a feature, although I expect we'll need to lock it down a little bit. I have some UI ideas to help avoid errors.
Test Plan: Created inlines on a single line; on a range of lines; on the same line; multiple inlines at the same time.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17893
Summary:
Ref T12616. This moves the delete actions to the new, more stateful way of doing things.
These are a little tricky because you can click "Delete" on an inline, but you can also click "Delete" from the preview area at the bottom of the page. If you do, the inline you are deleting may or may not be present on the page.
This has a few bugs -- notably, deleting from the preview without interacting with the on-page inline first won't actually delete the on-page inline yet -- but nothing too serious.
Test Plan: Deleted inlines, undid deletion, deleted from preview.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17890
Summary:
Ref T12616. This updates clicking the "Done" checkbox for the new stuff.
This one is pretty clean since the "Done" checkbox doesn't do too much weird magic.
Test Plan: Clicked the box a few times.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17888
Summary:
Ref T12616. This doesn't pull over everything (some UI feedback didn't make it yet, and you can't cancel + undo cancelling edits yet) but editing comments technically works.
This is a little shaky, but feels less shaky than every other approach I've tried, so I think I'm finally on a reasonable track here.
Test Plan: Edited some inline comments.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17887
Summary:
Ref T12616. Fixes T12153. Currently, when you hide inlines, they hide completely and turn into a little bubble on the previous line.
Instead, collapse them to a single line one-by-one. Narrowly, this fixes T12153.
In the future, I plan to make these changes so this feature makes more sense:
- Introduce global "hide everything" states (T8909) so you can completely hide stuff if you want, and this represents more of a halfway state between "nuke it" and "view it".
- Make the actual rendering better, so it says "epriestley: blah blah..." instead of just "..." -- and looks less dumb.
The real goal here is to introduce `DiffInline` and continue moving stuff from the tangled jungle of a million top-level behaviors to sensible smooth statefulness.
Test Plan:
- Hid and revealed inlines in unified and two-up modes.
- These look pretty junk for now:
{F4948659}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616, T12153
Differential Revision: https://secure.phabricator.com/D17861
