Summary:
Ref T13586. Currently, Herald condition logs encode "pass" or "fail" robustly, "forbidden" through a sort of awkward side channel, and can not properly encode "invalid" or "exception" outcomes.
Structure the condition log so results are represented unambiguously and all possible outcomes (pass, fail, forbidden, invalid, exception) are clearly encoded.
Test Plan:
{F8446102}
{F8446103}
Maniphest Tasks: T13586
Differential Revision: https://secure.phabricator.com/D21563
Summary: Ref T13613. Improve the performance of this migration by using a temporary table and an "UPDATE x JOIN y ..." pattern.
Test Plan:
- Ran on `secure`, got exit after a few seconds since the migration is idempotent and changesets already had PHIDs.
- Ran on `secure` with the `continue;` commented out, got valid new PHIDs in 53s (from 153s).
- Tried a larger page size (16K), didn't see any improvement.
- From "--trace", client PHID generation seems to be the limiting factor.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13613
Differential Revision: https://secure.phabricator.com/D21570
Summary:
Ref T13587. D21495 has significant changes to the ngram indexer, which might possibly contain bugs.
Make it easier to reindex a subset of documents (based on the date when the index was built, and/or the software version which generated the index).
This is in addition to the existing versioning, which is focused on object versions.
Test Plan: Ran `bin/search index` with various old and new arguments. Spot-checked the `IndexVersion` table.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13587
Differential Revision: https://secure.phabricator.com/D21560
Summary:
Ref T13605. Changesets currently have no PHID, which limits their ability to use standard API infrastructure.
Give them a PHID, since there's no reason they don't have one other than their age.
Test Plan:
- Ran migrations, saw PHIDs populated.
- Created new changesets, saw PHIDs.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13605
Differential Revision: https://secure.phabricator.com/D21557
Summary: Ref T13602. Similar to subscriber and mention treatments, make it clear when a user doesn't have view permission.
Test Plan: {F8430595}
Maniphest Tasks: T13602
Differential Revision: https://secure.phabricator.com/D21555
Summary:
Ref T13602. When rendering a user hovercard, pass the object on which the reference appears. If the user can't see the object, make it clear on the hovecard.
Restyle the "nopermission" markup in mentions to make it more obvious what the style means: instead of grey text, use red with an explicit icon.
Test Plan: {F8430398}
Maniphest Tasks: T13602
Differential Revision: https://secure.phabricator.com/D21554
Summary:
Ref T13602. Currently, Hovercards are functions only of the object they represent (and the viewer, etc).
Recent changes to how users who can't see an object are rendered motivate making them a function of both the object they represent //and// the context in which they are being viewed. In particular, this enables a hovecard for a user to explain "This user can't see the thing you're lookign at right now.", so visual "exiled" markers can have a path forward toward discovery.
Test Plan:
- This change isn't expected to affect any behavior.
- Viewed hovercards, moused over/out, resized windows, viewed standalone cards, viewed debug cards, saw no behavioral changes.
Maniphest Tasks: T13602
Differential Revision: https://secure.phabricator.com/D21553
Summary:
Ref T13602. When a subscriber can't see an object, it's currently hard to figure it out.
Show this status clearly in the curtain UI.
Test Plan: {F8382865}
Maniphest Tasks: T13602
Differential Revision: https://secure.phabricator.com/D21547
Summary:
Ref T13591. Worker queue tasks which affect commits currently (mostly) store the commit as an "objectPHID", but do not directly reference the repository the commit belongs to.
This can make certain operations (like "change the priority of all tasks affecting repository Y") more difficult than it needs to be.
Support a "containerPHID", similar to the field of the same name on builds, that can store a parent object like a repository and better support operations against subsets of tasks.
See also D11044 for the genesis of "objectPHID".
This depends on the introduction of storage patch phases (in D21529) so that earlier migrations which queue worker tasks don't try to insert this column before it actually exists.
Test Plan:
- Ran `bin/storage upgrade`.
- No callers yet, see further changes for usage.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13591
Differential Revision: https://secure.phabricator.com/D21531
Summary:
Ref T13591. Some storage patches queue worker tasks, currently always to rebuild search indexes.
These patches can not execute in creation order if a later patch modifies the worker task table, since they'll try to perform a modern INSERT against an out-of-date table schema. Such a modification is desirable in the context of T13591, but making it causes these patches to fail.
Patches have an existing "after" mechanism which allows them to have explicit dependencies. This mechanism could be used to resolve this issue, but all patches with a dependency like this would need to be updated every time the queue table changes.
Instead, introduce "phases" to provide broader ordering rules. There are now two phases: "default" and "worker". Patches in the "worker" phase execute after patches in the "default" phase.
Phases may eventually be further separated, but
Test Plan:
- Ran `bin/storage status`, saw patches annotated with phases.
- Will apply `containerPHID` changes on top of this.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13591
Differential Revision: https://secure.phabricator.com/D21529
Summary:
See PHI1900. Recent changes to how commit graphs are drawn made the height automatic in most cases, but it fails in Differential because the element isn't initially visible so the computed height is 0.
Just give them an explicit height so they show up again.
Test Plan: Viewed graphs in Maniphest, Differential, and Diffusion; saw them all render properly.
Differential Revision: https://secure.phabricator.com/D21481
Summary:
See PHI1898. An install is reporting an execution/initialization order issue where this code is reachable before `_inlines` is initialized.
I can't immediately reproduce it, but using "getInlines()" is preferable anyway and seems likely to fix the problem.
Test Plan: Viewed revisions with inlines, added/removed/edited/replied to inlines, didn't find anything broken.
Differential Revision: https://secure.phabricator.com/D21475
Summary:
Ref T13564. See PHI1798. Earlier efforts here (see D21439) still leave us with:
- Incorrect behavior for long URIs, like `http://www.example.com/MMMMM...`.
- Incorrect beahvior for long text blocks, like `MMMMMM...`.
- Undesirable behavior for monospaced text in non-printing contexts (it wraps when we'd prefer it not wrap).
Apply the wrapping rules to all "<td>" content to resolve these three prongs.
Test Plan:
- Viewed long URIs, text blocks, and monospaced text in and out of tables, while printed and not printed, in Safari, Firefox, and Chrome.
- All browser behavior now appears to be correct ("all content is preserved in printed document").
- Some browser behavior when making wrapping choices is questionable, but I can't find an automatic solution for that.
Maniphest Tasks: T13564
Differential Revision: https://secure.phabricator.com/D21472
Summary: Ref T13577. After the fix in D21453, lint identifies additional static errors in Phabricator; fix them.
Test Plan: Ran `arc lint`; these messages are essentially all very obscure.
Subscribers: hach-que, yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13577
Differential Revision: https://secure.phabricator.com/D21457
Summary:
Ref T13573. Using the browser "Print" feature on pages produces "Thu, Aug 4, 12:22" timestamps which require context to interpret precisely (they don't have a year and don't have a timezone).
Instead, retain these timestamps in "screen" contexts but use "YYYY-MM-DD HH:MM:SS (UTC+X)" timestamps when printing.
Test Plan: Printed Maniphest tasks and other pages in Safari and Chrome using "?__print__=1" and "Print to PDF", saw absolute timestamps after this chagne in the printed documents.
Maniphest Tasks: T13573
Differential Revision: https://secure.phabricator.com/D21451
Summary: Ref T13552. There are currently some content overflow issues on the graph view where the menu height can exceed the content height and the frame is drawn on a sub-element. Make the frame draw around all the content.
Test Plan: Viewed commit graph history view, saw more sensible UI.
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21440
Summary: Ref T13552. Provide a richer handle/status list item for commit lists.
Test Plan: Viewed commits in various interfaces, saw richer information.
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21431
Summary:
Ref T13552. Build the "commit list" elements so that the menu action items collapse under the element on mobile.
Also change the mobile breakpoint to 512px because my Safari window can't go any narrower than 508px. Future changes to responsive design will be more content-aware anyway.
Test Plan: Looked at commits in various interfaces, at desktop and mobile widths.
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21430
Summary:
Ref T13552. The current layout doesn't work particularly well on desktops or devices.
We have some device/desktop table layout code, but it isn't generic. We also have property list layout code, but it isn't generic either.
Provide generic layout elements ("Fuel", from "Phabricator UI Layout" to "PHUIL"?) and narrowly specialize their display behavior. Then swap the ListItemView stuff to use it.
Test Plan:
Saw slightly better responsive behavior:
{F7637457}
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21418
Summary:
Ref T13552. In unifying the various Graph/List/Table commit views, some information was dropped -- particularly, audit status.
Restore most of it. The result isn't very pretty, but has most of the required information.
Test Plan: {F7637411}
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21417
Summary: Ref T13552. Some of the CSS can be removed or simplified now that essentially all lists of commits are on a single rendering pathway.
Test Plan: Grepped for affected CSS, viewed commit graph.
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21416
Summary:
Ref T13552. This older view mostly duplicates other code and has only two callsites:
- The "Commits" section of user profile pages.
- The "Ambiguous Hash" page when you visit a commit hash page which is an ambiguous prefix of two or more commit hashes.
Replace both with "DiffusionCommitGraphView".
Test Plan:
- Visited profile page, clicked "Commits".
- Visited an ambiguous hash page (`rPbd3c23`).
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21412
Summary: Ref T13552. In the new combined "table/list" graph view, tidy up the graph rendering.
Test Plan: {F7633504}
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21411
Summary:
See T13564. In Chrome only, printing tables with a cell containing an unbroken monospaced text element fails to wrap/break the cell.
Adding "overflow-wrap" appears to fix this without making anything worse. Try this until new problems arise.
Test Plan: Printed such a table to PDF in Chrome, got wrapping with all content visible in the PDF.
Differential Revision: https://secure.phabricator.com/D21439
Summary:
Reverts D21419. See PHI1814. Previously, I used "user-select: all" to group sequences of spaces for selection.
However, this has a side effect: the sequence is now selected with a single click. I didn't read the docuementation on the CSS property thoroughly and missed this in testing, since I was focused on drag-selection behavior.
This behavior is enough of a net negative that I think we're in a worse state overall; revert it.
Test Plan: Straight revert.
Differential Revision: https://secure.phabricator.com/D21429
Summary:
Ref T2495. See PHI1814. Currently, Phabricator replaces tabs with spaces when rendering diffs.
This may or may not be the best behavior in the long term, but it gives us more control over expansion of tabs than using tab literals.
However, one downside is that you can use your mouse cursor to select "half a tab", and can't use your mouse cursor to distinguish between tabs and spaces. Although you probably shouldn't be doing this, this behavior is less accurate/correct than selecting the entire block as a single unit.
A specific correctness issue with this behavior is that the entire block is copied to the clipboard as a tab literal if you select any of it, so two different visual selection ranges can produce the same clipboard content.
This particular behavior can be improved with "user-select: all", to instruct browsers to select the entire element as a single logical element. Now, selecting part of the tab selects the whole thing, as though it were really a tab literal.
(Some future change might abandon this approach and opt to use real tab literals with "tab-size" CSS, but we lose some ability to control alignment behavior if we do that and it doesn't have any obvious advantages over this approach other than cursor selection behavior.)
Test Plan:
- In Safari and Firefox, dragged text to select a whitespace-expanded tab literal. Saw browsers select the whole sequence as though it were a single tab.
- In Chorme, this also mostly works, but there's some glitchiness and flickering. I think this is still a net improvement, it's just not as smooth as Safari and Firefox.
Maniphest Tasks: T2495
Differential Revision: https://secure.phabricator.com/D21419
Summary:
Ref PHI1798. If you put an SSH public key in a table cell with monospaced formatting and then print the table, the cell scrolls and not all of the content appears in your physical printed document.
Generally, the current scrolling behavior for monospaced text seems never-desirable: I can't imagine any cases where we want the table cell to scroll. (There's more of an argument for complex cases where a table cell has, say, an embedded paste.)
Add `line-break: anywhere` to break monospaced text inside these cells.
Test Plan: In Safari, Firefox, and Chrome, viewed a ##|`MMMMM....`|## table. Saw scrolling before and wrapping/breaking after.
Differential Revision: https://secure.phabricator.com/D21370
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
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
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
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
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
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
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
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
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
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
Summary: Ref T13513. 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
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
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
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
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
Summary:
Ref T13513. Give selected inlines a selection state and visual cues which are similar to the changeset selection state.
Also fix a couple of minor issues with select interactions and offset comments.
Test Plan: Selected inlines, saw obvious visual cues.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21256
Summary: Ref T13513. 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
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
Summary: Ref T13513. This fixes a bug where clicking a line number, then clicking "Cancel" causes the paths panel to briefly update with an extra inline comment counted on the file.
Test Plan:
- Clicked a line number.
- Typed some text.
- Clicked "Cancel".
- Before patch: paths panel flashes "1".
- After patch: paths panel stays stable.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21253
Summary:
Ref T13513. Currently, when creating an inline by selecting a line range, slightly careless handling leads to an inline with "0" offsets (by passing "undefined" to the server). This causes the block to highlight every line except the last one as fully bright, which is incorrect.
An inline with "0" offsets and an inline with no offsets are different. Be more careful about passing offsets around and rendering them.
Test Plan:
- Used the line numbers to add an inline to lines 4-8 of a change.
- Hovered the inline.
- Saw all four lines marked as "dull"-highlighted (previously: three bright lines, one dull line).
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21252
Summary:
Ref T13513. When a user selects a text range and uses "New Inline Comment" to create a comment directly from a range, store the offset information alongside the comment.
When hovering the comment, highlight the original range.
Test Plan: {F7480926, size=full}
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21250
Summary:
Ref 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
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
