phabricator.git
3 weeks agoUpdate tab completion doc master
Aviv Eyal [Mon, 15 Jun 2020 13:27:18 +0000 (13:27 +0000)]
Update tab completion doc

Test Plan: `aspell -c`

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D21359

4 weeks agoAdd an explicit "uri" to the "harbormaster.buildable.search" results
epriestley [Wed, 10 Jun 2020 17:11:01 +0000 (10:11 -0700)]
Add an explicit "uri" to the "harborbuildable.search" results

Summary: Ref T13546. This makes some "arc" tasks a little easier, and will make them more correct if "arc" ever switches to using SSH.

Test Plan: Ran "harbormaster.buildable.search" from the web UI, saw URIs in the result set.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21346

5 weeks agoCorrect a prose diff behavior when prose pieces include newlines
epriestley [Sat, 30 May 2020 20:31:42 +0000 (13:31 -0700)]
Correct a prose diff behavior when prose pieces include newlines

Summary:
See <https://discourse.phabricator-community.org/t/bad-regex-in-prose-diff-logic/3969>.

The prose splitting rules normally guarantee that newlines appear only at the beginning or end of blocks. However, if a prose sentence ends with text like "...x\n.", we can end up with a newline inside a "sentence".

If we do, the regular expression that breaks it into pieces will fail.

Arguably, this is an error in how sentences are split apart (we might prefer to split this into two sentences, "x\n" and ".", rather than a single "x\n." sentence) but in the general case it's not unreasonable for blocks to contain newlines, so a simple fix is to make the pattern more robust.

Test Plan: Added a failing test which includes this behavior, made it pass.

Differential Revision: https://secure.phabricator.com/D21295

6 weeks agoIn Phortune accounts, prevent self-removal more narrowly
epriestley [Tue, 26 May 2020 14:02:31 +0000 (07:02 -0700)]
In Phortune accounts, prevent self-removal more narrowly

Summary:
Currently, Phortune attempts to prevent users from removing themselves as account managers. It does this by checking that the new list includes them.

Usually this is sufficient, because you can't normally edit an account unless you're already a manager. However, we get the wrong result (incorrect rejection of the edit) if the actor is omnipotent and the acting user was not already a member.

It's okay to edit an account into a state which doesn't include you if you have permission to edit the account and aren't already a manager.

Specifically, this supports more formal tooling around staff modifications to billing accounts, where the actor has staff-omnipotence and the acting user is a staff member and only used for purposes of leaving a useful audit trail.

Test Plan: Elsewhere, ran staff tooling to modify accounts and was able to act as "alice" to add "bailey", even though "alice" was not herself a manager.

Differential Revision: https://secure.phabricator.com/D21288

6 weeks agoFix an issue where inline comments with only edit suggestions are considered empty
epriestley [Sat, 23 May 2020 15:13:41 +0000 (08:13 -0700)]
Fix an issue where inline comments with only edit suggestions are considered empty

Summary:
Ref T13513. An inline is not considered empty if it has a suggestion, but some of the shared transaction code doesn't test for this properly.

Update the shared transaction code to be aware that application comments may have more complex emptiness rules.

Test Plan:
  - Posted an inline with only an edit suggestion, comment went through.
  - Tried to post a normal empty comment, got an appropriate warning.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21287

6 weeks agoWhen cancelling an edit of an inline with content, don't hide the inline
epriestley [Fri, 22 May 2020 22:27:52 +0000 (15:27 -0700)]
When cancelling an edit of an inline with content, don't hide the inline

Summary: See PHI1753. This condition got rewritten for suggested edits and accidentally inverted.

Test Plan:
  - Create a comment, type text, save draft, edit comment, cancel.
  - Before: comment hides itself.
  - After: comment properly cancels into pre-edit draft state.

Differential Revision: https://secure.phabricator.com/D21286

6 weeks agoWhen executing a repository passthru command via CommandEngine, don't set a timeout
epriestley [Fri, 22 May 2020 12:42:34 +0000 (05:42 -0700)]
When executing a repository passthru command via CommandEngine, don't set a timeout

Summary:
Ref T13541. The passthru future does not have time limit behavior, so if we reach this code we currently fail.

Phabricator never reaches this code normally, but this code is reachable during debugging if you try to foreground a slow fetch to inspect it.

Passthru commands generally only make sense to run interactively, and the caller or control script can enforce their own timeouts (usually by pressing "^C" with their fingers).

Test Plan: Used a debugging script to run ref-by-ref fetches in the foreground.

Maniphest Tasks: T13541

Differential Revision: https://secure.phabricator.com/D21284

6 weeks agoFix two rendering issues with Jupyter notebooks
epriestley [Fri, 22 May 2020 18:47:10 +0000 (11:47 -0700)]
Fix two rendering issues with Jupyter notebooks

Summary:
See PHI1752.

  - Early exit of document layout can cause us to fail to populate available rows.
  - Some Jupyter documents have "markdown" cells with plain strings, apparently.

Test Plan: Successfully rendered example diff from PHI1752.

Differential Revision: https://secure.phabricator.com/D21285

7 weeks agoPrevent keyboard selection of change blocks inside edit suggestions
epriestley [Thu, 21 May 2020 22:01:10 +0000 (15:01 -0700)]
Prevent keyboard selection of change blocks inside edit suggestions

