1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-27 16:00:59 +01:00
Commit graph

16678 commits

Author SHA1 Message Date
epriestley
b021da71a5 When users click headers to select diff UI elements, don't eat the events
Summary: Ref T13513. Currently, clicking inline or changeset headers eats the click events. This doesn't serve any clear purpose, and means these clicks do not clear text selections from the document, which is unusual.

Test Plan:
  - Selected some text in a diff.
  - Clicked a changeset header to select it.
  - Before patch: text selection and context menu were retained.
  - After patch: text selection was cleared and context menu was dismissed.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21255
2020-05-14 14:34:38 -07:00
epriestley
42f1a95a12 Fix a flash of document selection when "oncopy" and "inline on range" behaviors interact
Summary:
Ref T13513. In Safari, do this:

  - view a 2-up diff with content on both sides;
  - select more than one line on the right side; and
  - use your mouse to click "New Inline Comment" in the context menu that pops up.

The mousedown event for the "New Inline Comment" click removes the "copy selection" behavior and creates a flash where both sides of the diff are selected.

This doesn't happen with (most) normal clicks because mouse down on a non-grabbable element clears the document selection.

To avoid this, don't reset the copy selection behavior if the user mouses down on an "<a />". This might not be robust, but seems simple and plausible as a fix.

Test Plan:
  - See above.
  - Before patch: flash of overbroad selection when clicking "New Inline Comment".
  - After patch: no selection flash.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21254
2020-05-14 14:29:46 -07:00
epriestley
f45519c060 When cancelling an inline comment edit, exit the edit state after the response arrives
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
2020-05-14 14:28:20 -07:00
epriestley
cfb5de6fa7 Distinguish more carefully between "null" inline offsets and "0" inline offsets
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
2020-05-14 14:26:54 -07:00
epriestley
2f5398796e Store inline comment offset information and show it when highlighting comments
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
2020-05-13 17:21:53 -07:00
epriestley
ebef22ccc1 Improve select-to-comment behavior in Firefox and on unified diffs
Summary:
Ref T13513.

  - Firefox represents multiple selected rows as a discontinuous range. Accommodate this.
  - Unified diffs don't have a "copy" marker. Do something sort-of-reasonable for them.

Test Plan:
  - Selected multiple lines of content in Firefox, got an option to add a comment.
  - Selected content in unified mode, got an option to add a comment.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21249
2020-05-13 17:19:53 -07:00
epriestley
42378ea393 Allow users to create inline comments by directly selecting text directly
Summary: Ref T13513. Support direct text selection for inlines. This is currently just an alternate way to get to the same place as using line numbers, but can preserve offset/range information in the future.

Test Plan:
  - Selected some text, hit "c", clicked "New Inline Comment", got sensible comments on both sides of a diff in Safari, Chrome, and (with limitations) Firefox.
  - Caveats: no unified support, doesn't work across lines in Firefox.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21248
2020-05-13 17:15:18 -07:00
epriestley
c063e0e5ec Add "View Raw Remarkup" to inline comments
Summary: Ref T13513. Ref T11401. Support viewing raw remarkup for inlines.

Test Plan: Viewed raw remarkup on inlines.

Maniphest Tasks: T13513, T11401

Differential Revision: https://secure.phabricator.com/D21246
2020-05-13 17:14:20 -07:00
epriestley
419b7ceebb Move inline comment actions into a dropdown menu
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
2020-05-13 17:13:18 -07:00
epriestley
1da54837ea Improve line breaking behavior in Firefox and Chrome under complex conditions
Summary: See <https://github.com/phacility/phabricator/pull/854>. In some situations, `line-break: anywhere` produces better behavior than `word-break: break-all`. It never appears to produce worse behavior.

Test Plan:
  - Break behavior changes if a line contains "<span />" elements caused by syntax highlighting. This CSS adjustment only appears to apply to text with internal "<span />" elements.
  - This specifically impacts certain internal breakpoints adjacent to punctuation, so the test case is highly specific. Generic test cases with latin word characters do not evidence any behavioral changes.
  - This change appears to have no impact on Safari, which uses the better behavior in all cases.
  - Before Patch: In Firefox and Chrome, this specific change breaks awkwardly. There is more room for text to fit on the broken line:

Firefox

{F7480567}

Chrome

{F7480568}

  - After Patch: Firefox and Chrome break the line better. Here's Firefox:

{F7480569}

  - Additional context:

Safari Behavior (Unchanged)

{F7480570}