Summary: Ref T13513. When a revision has inlines with edit suggestions, pressing "j" and "k" can incorrectly select the blocks inside the diffs inside the inlines.

Test Plan: Used "j" to cycle through changes in a revision with inline comments with edit suggestions, didn't get jumped into the suggestion diffs.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21283

7 weeks agoMake "Open in Editor" use the simple line number of the current selected block
epriestley [Thu, 21 May 2020 21:03:21 +0000 (14:03 -0700)]
Make "Open in Editor" use the simple line number of the current selected block

Summary:
Ref PHI1749. Instead of opening files to the last unchanged line on either side of the change, open files to the "simple" line number of the selected block.

For inlines, this is the inline line number.

For blocks, this is the first new-file line number, or the first old-file line number if no new-file line number exists in the block.

This may not always be what the user is hoping for (we can't know what the state of their working copy is) but should produce more obvious behavior.

Test Plan:
  - In Diffusion, used "Open in Editor" with and without line selections. Saw same behavior as before.
  - Used "n" and "r" to leave an inline with the keyboard, saw same behavior as before.
  - Used "\" and "Open in Editor" menu item to open a file with:
    - Nothing selected or changeset selected (line: 1).
    - An inline selected (line: inline line).
    - A block selected (line: first line in block, per above).

Differential Revision: https://secure.phabricator.com/D21282

7 weeks agoDrop old "differential_commit" table
epriestley [Wed, 20 May 2020 18:53:54 +0000 (11:53 -0700)]
Drop old "differential_commit" table

Summary: Ref T13276. Ref T13513. All readers and writers were removed more than a year ago; clean up the last remnants of this table.

Test Plan: Grepped for table references, found none.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13513, T13276

Differential Revision: https://secure.phabricator.com/D21281

7 weeks agoUse the changeset parse cache to cache suggestion changesets
epriestley [Wed, 20 May 2020 17:59:33 +0000 (10:59 -0700)]
Use the changeset parse cache to cache suggestion changesets

Summary:
Ref T13513. Syntax highlighting is potentially expensive, and the changeset rendering pipeline can cache it. However, the cache is currently keyed ONLY by Differential changeset ID.

Destroy the existing cache and rebuild it with a more standard cache key so it can be used in a more ad-hoc way by inline suggestion snippets.

Test Plan: Used Darkconsole, saw cache hits and no more inline syntax highlighting for changesets with many inlines.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21280

7 weeks agoPut a readthrough cache in front of inline context construction
epriestley [Wed, 20 May 2020 17:32:52 +0000 (10:32 -0700)]
Put a readthrough cache in front of inline context construction

Summary: Ref T13513. Inline comment context information is somewhat expensive to construct and can be cached. Add a readthrough cache on top of it.

Test Plan: Loaded a source code changeset with many inline comments, used Darkconsole to inspect query activity. Saw caches get populated. Updated cache key, saw caches regenerate. Browsed Diffusion, nothing looked broken.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21279

7 weeks agoClean up Diffusion behaviors for inline edit suggestions
epriestley [Wed, 20 May 2020 16:18:18 +0000 (09:18 -0700)]
Clean up Diffusion behaviors for inline edit suggestions

Summary: Ref T13513. For now, I'm not supporting inline edit suggestions in Diffusion, although it's likely not difficult to do so in the future. Clean up some of the code so that plain ol' inlines work the same way they did before.

Test Plan:
  - Created, edited, reloaded, submitted inlines in Diffusion: familiar old behavior.
  - Created, edited, reloaded, submitted inlines with suggestions in Differential: familiar new behavior.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21278

7 weeks agoRender inline comment suggestions as real diffs
epriestley [Wed, 20 May 2020 16:01:23 +0000 (09:01 -0700)]
Render inline comment suggestions as real diffs

Summary: Ref T13513. When rendering an inline suggestion for display, use highlighting and diffing.

Test Plan: {F7495053}

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21277

7 weeks agoRoughly support inline comment suggestions
epriestley [Tue, 19 May 2020 21:50:47 +0000 (14:50 -0700)]
Roughly support inline comment suggestions

Summary:
Ref T13513. This still has quite a few rough edges and some significant performance isssues, but appears to mostly work.

Allow reviewers to "Suggest Edit" on an inline comment and provide replacement text for the highlighted source.

Test Plan: Created, edited, reloaded, and submitted inline comments in various states with and without suggestion text.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21276

7 weeks agoMake server components of inline comment content handling state-oriented
epriestley [Tue, 19 May 2020 21:50:32 +0000 (14:50 -0700)]
Make server components of inline comment content handling state-oriented

Summary: Ref T13513. Introduce a formal server-side content state object so the whole state can be saved and restored to the drafts table, read from the request, etc.

Test Plan: Created and edited inlines. Reloaded drafts with edits. Submitted normal and editing comments. Grepped for affected symbols.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21275

7 weeks agoAllow "has draft inlines?" queries to overheat
epriestley [Tue, 19 May 2020 20:52:14 +0000 (13:52 -0700)]
Allow "has draft inlines?" queries to overheat

Summary:
Ref T13513. If your 10 most recently authored inlines have all been deleted, these queries can fail by overheating. This is silly and probably rarely happens outside of development.

For now, just let them overheat. This may create a false negative (incorrect "no draft" signal when the real condition is "drafts, but 10 most recent comments were deleted"). This could be sorted out later with a query mode like "executeAny()", perhaps.

Test Plan:
  - Created and deleted 10 inlines.
  - Submitted comments.
  - Before: overheating fatal during draft flag generation.
  - After: clean submission.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21274

7 weeks agoMake inline content "state-oriented", not "string-oriented"
epriestley [Tue, 19 May 2020 20:46:58 +0000 (13:46 -0700)]
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

7 weeks agoRemove PHPMailer code which generates bogus "Message-ID" email headers
epriestley [Fri, 15 May 2020 13:20:28 +0000 (06:20 -0700)]
Remove PHPMailer code which generates bogus "Message-ID" email headers

Summary:
See <https://discourse.phabricator-community.org/t/how-to-override-localhost-localdomain-in-email-message-id/3876/>.

Currently, Phabricator generates a "Message-ID" only in a subset of cases (roughly: when the message is first-in-thread and we expect the thread may have more than one message).

In cases where it does not generate a message ID, it expects the SMTP server to generate one for it. Servers will generally do this, and some ONLY do this (that is, they ignore IDs from Phabricator and replace them). Thus, several pieces of configuration control whether Phabricator attempts to generate a "Message-ID" at all.

The PHPMailer code has fallback behavior which generates a "<random>@localhost.localdomain" message ID. This is never desirable and ignores Phabricator-level configuration that Message IDs should not be generated.

For now, remove this code: it is never the desired behavior and sometimes explicitly contradicts the intent of configuration.

Possibly, a better change may be to make Phabricator always generate a message ID in cases where it isn't forbidden from doing so by configuration. However, that's a more complicated change and it's not clear if/when it would produce better behavior, so start here for now.

Test Plan: Confirmed by affected user (see linked thread).

Differential Revision: https://secure.phabricator.com/D21272

7 weeks agoTreat PHP7 "Throwable" exceptions like other unhandled "Exception" cases in the worke...
epriestley [Tue, 19 May 2020 17:36:43 +0000 (10:36 -0700)]
Treat PHP7 "Throwable" exceptions like other unhandled "Exception" cases in the worker queue

Summary: See PHI1745. Under PHP7, errors raised as Throwable miss this "generic exception" logic and don't increment their failure count. Instead, treat any "Throwable" we don't recognize like any "Exception" we don't recognize.

Test Plan:
  - Under PHP7, caused a worker task to raise a Throwable (e.g., call to undefined method, see D21270).
  - Ran `bin/worker execute --id ...`.
  - Before: worker failed, but did not increment failure count.
  - After: worker fails and increments failure count as it would for other types of unknown error.

Differential Revision: https://secure.phabricator.com/D21271

7 weeks agoUpdate out-of-date API calls when rendering diffs inline in email
epriestley [Tue, 19 May 2020 17:29:43 +0000 (10:29 -0700)]
Update out-of-date API calls when rendering diffs inline in email

Summary: See PHI1745. This callsite for "ChangesetParser" was not properly updated for recent changes.

Test Plan:
  - Set `metamta.differential.inline-patches` to 100.
  - Created a new revision with a small (<100 line) diff, with at least one reviewer.
  - Ran `bin/phd debug` and observed outbound mail queue with `bin/mail list-outbound`.
  - Before: fatal when trying to generate the inline changes for mail.
  - After: clean mail generation.

Differential Revision: https://secure.phabricator.com/D21270

7 weeks agoFix an issue where builds with no initiator failed to render in build plans
epriestley [Tue, 19 May 2020 16:43:01 +0000 (09:43 -0700)]
Fix an issue where builds with no initiator failed to render in build plans

Summary: See PHI1743. If a build has no initiator PHID, the rendering pathway incorrectly tries to access a handle for it anyway.

Test Plan:
  - Set a build to have no initiator PHID.
  - Viewed the build plan for the build.
  - Before: fatal when trying to access the `null` handle.
  - After: clean build plan rendering.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D21269

7 weeks agoFix "r" and "R" both replying with quote on inline comments
epriestley [Tue, 19 May 2020 16:25:09 +0000 (09:25 -0700)]
Fix "r" and "R" both replying with quote on inline comments

Summary: Ref T13513. The code which added "r" and "R" to the inline menu accidentally discarded the difference between the keystrokes.

Test Plan:
  - Clicked an inline, pressed "r", got new empty inline (previously: inline with quote).
  - Clicked an inline, pressed "R", got a new quoted inline.
  - Repeated steps with the menu items, got the expected behaviors.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21268

7 weeks agoFix an unusual issue with intradiff highlighting of files with uncommon end-of-file...
epriestley [Tue, 19 May 2020 15:57:42 +0000 (08:57 -0700)]
Fix an unusual issue with intradiff highlighting of files with uncommon end-of-file modifications

Summary:
Fixes T13539. See that task for discussion and a reproduction case.

This algorithm currently counts "\ No newline at end of file" lines as though they were normal source lines. This can cause offset issues in the rare case that a diff contains two of these lines (for each side of the file) and has changes between them (because the last line of the file was modified between the diffs).

Instead, don't count "\" as a display line.

Test Plan:
  - See T13539 and PHI1740.
  - Before: got fatals on the "wild" diff and the synthetic simplified version.
  - After: clean intradiff rendering in both cases.

Maniphest Tasks: T13539

Differential Revision: https://secure.phabricator.com/D21267

7 weeks agoSurvive importing Git commits with no commit message and/or no author
epriestley [Tue, 19 May 2020 02:55:48 +0000 (19:55 -0700)]
Survive importing Git commits with no commit message and/or no author

Summary:
Ref T13538. See PHI1739. Synthetic Git commits with no author and/or no commit message currently extract `null` and then fail to parse.

Ideally, we would carefully distinguish between `null` and empty string. In practice, that requires significant schema changes (these columns are non-nullable and have indexing requirements) and these cases are degenerate. These commits are challenging to build and can not normally be constructed with `git commit`.

At least for now, merge the `null` cases into the empty string cases so we can survive import.

Test Plan:
  - Constructed a commit with no author and no commit message using the approach described in T13538; pushed and parsed it.
  - Before: fatals during identity selection and storing the commit message (both roughly NULL inserts into non-null columns).
  - After: clean import.

This produces a less-than-ideal UI in Diffusion, but it doesn't break anything:

{F7492094}

Maniphest Tasks: T13538

Differential Revision: https://secure.phabricator.com/D21266

7 weeks agoUpdate MySQL schema inspection code for deprecation of integer display widths
epriestley [Mon, 18 May 2020 18:59:26 +0000 (11:59 -0700)]
Update MySQL schema inspection code for deprecation of integer display widths

Summary:
Fixes T13536. See that task for discussion.

Older versions of MySQL (roughly, prior to 8.0.19) emit "int(10)" types. Newer versions emit "int" types. Accept these as equivalent.

Test Plan: Ran `bin/storage upgrade --force` against MySQL 8.0.11 and 8.0.20. Got clean adjustment lists on both versions.

Maniphest Tasks: T13536

Differential Revision: https://secure.phabricator.com/D21265

7 weeks agoFix an email address validation UI feedback issue when creating new users
epriestley [Mon, 18 May 2020 14:32:05 +0000 (07:32 -0700)]
Fix an email address validation UI feedback issue when creating new users

Summary: On the "New User" web workflow, if you use an invalid email address, you get a failure with an empty message.

Test Plan:
  - Before: Tried to create a new user with address "asdf". Got no specific guidance.
  - After: Got specific guidance about email address formatting and length.

Differential Revision: https://secure.phabricator.com/D21264

7 weeks agoFix an out-of-order access issue with inlines
epriestley [Fri, 15 May 2020 20:25:52 +0000 (13:25 -0700)]
Fix an out-of-order access issue with inlines

Summary: Ref T13513. On `secure`, I caught an issue where inlines may be accessed directly before they're constructed. Instead, access them through the relevant accessor.

Test Plan: Will deploy.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21263

7 weeks agoUse a more consistent inline highlighting style with fewer redraws
epriestley [Fri, 15 May 2020 17:05:23 +0000 (10:05 -0700)]
Use a more consistent inline highlighting style with fewer redraws

Summary:
Ref T13513. The on-hover-inline reticle has switched over to have cell-based behavior. Switch the on-hover-line-number reticle to use the same behavior.

Also, clean up the dirty/redraw loop slightly: we no longer need to dirty on resize, and we don't need to redraw if the range isn't actually dirty.

Test Plan: Highlighted lines and line ranges. Hovered over inlines.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21262

7 weeks agoReduce the frequency of DOM scans to rebuild inlines when scrolling revisions
epriestley [Fri, 15 May 2020 15:02:52 +0000 (08:02 -0700)]
Reduce the frequency of DOM scans to rebuild inlines when scrolling revisions

Summary:
Ref T13513. See PHI1734, which raises a concern about the performance of large revisions near the 100-change threshold.

Currently, `getInlines()` is called whenever the scroll position transitions between two changesets, and it performs a relatively complicated DOM scan to lift inlines out of the document.

This shows up as taking a small but nontrivial amount of time in Firefox profiles and should be safely memoizable.

Test Plan:
  - Under Firefox profiling, scrolled through a large revision.
  - Before change: `getInlines()` appeared as the highest-cost thing we're explicitly doing on profiles.
  - After change: `getInlines()` was no longer meaningfully represented on profiles.
  - Created inlines, edited inlines, etc. Didn't identify any broken behavior.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21261

7 weeks agoRemove code which overrides "diffusion.ssh-username" when instanced
epriestley [Fri, 15 May 2020 13:25:36 +0000 (06:25 -0700)]
Remove code which overrides "diffusion.ssh-username" when instanced

Summary:
Ref T13529. Now that instances can be renamed, an instance may have multiple valid SSH usernames and the preferred SSH username may not be the intenal instance name.

`PhacilitySiteSource` should already always set `diffusion.ssh-username` correctly, to the current preferred SSH username (which may be "new-name" after a rename from "old-name"), so we should never be able to reach this code without an accurate `diffusion.ssh-username` value available.

The code to resolve names into instances also already works for both "ssh old-name@..." and "ssh new-name@...".

So I believe this code has no beneficial effects and only causes harm: it may force us to return "old-name" when falling through would correctly return "new-name".

Test Plan:
  - Previously: renamed an instance, then SSH'd to it using both the old and new names. Both work.
  - Previously: verified that `diffusion.ssh-username` is set correctly after a rename.
  - Verified that Diffusion "Clone" UI now shows "new-name" after an instance rename.
  - The real question here is: does this break something I'm not thinking of? And the change probably has to go to production to answer that.

Maniphest Tasks: T13529

Differential Revision: https://secure.phabricator.com/D21259

8 weeks agoImprove offset/range inline behavior for rich diffs and unified diffs
epriestley [Thu, 14 May 2020 22:42:35 +0000 (15:42 -0700)]
Improve offset/range inline behavior for rich diffs and unified diffs

Summary:
Ref T13513. The way I'm highlighting lines won't work for Jupyter notebooks or other complex content blocks, and I don't see an obvious way to make it work that's reasonably robust.

However, we can just ignore the range behavior for complex content and treat the entire block as selected. This isn't quite as fancy as the source behavior, but pretty good.

Also, adjust unified diff behavior to work correctly with highlighting and range selection.

Test Plan:
  - Used range selection in a Jupyter notebook, got reasonable behavior (range is treated as "entire block").
  - Used range selection in a unified diff, got equivalent behavior to 2-up diffs.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21257

8 weeks agoGive selected inline comments are more obvious selected state
epriestley [Thu, 14 May 2020 19:49:29 +0000 (12:49 -0700)]
Give selected inline comments are more obvious selected state

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

8 weeks agoWhen users click headers to select diff UI elements, don't eat the events
epriestley [Thu, 14 May 2020 18:43:55 +0000 (11:43 -0700)]
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

8 weeks agoFix a flash of document selection when "oncopy" and "inline on range" behaviors interact
epriestley [Thu, 14 May 2020 18:34:33 +0000 (11:34 -0700)]
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

8 weeks agoWhen cancelling an inline comment edit, exit the edit state after the response arrives
epriestley [Thu, 14 May 2020 18:17:51 +0000 (11:17 -0700)]
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

8 weeks agoDistinguish more carefully between "null" inline offsets and "0" inline offsets
epriestley [Thu, 14 May 2020 17:53:03 +0000 (10:53 -0700)]
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

8 weeks agoStore inline comment offset information and show it when highlighting comments
epriestley [Wed, 13 May 2020 22:27:11 +0000 (15:27 -0700)]
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

8 weeks agoImprove select-to-comment behavior in Firefox and on unified diffs
epriestley [Wed, 13 May 2020 20:37:42 +0000 (13:37 -0700)]
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

8 weeks agoAllow users to create inline comments by directly selecting text directly
epriestley [Wed, 13 May 2020 18:09:44 +0000 (11:09 -0700)]
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

8 weeks agoAdd "View Raw Remarkup" to inline comments
epriestley [Wed, 13 May 2020 16:24:33 +0000 (09:24 -0700)]
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

8 weeks agoMove inline comment actions into a dropdown menu
epriestley [Wed, 13 May 2020 11:59:04 +0000 (04:59 -0700)]
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

8 weeks agoImprove line breaking behavior in Firefox and Chrome under complex conditions
epriestley [Wed, 13 May 2020 18:16:56 +0000 (11:16 -0700)]
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

8 weeks agoFix an issue where passphrase-protected private keys were stored without discarding...
epriestley [Wed, 13 May 2020 14:59:06 +0000 (07:59 -0700)]
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

8 weeks agoRender proper "Show Context" links in DocumentEngine diffs, not just bullets
epriestley [Tue, 12 May 2020 22:22:41 +0000 (15:22 -0700)]
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

8 weeks agoWhen an inline was left on a rendered DocumentEngine document, don't include an email...
epriestley [Tue, 12 May 2020 20:55:00 +0000 (13:55 -0700)]
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

8 weeks agoMake "View as Document Type..." only show valid options
epriestley [Tue, 12 May 2020 20:24:53 +0000 (13:24 -0700)]
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

8 weeks agoWhen creating an inline, save the current document engine
epriestley [Tue, 12 May 2020 18:37:55 +0000 (11:37 -0700)]
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

2 months agoFix an issue where storage inlines are fed to InlineAdjustmentEngine
epriestley [Fri, 8 May 2020 23:49:14 +0000 (16:49 -0700)]
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

2 months agoMake "Delete" from inline comment previews function correctly while editing comments
epriestley [Fri, 8 May 2020 14:59:18 +0000 (07:59 -0700)]
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

2 months agoMake "View" from inline comment previews correctly jump to "isEditing" inlines
epriestley [Fri, 8 May 2020 14:27:43 +0000 (07:27 -0700)]
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

2 months agoPersist "Show Changeset" and improve path text selection
epriestley [Fri, 8 May 2020 13:36:31 +0000 (06:36 -0700)]
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

2 months agoLift inline comment draft behaviors to "InlineController"
epriestley [Fri, 8 May 2020 12:58:49 +0000 (05:58 -0700)]
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

2 months agoReplace "loadUnsubmittedInlineComments()" with a modern "DiffQuery"
epriestley [Thu, 7 May 2020 20:54:04 +0000 (13:54 -0700)]
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

2 months agoRemove "DifferentialInlineCommentQuery"
epriestley [Thu, 7 May 2020 20:17:56 +0000 (13:17 -0700)]
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

2 months agoMove the "Inline List" view to "DiffInlineCommentQuery"
epriestley [Thu, 7 May 2020 20:02:06 +0000 (13:02 -0700)]
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

2 months agoLift most "InlineController" querying to the base class
epriestley [Thu, 7 May 2020 19:42:14 +0000 (12:42 -0700)]
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

2 months agoReplace remaining pseudo-query methods on AuditInlineComment
epriestley [Thu, 7 May 2020 19:22:29 +0000 (12:22 -0700)]
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

2 months agoReplace "loadDraftComments()" with a Query
epriestley [Thu, 7 May 2020 19:05:25 +0000 (12:05 -0700)]
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

2 months agoReplace "loadDraftAndPublishedComments()" with a Query
epriestley [Thu, 7 May 2020 18:24:08 +0000 (11:24 -0700)]
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

2 months agoMake InlineCommentQueries more robust/consistent
epriestley [Thu, 7 May 2020 17:46:49 +0000 (10:46 -0700)]
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

2 months agoAllow inline comment storage objects to generate their own runtime objects
epriestley [Thu, 7 May 2020 15:42:35 +0000 (08:42 -0700)]
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

2 months agoMake the "attach_inlines" parameter to "differential.createcomment" a no-op
epriestley [Wed, 6 May 2020 18:23:01 +0000 (11:23 -0700)]
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

2 months agoRemove the obsolete "DiffusionInlineCommentPreviewController"
epriestley [Mon, 4 May 2020 23:24:53 +0000 (16:24 -0700)]
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

2 months agoFix an issue where non-ID changeset state keys were used as changeset IDs
epriestley [Mon, 4 May 2020 22:57:22 +0000 (15:57 -0700)]
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

2 months agoFix a JS issue when the anchor element on a page has no container
epriestley [Mon, 4 May 2020 22:48:47 +0000 (15:48 -0700)]
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

2 months agoFix an intradiff error when the newer changeset does not exist
epriestley [Mon, 4 May 2020 22:34:27 +0000 (15:34 -0700)]
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

2 months agoWhen cancelling an unsaved editing inline after a reload, don't cancel into an empty...
epriestley [Mon, 4 May 2020 20:55:43 +0000 (13:55 -0700)]
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

2 months agoWhen a user submits "isEditing" inlines and chooses to publish them, publish their...
epriestley [Mon, 4 May 2020 20:43:10 +0000 (13:43 -0700)]
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

2 months agoWhen loading a page with inlines, don't select/focus inlines which we immediately...
epriestley [Mon, 4 May 2020 20:32:13 +0000 (13:32 -0700)]
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

2 months agoSave drafts for inline comments currently being edited
epriestley [Mon, 4 May 2020 17:52:12 +0000 (10:52 -0700)]
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

2 months agoDon't consider empty inlines when considering whether a revision has draft comments...
epriestley [Mon, 4 May 2020 17:21:25 +0000 (10:21 -0700)]
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

2 months agoWhen rendering changesets, discard empty draft inline comments
epriestley [Mon, 4 May 2020 17:15:09 +0000 (10:15 -0700)]
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

2 months agoRefine unusual inline comment client interactions
epriestley [Mon, 4 May 2020 16:22:23 +0000 (09:22 -0700)]
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

2 months agoDon't publish "empty" inline comments
epriestley [Mon, 4 May 2020 16:22:20 +0000 (09:22 -0700)]
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

2 months agoWhen users submit "editing" inlines, warn them that their inlines will be saved
epriestley [Wed, 29 Apr 2020 19:51:51 +0000 (12:51 -0700)]
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

2 months agoWhen draft inline comments are submitted, disengage the editor
epriestley [Wed, 29 Apr 2020 18:51:56 +0000 (11:51 -0700)]
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

2 months agoWhen a user clicks "Cancel" on an inline comment to leave the "Editing" state, save...
epriestley [Wed, 29 Apr 2020 18:25:43 +0000 (11:25 -0700)]
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

2 months agoMake "editing" state persistent for inline comments
epriestley [Tue, 28 Apr 2020 21:45:54 +0000 (14:45 -0700)]
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

2 months agoAdd generic "attributes" storage to inline comment tables
epriestley [Tue, 28 Apr 2020 20:38:46 +0000 (13:38 -0700)]
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

2 months agoRestore highlighting when jumping to transactions using URI anchors
epriestley [Mon, 4 May 2020 16:57:30 +0000 (09:57 -0700)]
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

2 months agoFix an issue where text intradiff bodies may not render
epriestley [Mon, 4 May 2020 14:35:21 +0000 (07:35 -0700)]
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

2 months agoProvide a hint about how to quote search terms containing literal colons
epriestley [Sun, 3 May 2020 16:32:13 +0000 (09:32 -0700)]
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

2 months agoFix an issue where the "%%%" parser could match too many lines in unterminated blocks
epriestley [Sun, 3 May 2020 16:13:13 +0000 (09:13 -0700)]
Fix an issue where the "%%%" parser could match too many lines in unterminated blocks

Summary: Fixes T13530. The block parser could match too many lines in an unterminated "%%%" literal block. Adjust the logic to stop doing this (and hopefully be a little easier to read).

Test Plan: Added a failing test, made it pass.

Maniphest Tasks: T13530

Differential Revision: https://secure.phabricator.com/D21208

2 months agoProvide detailed information about reviewer changes in "transaction.search"
epriestley [Fri, 1 May 2020 21:05:58 +0000 (14:05 -0700)]
Provide detailed information about reviewer changes in "transaction.search"

Summary:
See PHI1722, which requests transaction details about reviewer changes.

This adds them; they're structured to be similar to "projects" and "subscribers" transactions and the "reviewers" attachment on revisions.

Test Plan: {F7410675}

Differential Revision: https://secure.phabricator.com/D21207

2 months agoAdd "idea://" to the upstream editor whitelist
epriestley [Fri, 1 May 2020 19:51:34 +0000 (12:51 -0700)]
Add "idea://" to the upstream editor whitelist

Summary: This supports the IntelliJ IDEA editor.

Test Plan:
  - Looked at the editor settings panel, saw "idea://".
  - Set my editor pattern to "idea://a?b".
  - (Did not actually install IntelliJ IDEA.)

Differential Revision: https://secure.phabricator.com/D21206

2 months agoUse underlines instead of background color to show file moves/renames
epriestley [Fri, 1 May 2020 19:19:01 +0000 (12:19 -0700)]
Use underlines instead of background color to show file moves/renames

Summary: Ref T13520. This is a style tweak that I think looks a little cleaner.

Test Plan: {F7410424}

Maniphest Tasks: T13520

Differential Revision: https://secure.phabricator.com/D21205

2 months agoAdd "uri" to the API results for File objects
epriestley [Fri, 1 May 2020 16:08:07 +0000 (09:08 -0700)]
Add "uri" to the API results for File objects

Summary: Ref T13528. This supports "arc upload --browse ...".

Test Plan: Called "file.search", saw URIs in results.

Maniphest Tasks: T13528

Differential Revision: https://secure.phabricator.com/D21204

2 months agoMake content more prominent in Files and move some details to the curtain
epriestley [Fri, 1 May 2020 14:38:04 +0000 (07:38 -0700)]
Make content more prominent in Files and move some details to the curtain

Summary: Ref T13528. Now that we're hinting users into Files, put the content first and move the detail panel under it. Move the most-useful details (author, size, dimensions) into the curtain.

Test Plan: {F7409925}

Maniphest Tasks: T13528

Differential Revision: https://secure.phabricator.com/D21201

2 months agoPut application curtain panels above extension curtain panels
epriestley [Fri, 1 May 2020 14:22:55 +0000 (07:22 -0700)]
Put application curtain panels above extension curtain panels

Summary:
Ref T13528. The original rationale here was that it's easier to find items at the bottom of the curtain than somewhere in the middle, since they're in a more clearly predictable visual location.

This might be true in some sense, but user feedback about this has fairly consistently indicated that the layout is surprising. Try the other order. See also D20967 for some discussion.

In practice, this primarily moves "Author / Assigned" above other panel elements in Maniphest.

Test Plan: Looked at tasks, aw "Author / Assigned" above "Tags / Subscribers".

Maniphest Tasks: T13528

Differential Revision: https://secure.phabricator.com/D21200

2 months agoWhen creating a File storage object for a Paste, try to give it the same name as...
epriestley [Fri, 1 May 2020 13:49:04 +0000 (06:49 -0700)]
When creating a File storage object for a Paste, try to give it the same name as the Paste

Summary:
Ref T13528. Paste data is stored in files, but the files are always named "raw.txt".

Now that Paste provides a hint to use Files for "DocumentEngine" rendering, try to use the same name as the paste instead.

Test Plan:
  - Created a paste named "staggering-insight.ipynb".
  - Clicked "View as Jupyter Notebook" from Paste.
  - Saw a file named "staggering-insight.ipynb", not "raw.txt".
  - Created a paste with no name, saw a file named "raw-paste-data.txt" get created.

Maniphest Tasks: T13528

Differential Revision: https://secure.phabricator.com/D21197

2 months agoWhen a Paste has a useful alternative rendering in Files, provide a hint
epriestley [Fri, 1 May 2020 13:00:37 +0000 (06:00 -0700)]
When a Paste has a useful alternative rendering in Files, provide a hint

Summary: Ref T13528. When a file in Paste (like a Jupyter notebook) has a good/useful document engine, provide a link to Files.

Test Plan: {F7409881}

Maniphest Tasks: T13528

Differential Revision: https://secure.phabricator.com/D21196

2 months agoRoute hard-coded "/favicon.ico" requests to a favicon resource
epriestley [Fri, 1 May 2020 12:12:01 +0000 (05:12 -0700)]
Route hard-coded "/favicon.ico" requests to a favicon resource

Summary:
See PHI1719. User agents making hard-coded requests to "/favicon.ico" currently 404. This is a mild source of log noise, and we can reasonably route this request.

Limitations:

  - This only routes the "PlatformSite". Other sites (custom Phame blogs, third-party sites, Phurl redirectors) won't route here for now.
  - This returns a "Location:" redirect to the correct resource rather than icon data directly. This produces the right icon with the right caching behavior, and returning icon data directly is difficult in the general case. However, it won't perform/cache as well as a direct response would.

Test Plan:
  - Visted `/favicon.ico`.
  - Before: 404.
  - After: redirect to favicon.

Differential Revision: https://secure.phabricator.com/D21195

2 months agoStabilize fatals when a build has a build plan the viewer can't see because of policy...
epriestley [Thu, 30 Apr 2020 14:41:50 +0000 (07:41 -0700)]
Stabilize fatals when a build has a build plan the viewer can't see because of policy restrictions

Summary:
Ref T13526. Currently, if a build plan is restricted, viewers may fatal when trying to view related builds.

The old behavior allowed them to see the build even if they can not see the build plan. This is sort of incoherent, but try to stabilize things before fixing this.

Test Plan:
This is a muddy change.

  - Created a build with a build plan that Alice can't see.
  - As Alice, viewed the build page (restricted before, restricted after); the buildable page (fatal before, works after).
  - Also viewed a revision page (works before and after, but user-reported fatal).

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13526

Differential Revision: https://secure.phabricator.com/D21194

2 months agoReplace nonexistent "withPHIDs()" in ChangesetQuery with "withIDs()"
epriestley [Wed, 29 Apr 2020 21:41:28 +0000 (14:41 -0700)]
Replace nonexistent "withPHIDs()" in ChangesetQuery with "withIDs()"

Summary:
Ref T13519. See <https://discourse.phabricator-community.org/t/error-call-to-undefined-method-differentialchangesetquery-withphids/3816/>.

Changesets do not have PHIDs, and the Query has no "withPHIDs()" method. The keys in the viewstate storage are (usually) IDs.

Test Plan:
  - On a revision with Diff 1 and Diff 2 affecting the same file:
    - Viewed Diff 1.
    - Hid file A.
    - Viewed Diff 2.
  - Before patch: exception about call to "withPHIDs()", which does not exist for ChangesetQuery.
  - After patch: no exception. Also, file actually unhid, which is the correct behavior!

Maniphest Tasks: T13519

Differential Revision: https://secure.phabricator.com/D21189

2 months agoFix an issue where the Maniphest burnup chart was trying to render a non-View object
epriestley [Wed, 29 Apr 2020 11:53:41 +0000 (04:53 -0700)]
Fix an issue where the Maniphest burnup chart was trying to render a non-View object

Summary:
See PHI1714. This code is incorrectly rendering the chart panel twice, sort of, and passing a non-View object to rendering.

After D21044, this fatals by raising an exception in rendering.

Test Plan: Loaded page, no more exception.

Differential Revision: https://secure.phabricator.com/D21185

2 months agoUpdate the diff table of contents to use hierarchical views and edit distance renames
epriestley [Tue, 28 Apr 2020 15:58:17 +0000 (08:58 -0700)]
Update the diff table of contents to use hierarchical views and edit distance renames

Summary:
Ref T13520. Generally, make the table of contents look and more like the paths panel:

  - Show a hierarchy, with compression for single-sibling children.
  - Use the same icons, instead of "M D" and "(img)" stuff.
  - Use EditDistanceMatrix to do a piece-by-piece diff of paths changes.
  - Show path changes within the path list.

I'm not entirely sold on this, but it was complicated to write and I've never heard the term "sunk cost fallacy". I think this is mostly a net improvement, but may need some adjustments and followup.

Test Plan: Viewed various changes in Differential and Diffusion, saw a more usable table of contents.

Maniphest Tasks: T13520

Differential Revision: https://secure.phabricator.com/D21183

2 months agoFix an issue with "Auditors:" where an edge edit was used as a PHID list
epriestley [Tue, 28 Apr 2020 12:42:18 +0000 (05:42 -0700)]
Fix an issue with "Auditors:" where an edge edit was used as a PHID list

Summary:
See <https://discourse.phabricator-community.org/t/runtimeexception-during-import-of-commit/3801>. When importing commits with "Auditors:", a raw transaction new value (with an edge edit map using a "+" key) may be passed as an unmentionable PHID list.

Instead, pass an actual PHID list.

Test Plan:
  - Pushed a commit with "Auditors: duck".
  - Ran daemons.
    - Before patch: umentionable PHID exception.
    - After patch: clean commit import.
  - Verified "duck" was added as an auditor.

Differential Revision: https://secure.phabricator.com/D21181

2 months agoImprove the construction of synthetic "comparison/intradiff" changesets
epriestley [Mon, 27 Apr 2020 23:12:43 +0000 (16:12 -0700)]
Improve the construction of synthetic "comparison/intradiff" changesets

Summary:
Ref T13523. Currently, when building a "comparison" changeset, metadata is taken from the left changeset. This is somewhat arbitrary.

This means that intradiffs of images don't work properly because the rendered changeset has only the left (usually "old") information.

Later, some of the code attempts to ignore the file data stored on the changeset and reconstruct the correct file data, which is how the result ends up not-completely-wrong.

Be more careful about building sensible-ish metadata, and then just use it directly later on. This fixes the "spooky" code referencing D955 + D6851.

There are some related issues, where "change type" and "file type" are selected arbitrarily and then used to determine whether the change has an "old/new" state or not (i.e., is the left side of the diff empty, since the change creates the file)?

In many cases, neither of the original changesets have a "change type" which will answer this question correctly. Separate this concept from "has state" from "change type", and make more of the code ask narrower questions about the specific conditions or states it cares about, rather than "change type".

Test Plan:
  - Created a revision with Diff 1, Diff 2, and Diff 3. Diff 1 takes an image from "null -> A". Diff 2 takes the same image from "null -> B". Diff 3 takes the same image from "A -> B'.
  - Intradiffed 1v2 and 1v3.
  - Before patch:
    - Left side usually missing, which is incorrect (should always be "A").
    - Change properties are a mess ("null -> image/png" for MIME type, e.g.)
    - Uninteresting/incorrect "unix:filemode" stuff.
  - After patch;
    - Left side shows state "A".
    - Change properties only show size changes (which is correct).

{F7402012}

Maniphest Tasks: T13523

Differential Revision: https://secure.phabricator.com/D21180