Chrome with no highlighting (desirable behavior). Firefox does the same thing.

{F7480571}

Also tested other cases, which seem never-worse in any browser.

{F7480574}

Differential Revision: https://secure.phabricator.com/D21247
2020-05-13 11:54:42 -07:00
epriestley
3dea92081b Fix an issue where passphrase-protected private keys were stored without discarding passphrases
Summary:
Ref T13454. See <https://discourse.phabricator-community.org/t/newly-created-ssh-private-keys-with-passphrase-not-working-anymore/3883>.

After changes to distinguish between invalid and passphrase-protected keys, SSH private key management code incorrectly uses "-y ..." ("print public key") when it means "-p ..." ("modify input file, removing passphrase"). This results in the command having no effect, and Passphrase stores the raw input credential, not the stripped version.

We can't recover the keys because we don't store the passphrase, so no migration here is really possible. (We could add more code to detect this case, but it's presumably rare.)

Also, correct the behavior of the "Show Public Key" action: this is available for users who can see the credential and does not require edit permission.

Test Plan:
  - Created a new credential with a passphrase, then showed the public key.

Maniphest Tasks: T13006, T13454

Differential Revision: https://secure.phabricator.com/D21245
2020-05-13 08:14:37 -07:00
epriestley
df139f044b Render proper "Show Context" links in DocumentEngine diffs, not just bullets
Summary:
Ref T13513. Currently, viewing a Jupyter document, hidden context just gets a plain "* * *" facade with no way to expand it.

Support click-to-expand, like source changes.

Test Plan:
  - Clicked to expand various Jupyter diffs.
  - Clicked to expand normal source changes.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21243
2020-05-12 16:09:22 -07:00
epriestley
e8109e4a92 When an inline was left on a rendered DocumentEngine document, don't include an email context patch
Summary:
Ref T13513. If you leave an inline on line 20 of a Jupyter document, we currently render context around *raw* line 20, which is inevitably some unrelated piece of JSON.

Instead, drop this context. (Ideal behavior would be to render context around Jupyter block 20, but that's a whole lot of work.)

Test Plan:
  - On Jupyter changes and normal source changes, made and submitted inline comments, then viewed text and HTML mail.
  - Saw no context on Jupyter comments (instead of bad context), and unchanged behavior (useful context) on normal source changes.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21242
2020-05-12 14:34:33 -07:00
epriestley
acc1fa1655 Make "View as Document Type..." only show valid options
Summary:
Ref T13513. Currently, "View as Document Type..." lists every available engine.

This is hard to get completely right because we can't always rebuild the document ref accurately in the endpoint, but try harder to fake something reasonable.

Test Plan: Used "View as Document Type..." on Jupyter notebooks, was given "Jupyter" and "Source" as options.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21241
2020-05-12 14:25:37 -07:00
epriestley
0cca40db3b When creating an inline, save the current document engine
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
2020-05-12 14:25:09 -07:00
epriestley
6dc20d1e2e Fix an issue where storage inlines are fed to InlineAdjustmentEngine
Summary:
Ref T13513. If an intradiff has at least one unchanged file ("hasSameEffectAs()") or more than 100 files ("Large Change"), we hit this block and don't upcast storage inlines to runtime inlines. I missed this in testing.

Add the conversion step.

Test Plan: Viewed an intradiff with at least one unchanged file and at least one inline comment, saw correct rendering instead of fatal.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21239
2020-05-08 16:55:54 -07:00
epriestley
e7ebd5d9d1 Make "Delete" from inline comment previews function correctly while editing comments
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
2020-05-08 08:54:48 -07:00
epriestley
b804e8cffa Make "View" from inline comment previews correctly jump to "isEditing" inlines
Summary:
Ref T13513. Currently, clicking "View" from the inline comment preview (below the "add comment" area at the bottom of the page) only works if the inline isn't being edited.

Update this behavior so it works on inlines in either "Viewing" or "Editing" states.

Test Plan:
  - Clicked "View" on a normal inline, got jumped/selected.
  - Clicked "View" on an editing inline, got jumped/selected.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21237
2020-05-08 08:52:42 -07:00
epriestley
24ba66f106 Persist "Show Changeset" and improve path text selection
Summary:
Ref T13513. Currently:

  - If you click the "Show Changeset" button, your state change doesn't actually get saved on the server.
  - It's hard to select a changeset path name for copy/paste because the "highlight the header" code tends to eat the event.

Instead: persist the former event; make the actual path text not be part of the highlight hitbox.

Test Plan:
  - Clicked "Show Changeset", reloaded, saw changeset visibility persisted.
  - Selected changeset path text without issues.
  - Clicked non-text header area to select/deselect changesets.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21236
2020-05-08 06:59:10 -07:00
epriestley
fa2d30ee36 Lift inline comment draft behaviors to "InlineController"
Summary:
Ref T13513. Currently, if you:

  - click a line to create an inline;
  - type some text;
  - wait a moment; and
  - close the page.

...you don't get an "Unsubmitted Draft" marker in the revision list.

Lift all the draft behavior to "InlineController" and make saving a draft dirty the overall container draft state.

Test Plan:
  - Took the steps described above, got a draft state marker.
  - Created, edited, submitted, etc., inlines in Diffusion and Differential.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21235
2020-05-08 06:52:29 -07:00
epriestley
94a95efa05 Replace "loadUnsubmittedInlineComments()" with a modern "DiffQuery"
Summary: Ref T13513. All queries now go through a reasonably minimal set of pathways and should have consistent behavior.

Test Plan:
- Loaded a revision with inlines.
- Created a new empty inline, reloaded page, saw it vanish.
- Created a new empty inline, typed draft text, did not save, reloaded page, saw draft present.
- Created a new empty inline, typed draft text. Submitted feedback, got prompt, answered "Y", saw draft text submit.
- Created a new empty inline, typed draft text, scrolled down to bottom of page, typed non-draft text, saw preview include draft text.
- Marked and submitted "Done".
- Used hide/show on inlines, verified state persisted.
- Did much of the same stuff in Diffusion, where it all works the same way (except: there's no prompt when submitting draft is-editing inlines).

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21234
2020-05-07 16:11:02 -07:00
epriestley
79107574a7 Remove "DifferentialInlineCommentQuery"
Summary: Ref T13513. Replaces "DifferentialInlineCommentQuery" with the similar but more modern "DifferentialDiffInlineCommentQuery".

Test Plan: Viewed comments in timeline, changesets. Created, edited, and submitted comments. Hid and un-hid comments, reloading (saw state preserved).

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21233
2020-05-07 16:07:55 -07:00
epriestley
983d77848b Move the "Inline List" view to "DiffInlineCommentQuery"
Summary: Ref T13513. Continue removing usage sites for the obsolete "DifferentialInlineCommentQuery".

Test Plan: Viewed the inline list in Differential, saw sensible inlines.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21232
2020-05-07 16:07:23 -07:00
epriestley
af5b94b234 Lift most "InlineController" querying to the base class
Summary: Ref T13513. Move querying to "DiffInlineCommentQuery" classes and lift them into the base Controller.

Test Plan: In Differential and Diffusion, created, edited, and submitted inline comments.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21231
2020-05-07 16:04:51 -07:00
epriestley
949b9163d0 Replace remaining pseudo-query methods on AuditInlineComment
Summary: Ref T13513. Another step closer to the light.

Test Plan: Created, edited, deleted, replied to, and submitted inline comments in Diffusion.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21230
2020-05-07 16:04:27 -07:00
epriestley
6c59db20a3 Replace "loadDraftComments()" with a Query
Summary: Ref T13513. Take another step toward coherent query pathways for inlines.

Test Plan:
  - Created, previewed, and submitted inlines in Diffusion.
  - Got a (mostly) appropriate draft state.
  - Got proper comment peristence, preview behavior, and submission behavior.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21229
2020-05-07 16:02:35 -07:00
epriestley
d0593a5a78 Replace "loadDraftAndPublishedComments()" with a Query
Summary: Ref T13513. Continue marching toward coherent query pathways for all access to inline comments.

Test Plan:
  - Viewed a commit and a path within that commit, as a user with unpublished inlines and a different user.
  - Saw appropriate inlines in all cases (published inlines, plus undeleted unpublished inlines authored by the current viewer).
  - Grepped for "loadDraftAndPublishedComments()".

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21228
2020-05-07 16:02:10 -07:00
epriestley
c1f1345cc0 Make InlineCommentQueries more robust/consistent
Summary:
Ref T13513. Improve consistency and robustness of the "InlineComment" queries.

The only real change here is that these queries now implicitly add a clause for selecting inlines ("pathID IS NULL" or "changesetID IS NULL").

Test Plan: Browed, created, edited, and submitted inlines.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21227
2020-05-07 16:00:28 -07:00
epriestley
1656a2ff08 Allow inline comment storage objects to generate their own runtime objects
Summary:
Ref T13513. Currently, inline storage objects ("TransactionComment") can't directly generate a runtime object ("InlineComment").

Allow this transformation to be performed in a genric way so clunky code which does it per-object-type can be removed, lifted, or simplified.

Simplify an especially gross callsite in preview code.

Test Plan: Previewed inline comments.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21226
2020-05-07 15:57:49 -07:00
epriestley
397648855f Make the "attach_inlines" parameter to "differential.createcomment" a no-op
Summary: Ref T13513. See that task for some discussion. This prepares to lift "loadUnsubmittedInlineComments(...)" into shared code.

Test Plan: Grepped for callers, found none in the upstream. This is a backward compatibilty break. See T13513.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21225
2020-05-07 15:55:37 -07:00
epriestley
0067f1a521 Remove the obsolete "DiffusionInlineCommentPreviewController"
Summary: Ref T13513. This controller was obsoleted by EditEngine and appears unreachable without explicitly typing the URL.

Test Plan:
  - Grepped for the route, didn't find any hits.
  - Deleted the controller, successfully previewed comments in Diffusion.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21224
2020-05-06 10:13:28 -07:00
epriestley
a590db28b2 Fix an issue where non-ID changeset state keys were used as changeset IDs
Summary:
Ref T13519. This is a little fuzzy, but I think the workflow here is:

  - View an intradiff, generating an ephemeral comparison changeset with no changeset ID. This produces a state key of "*".
  - Apply "hidden" state changes to the changeset.
  - View some other intradiff and/or diff view.
  - The code attempts to use "*" as a changset ID?

I'm not entirely sure this is accurate; this was observed in production and I couldn't get a clean reproduction case locally.

Optimistically, try making changeset IDs explicit rather than relying on state keys to be "usually changeset-ID-like".

Test Plan: Used "hidden" locally across multiple intradiffs, but I wasn't cleanly able to reproduce the initial issue.

Maniphest Tasks: T13519

Differential Revision: https://secure.phabricator.com/D21223
2020-05-04 16:05:05 -07:00
epriestley
6b69102990 Fix a JS issue when the anchor element on a page has no container
Summary: See D21213. If there's no matching element, `findAbove()` throws. Handle these cases correctly.

Test Plan: Visited `#toc` on a revision, no longer saw a JS error.

Differential Revision: https://secure.phabricator.com/D21222
2020-05-04 15:57:31 -07:00
epriestley
6430d6d638 Fix an intradiff error when the newer changeset does not exist
Summary: Ref T13523. If a file hasn't been touched in the newer changeset, we can currently hit an error in the interdiff.

Test Plan:
  - Touched "moo.txt" in Diff 1.
  - Reverted the changes to "moo.txt" in Diff 2.
  - Diffed 2 vs 1.
  - Before patch: fatal (call to getFilename() on null).
  - After patch: clean interdiff.

Maniphest Tasks: T13523

Differential Revision: https://secure.phabricator.com/D21220
2020-05-04 15:42:34 -07:00
epriestley
07e160bde1 When cancelling an unsaved editing inline after a reload, don't cancel into an empty state
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
2020-05-04 15:20:31 -07:00
epriestley
7fa47408a3 When a user submits "isEditing" inlines and chooses to publish them, publish their current draft state as-shown
Summary: Ref T13513. When users choose to publish inlines, we want to publish the visible text, not the last "checkpointed" state.

Test Plan:
  - Created an inline ("AAA").
  - Edited it into "BBB", did not save.
  - Submitted.
  - Confirmed that I want to publish the unsaved inline.
  - Saw "BBB" publish.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21218
2020-05-04 15:16:18 -07:00
epriestley
3a76248071 When loading a page with inlines, don't select/focus inlines which we immediately upgrade to "editing"
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
2020-05-04 15:15:27 -07:00
epriestley
fe501bd7f7 Save drafts for inline comments currently being edited
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
2020-05-04 13:19:42 -07:00
epriestley
27b7ba814a Don't consider empty inlines when considering whether a revision has draft comments or not
Summary: Ref T13513. When computing whether a revision has draft comments or not, ignore empty inlines.

Test Plan: Added empty inlines to a revision, no longer saw a yellow "draft" bubble in the list UI.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21215
2020-05-04 13:17:20 -07:00
epriestley
9307f57747 When rendering changesets, discard empty draft inline comments
Summary: Ref T13513. When you load a changeset, discard all empty inlines. This is likely a more desirable behavior than keeping empty editors around, even though the rest of the pipeline generally handles them fairly well now.

Test Plan:
  - Started an inline, didn't type any text or save, reloaded page.
    - Before: page restores empty editor in the same place.
    - After: we just discard this likely-pointless empty inline.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21214
2020-05-04 13:16:42 -07:00
epriestley
63bfad0ff4 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 13:15:01 -07:00
epriestley
f5ef341c9e Don't publish "empty" inline comments
Summary:
Ref T13513. Currently, if you start an inline and then submit overall comments, we publish an empty inline. This is literally faithful to what you did, but almost certainly not the intent.

Instead, simply ignore empty inlines at publishing time (and ignore "done" state changes for those comments).

We could delete them outright, but if we do, they'll break if you have another window open with the empty inline (since the stored comment won't exist anymore). At least for now, leave them in place.

Test Plan: Created empty inlines, submitted comments, no longer saw them publish.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21211
2020-05-04 13:14:04 -07:00
epriestley
67da18e374 When users submit "editing" inlines, warn them that their inlines will be saved
Summary: Ref T13513. This slightly expands the existing-but-hacky "warning" workflow to cover both "mentions on draft" and "submitting inlines being edited".

Test Plan:
  - Submitted changes to a revision with mentions on a draft, inlines being edited, both, and neither.
  - Got sensible warnings in the cases where warnings were appropriate.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21191
2020-05-04 13:13:15 -07:00
epriestley
468aabd4ef When draft inline comments are submitted, disengage the editor
Summary:
Ref T13513. If you submit top-level comments while an inline comment editor is open, kick the comment out of the editing state.

(An improvement to this behavior would be to warn the user that we're going to do this first, but this is currently less straightforward.)

Test Plan:
  - Clicked a line number to create an inline.
  - Type text, save, click edit.
  - (Optional: reload page.)
  - Save changes overall using the form at the bottom of the page.
  - Outcome: published inline is no longer in an "editing" state.

Weirdness:

  - If you click a line number (and, optionally, type text), then submit without using "Save", the server-side version of the inline has no content.
    - This gives you a no-effect warning. Instead, these inlines should probably just be marked as deleted somewhere in the pipeline.
  - This saves the last "Saved" copy of the inline. That's (probably?) desired, but somewhat destructive without a warning.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21188
2020-05-04 13:12:04 -07:00
epriestley
b2ce0844b6 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-05-04 13:11:23 -07:00
epriestley
b48a22bf50 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-05-04 13:10:30 -07:00
epriestley
5ff0ae7d48 Add generic "attributes" storage to inline comment tables
Summary: Ref T13513. This plans for "currently editing", character range comments, code suggestions, document engine tracking. And absolutely nothing else.

Test Plan:
  - Ran `bin/storage upgrade -f`, got a clean upgrade.
  - Created and submitted some inline comments; nothing exploded.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21184
2020-05-04 13:09:55 -07:00
epriestley
54ec566281 Restore highlighting when jumping to transactions using URI anchors
Summary:
At some point, the highlighting behavior for the timeline broke. When you follow a link to a particular timeline story, the story should be highlighted.

Prior to this change, the `<a />` tag itself highlights, but there's no associated CSS and it's too deep in the tree to do anything useful.

(Since this change is fairly straightforward, I gave up digging for the root cause before finding it.)

Test Plan:
  - Clicked a timeline story anchor, saw the story highlight.

Differential Revision: https://secure.phabricator.com/D21213
2020-05-04 10:04:04 -07:00
epriestley
17426a60f0 Fix an issue where text intradiff bodies may not render
Summary:
Ref T13523. In the caching layer, there's a tricky clause about filetypes that skips some body rendering behavior.

Provide file type information which at least has a better chance of representing all changes (e.g., an image file may be replaced with a text file, but this can not be represented by a single file type).

Formalize "hasSourceTextBody()", to mean the changeset parser should engage the change as source text.

Test Plan: Intradiffed text changes, saw the body render properly.

Maniphest Tasks: T13523

Differential Revision: https://secure.phabricator.com/D21210
2020-05-04 07:40:47 -07:00
epriestley
dade977307 Provide a hint about how to quote search terms containing literal colons
Summary: See <https://phabricator.wikimedia.org/T243483>. Provide a more direct path forward if users hit the "unknown function" error but are trying to search for a term with a colon in it.

Test Plan:
{F7414068}

{F7414067}

Differential Revision: https://secure.phabricator.com/D21209
2020-05-03 10:14:47 -07:00