1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-29 10:12:41 +01:00
Commit graph

3395 commits

Author SHA1 Message Date
epriestley
ebef22ccc1 Improve select-to-comment behavior in Firefox and on unified diffs
Summary:
Ref T13513.

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

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

Maniphest Tasks: T13513

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

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

Maniphest Tasks: T13513

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

Test Plan: Viewed raw remarkup on inlines.

Maniphest Tasks: T13513, T11401

Differential Revision: https://secure.phabricator.com/D21246
2020-05-13 17:14:20 -07:00
epriestley
419b7ceebb Move inline comment actions into a dropdown menu
Summary: Ref T11401. Ref T13513. This paves the way for more comment actions, particularly an edit-after-submit action.

Test Plan: Took all actions from menus, via mouse and via keyboard (where applicable).

Maniphest Tasks: T13513, T11401

Differential Revision: https://secure.phabricator.com/D21244
2020-05-13 17:13:18 -07:00
epriestley
1da54837ea Improve line breaking behavior in Firefox and Chrome under complex conditions
Summary: See <https://github.com/phacility/phabricator/pull/854>. In some situations, `line-break: anywhere` produces better behavior than `word-break: break-all`. It never appears to produce worse behavior.

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

Firefox

{F7480567}

Chrome

{F7480568}

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

{F7480569}

  - Additional context:

Safari Behavior (Unchanged)

{F7480570}

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

{F7480571}

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

{F7480574}

Differential Revision: https://secure.phabricator.com/D21247
2020-05-13 11:54:42 -07:00
epriestley
acc1fa1655 Make "View as Document Type..." only show valid options
Summary:
Ref T13513. Currently, "View as Document Type..." lists every available engine.

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

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

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21241
2020-05-12 14:25:37 -07:00
epriestley
0cca40db3b When creating an inline, save the current document engine
Summary:
Ref T13513. As part of inline metadata, save the document engine the change is being rendered with.

This will allow other parts of the UI to detect that an inline was created on a Jupyter notebook but is being rendered on raw source, or whatever else.

The immediate goal is to fix nonsensical inline snippet rendering in email on Jupyter notebooks.

Test Plan:
  - Created inlines and replies on normal soure code, saw no document engine annotated in the database.
  - Created inlines and replies on a Jupyter notebook rendered in Jupyter mode, saw "jupyter" annotations in the database.
  - Swapped document engines between Jupyter and Source, etc.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21240
2020-05-12 14:25:09 -07:00
epriestley
e7ebd5d9d1 Make "Delete" from inline comment previews function correctly while editing comments
Summary: Ref T13513. Currently, if you're editing a comment, "delete" doesn't put the comment into the correct state. This action is normally only reachable from comment previews, since an editing inline has no "delete" button.

Test Plan:
  - Started editing an inline, clicked "Delete", got a deletion.
  - Created an inline, typed text,
  - Deleted a normal comment via preview.
  - Deleted a normal comment via the on-inline action.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21238
2020-05-08 08:54:48 -07:00
epriestley
b804e8cffa Make "View" from inline comment previews correctly jump to "isEditing" inlines
Summary:
Ref T13513. Currently, clicking "View" from the inline comment preview (below the "add comment" area at the bottom of the page) only works if the inline isn't being edited.

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

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

Maniphest Tasks: T13513

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

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

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

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

Maniphest Tasks: T13513

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

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

Simplify an especially gross callsite in preview code.

Test Plan: Previewed inline comments.

Maniphest Tasks: T13513

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

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

Differential Revision: https://secure.phabricator.com/D21222
2020-05-04 15:57:31 -07:00
epriestley
07e160bde1 When cancelling an unsaved editing inline after a reload, don't cancel into an empty state
Summary:
Ref T13513. Overloading "original text" to get "edit-on-load" comments into the right state has some undesirable side effects.

Instead, provide the text when the editor opens. This fixes a cancel interaction.

Test Plan:
  - Create an inline, type text, don't save.
  - Reload page.
  - Cancel.
  - Before: cancelled into empty state.
  - After: cancelled into deleted+undo state.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21219
2020-05-04 15:20:31 -07:00
epriestley
3a76248071 When loading a page with inlines, don't select/focus inlines which we immediately upgrade to "editing"
Summary:
Ref T13513. This is a bit clumsy, but the cleanest way to implement "isEditing" inlines today is to send them down as normal inlines and then simulate clicking "edit" on them.

When we do, don't focus the resulting editor: focusing it makes the page scroll around and highlight things in essentially random order as the editors load in.

Test Plan: Reloaded a page with some open editors, wasn't scrolled to them.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21217
2020-05-04 15:15:27 -07:00
epriestley
fe501bd7f7 Save drafts for inline comments currently being edited
Summary:
Ref T13513. As users type text into inline comments, save the comment state as a draft on the server.

This has some rough edges, particularly around previews, but mostly works. See T13513 for notes.

Test Plan: Started an inline, typed some text, waited a second, reloaded the page, saw an editing inline with the saved text.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21216
2020-05-04 13:19:42 -07:00
epriestley
63bfad0ff4 Refine unusual inline comment client interactions
Summary: Ref T13513. Refine some inline behaviors, see test plan.

Test Plan:
  - Edit a comment ("A"), type text ("AB"), cancel, edit.
    - Old behavior: edit and undo states (wrong, and undo does not function).
    - New behavior: edit state only.
  - Edit a comment ("A"), type text ("AB"), cancel. Undo ("AB"), cancel. Edit.
    - Old behavior: "AB" (wrong: you never submitted this text).
    - New behavior: "A".
  - Create a comment, type text, cancel.
    - Old behavior: counter appears in filetree (wrong, comment is undo-able but should not be counted).
    - New behavior: no counter.
  - Cancel editing an empty comment with no text.
    - Old behavior: Something buggy -- undo, I think?
    - New behavior: it just vanishes (correct behavior).

Maniphest Tasks: T13513

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

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

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21191
2020-05-04 13:13:15 -07:00
epriestley
b2ce0844b6 When a user clicks "Cancel" on an inline comment to leave the "Editing" state, save the state change
Summary:
Ref T13513. Now that the "currently being edited" state of inlines is saved on the server side, clear the flag when the user clicks "Cancel" to leave the "editing" state on the client.

This also serves to delete empty comments.

Test Plan:
  - Clicked a line number to create a new comment. Then:
    - Clicked "Cancel". Reloaded page, saw no more comment.
    - Typed text, saved. Reloaded page, saw non-editing draft. Clicked "Edit", reloaded page, saw editing draft. Clicked "Cancel", reloaded page, saw non-editing draft.
    - Typed text, saved. Clicked "Edit", deleted text, saved. Reloaded page, saw no more comment.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21187
2020-05-04 13:11:23 -07:00
epriestley
b48a22bf50 Make "editing" state persistent for inline comments
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.

In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.

---

Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.

On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.

Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).

To simplify this:

  - Make `PhabricatorInlineCommentInterface` an abstract base class instead.
  - Lift as much code out of the `Audit` and `Differential` subclasses as possible.
  - Delete methods which no longer have callers, or have only trivial callers.

---

Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.

Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.

These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.

The `EditView` can not fully render its content. Move the content rendering code into the view.

---

Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.

This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.

---

Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.

Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.

This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.

---

Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".

Test Plan:
  - Created comments on either side of a diff.
  - Edited a comment, reloaded, saw edit stick.
  - Saved comments, reloaded, saw save stick.
  - Edited a comment, typed text, cancelled, "unedited" to get state back.
  - Created a comment, typed text, cancelled, "unedited" to get state back.
  - Deleted a comment, "undeleted" to get state back.

Weirdness / known issues:

  - Drafts don't autosave yet.
  - Fixed in D21187:
    - When you create an empty comment then reload, you get an empty editor. This is a bit silly.
    - "Cancel" does not save state, but should, once drafts autosave.
  - Mostly fixed in D21188:
    - "Editing" comments aren't handled specially by the overall submission flow.
    - "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.

Subscribers: jmeador

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21186
2020-05-04 13:10:30 -07:00
epriestley
5ff0ae7d48 Add generic "attributes" storage to inline comment tables
Summary: Ref T13513. This plans for "currently editing", character range comments, code suggestions, document engine tracking. And absolutely nothing else.

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

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13513

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

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

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

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

Differential Revision: https://secure.phabricator.com/D21213
2020-05-04 10:04:04 -07:00
epriestley
1205070687 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
2020-05-01 12:23:59 -07:00
epriestley
f21f1d8ab9 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
2020-04-28 12:27:37 -07:00
epriestley
5a460e4ea5 Stick the page footer in the right place on Formation View pages
Summary: Ref T13516. This isn't terribly clean, but get the page footer into the bottom of the content page on FormationView pages so it doesn't overlap into the side panel.

Test Plan: With and without a footer, viewed normal and FormationView pages. Saw footers in appropriate places at appropriate times.

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21166
2020-04-24 11:25:31 -07:00
epriestley
e20feeeee9 Update static resource package definitions
Summary: Ref T13516. Differential got some new UI elements and behaviors, so update static resource package definitions.

Test Plan:
  - Saw JS requests drop from 17 to 4.
  - Saw CSS requests drop from 9 to 3.

(These won't quite match production since some JS/CSS is for DarkConsole.)

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21163
2020-04-23 13:50:19 -07:00
epriestley
4793bfcb7c Don't show the "file tree" view on tablets/phones
Summary: Ref T13516. Hide this UI on devices without the screen width to reasonably support it.

Test Plan: Viewed a revision at various window widths, saw the elements vanish at device widths and reappear at desktop widths.

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21162
2020-04-23 13:40:42 -07:00
epriestley
d2572f8b33 Refine more Differential review state behaviors
Summary:
Ref T13516.

- Add an "Add Comment" navigation anchor.
- Make selection state more clear.
- Make hidden state tidier and more clear.
- Hide "View Options" in the hidden state to dodge all the weird behaviors it implies.
- Click to select/deselect changesets.
- When you open the view dropdown menu, then press "h", close the dropdown menu.

Test Plan: Fiddled with all these behaviors.

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21161
2020-04-23 10:14:52 -07:00
epriestley
0ede616f31 Update the "View Options" menu for recent filetree changes
Summary:
Ref T13516. Minor improvements here:

  - Show key commands in the "View Options" dropdown.
  - Organize it slightly better.
  - Improve disabled item behaviors a little bit.
  - Add a "Browse Directory" action.
  - Rename "...in Diffusion" to "...in Repository".
  - Make "d", "D", and "h" use the same targeting rules as "\".
  - When you hide a file with the "h" menu item, select it.

Test Plan: Poked at the menu a lot, ran into less questionable behavior.

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21160
2020-04-23 08:23:12 -07:00
epriestley
60de1506fe Make "hidden" changesets sticky, and show hidden state in the filetree
Summary:
Ref T13455. Make "hidden" a changeset property similar to other changeset properties.

We don't need to render this on the server, so we make a request (to update the setting) and just discard the response.

Test Plan: {F7375468}

Maniphest Tasks: T13455

Differential Revision: https://secure.phabricator.com/D21158
2020-04-22 16:12:42 -07:00
epriestley
a72a66caa8 Mark "low importance" and "owned" changes in the filetree
Summary: Ref T13516. Mark low-importance changes (generated code, deleted files) and owned-with-authority changes in the filetree.

Test Plan: {F7375327}

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21157
2020-04-22 11:22:34 -07:00
epriestley
ff88eb588e Show change information in file icons in the filetree
Summary: Ref T13516. Restores "deleted"/"added" information to the tree icons.

Test Plan: {F7375145}

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21156
2020-04-22 08:38:29 -07:00
epriestley
9550ae6984 When a directory has a single directory child, collapse them into a single "a/b/" path entry
Summary:
Ref T13516. Instead of rendering trees like this:

  - a/
    - b/
      - c.txt

...render:

  - a/b/
    - c.txt

Test Plan: {F7374205}

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21155
2020-04-22 08:37:03 -07:00
epriestley
12eddb18fb Entirely replace the old filetree UI with the "flank" UI
Summary:
Ref T13516. Deletes all old filetree / flex / active / collapse nav code in favor of the new code.

Restores the inline tips in the path tree.

Test Plan: {F7374175}

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21154
2020-04-22 08:32:02 -07:00
epriestley
ba8071bbef Roughly style the new "flank" paths UI
Summary: Ref T13516. Apply basic UI styling to the new UI and make some more interaction work.

Test Plan: {F7374096}

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21153
2020-04-22 08:31:40 -07:00
epriestley
8cd1f9a309 Generate file trees from changesets in the new flank UI
Summary: Ref T13516. Generate a tree structure based on the page changesets. Still missing styles and a whole lot of behavior.

Test Plan: {F7373967}

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21152
2020-04-22 08:31:17 -07:00
epriestley
646280972b Glue the new FormationView on top of the older Filetree view in Differential
Summary: Ref T13516. This glues "FormationView" to "ChangesetList". The actual tree is not functional in any meaningful way yet.

Test Plan: {F7373838}

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21151
2020-04-22 08:29:04 -07:00
epriestley
fef2cdabfe Add a "FormationView" to support dynamic flank panels
Summary:
Ref T13516. Currently, the "File Tree" element is a semi-dynamic side panel that's implemented as a special mode of a side nav panel.

This implementation is fairly clunky, and arose from organic growth out of the side nav. As such, it has some weird behaviors, doesn't have builtin support for show/hide, and can't generalize easily.

Introduce a "FormationView" which supports loading a page up with piles of side panels in various modes.

Test Plan: No callers and no user-visible impact.

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21150
2020-04-22 08:23:21 -07:00
epriestley
ef69c7969f Restore editor behavior to Diffusion and support "\" shortcut
Summary:
Ref T13515. This restores the "Open in Editor" behavior to Diffusion, and makes "\" work there.

The URI pattern is now sent as a structured template to the client, so the code will work properly if a file path contains "%l".

Test Plan:
  - Clicked "Open in Editor" and pressed "\" in Diffusion when viewing a file.
  - Clicked a line, hit "\", got the file opened to that line.

Maniphest Tasks: T13515

Differential Revision: https://secure.phabricator.com/D21149
2020-04-19 09:41:37 -07:00
epriestley
537ff68edd In Differential, make the "Open in Editor" keystroke work with no selection, or a change or inline selected
Summary:
Ref T13515. Currently, "Open in Editor" only works with a file-level selection.

  - If we have a change-level or inline-level selection, open the parent changeset.
  - If we have no selection, but the banner is showing something, open the fine shown in the banner.

Test Plan: With files, inlines, changes, and no selection, pressed "\". Saw files pop open in my external editor.

Maniphest Tasks: T13515

Differential Revision: https://secure.phabricator.com/D21148
2020-04-19 09:41:03 -07:00
epriestley
8bdc713352 Make the "Keyboard Shortcuts" dialog in Differential less hideous
Summary:
Ref T13515. Adding "\" ("Open in External Editor") made this slighlty worse, but it was already pretty bad.

Long ago the keys had a special style on them, but this got changed and dropped somewhere around D16568 -- although at the time, I think they still had a grey background (see T11654).

Some later change removed this background.

Put the background back and separate the keystrokes into groups.

Test Plan: {F7370615}

Maniphest Tasks: T13515

Differential Revision: https://secure.phabricator.com/D21141
2020-04-19 09:01:07 -07:00
epriestley
c3c55d82ae Make "renderer", "engine", and "encoding" sticky across reloads in Differential and Diffusion
Summary:
Ref T13455. Update the other "view state" properties to work like "highlight" now works.

Some complexity here arises from these concerns:

  - In "View Standalone", we render the changeset inline. This is useful for debugging/development, and desirable to retain.
  - In all other cases, we render the changeset with AJAX.

So the client needs to be able to learn about the "state" properties of the changeset on two different flows. Prior to this change, each pathway had a fair amount of unique code.

Then, some bookkeeping issues:

  - At inital rendering time, we may not know which renderer will be selected: it may be based on the client viewport dimensions.
  - Prior to this change, the client didn't separate "value of the property for the changeset as rendered" and "desired value of the property".

Test Plan:
  - Viewed changes in Differential, Diffusion, and in standalone mode.
  - Toggled renderer, character sets, and document engine (this one isn't terribly useful). Reloaded, saw them stick.
  - Started typing a comment, cancelled it, hit the undo UI.

Maniphest Tasks: T13455

Differential Revision: https://secure.phabricator.com/D21138
2020-04-19 08:59:09 -07:00
epriestley
8aac55cc57 Make "Highlight As..." sticky across reloads in Diffusion and Differential
Summary:
Ref T13455. Add container-level storage for persistent view state, and persist "Highlight As..." inside it.

The storage generates a "PhabricatorChangesetViewState" configuration object as an output.

When preferences are expressed on a diff and that diff is later attached to a revision, we attempt to copy the preferences.

The internal storage tracks per-changeset settings, but currently always uses "last update wins" to apply the settings in the UI.

Test Plan:
  - Viewed revisions, changed highlighting, reloaded. Saw highlighting stick in revision view and standalone view.
  - Viewed commits, changed highlighting, reloaded. Saw highlighting stick.
  - Created a diff, changed highlighting, turned it into a revision, saw highlighting persist.

Subscribers: jmeador, PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13455

Differential Revision: https://secure.phabricator.com/D21137
2020-04-19 08:58:39 -07:00
epriestley
3d966d8a41 Add an "Open in External Editor" keystroke to Differential
Summary: Ref T13515. See PHI1661. If a file is selected, add a keystroke to click the "Open in External Editor" link.

Test Plan: In Safari, Chrome, and Firefox: used "J" to select a file, then "\" to open it in an external editor. (In Safari and Chrome, this prompts.)

Maniphest Tasks: T13515

Differential Revision: https://secure.phabricator.com/D21135
2020-04-17 10:06:46 -07:00
epriestley
925d2b051c Fix a "flickering" behavior with the menu bar transition animations in Chrome
Summary:
Fixes T13508. The "Notification" and "Messages" icons in the menu bar have a CSS transition animation on hover.

In Chrome, when this element moves up 2px, you can get a flicker in and out of the hover state if the user's cursor is at the very bottom of the element, since the bounding box for the element is rapidly sliding in and out of the area under the cursor.

To fix this: as we move the element up, also make it taller.

Test Plan: In Safari, Chrome, and Firefox: put my cursor at the very bottom of the element, no longer saw any animation flickering.

Maniphest Tasks: T13508

Differential Revision: https://secure.phabricator.com/D21133
2020-04-17 06:03:36 -07:00
epriestley
3e676bce9e No-op an ancient Paste edge migration which no longer functions after a database rename
Summary:
Fixes T13510. This migration currently fails because it tries to affect the "paste" database, but when it runs this database will be named "pastebin".

Since the cost of fixing it in place or moving it past the rename migration both seem relatively high (and the cost of throwing it away is plausibly zero) just throw it for now.

Test Plan: Looked at file, saw no more code that can execute.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13510

Differential Revision: https://secure.phabricator.com/D21132
2020-04-17 05:38:31 -07:00
Austin McKinley
ef1340bd32 Add Ferret support to Paste
Summary:
Ref PHI1292. Enable fulltext searchs in paste. Maybe this should only index a snippet instead of the entire content?

Also updates table names in `PhabricatorPasteQuery`.

Test Plan: Created some pastes, indexed them, searched for them.

Reviewers: amckinley

Subscribers: codeblock, Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D20650
2020-04-16 14:10:23 -07:00
epriestley
d86506052c Update a very old Phriction migration which incorrectly uses "save()"
Summary:
See <https://discourse.phabricator-community.org/t/storage-upgrade-error/3748>.

It is broadly unsafe for migrations to use "save()". If the object gains new fields later, the query will include "SET newField = X", which will fail against the old schema which is in the process of being upgraded.

Instead, migrations must issue raw SQL against the schema as it is expected to exist at the time the migration executes.

Migrations have followed this rule for a long time, but this ~6 year old migration was overlooked. Update it to issue a raw query to perform the policy update.

Test Plan: This is somewhat flimsy since rebuilding a genuine reproduction case is messy, but used "bin/storage --apply ..." to at least get the new query to execute against modern Phabricator without issues.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D21124
2020-04-15 08:06:30 -07:00
epriestley
785f3c98da Extract raw commit messages from Git more faithfully across Git versions
Summary:
Fixes T5028. Older versions of Git (apparently, from before 2010) did not provide a way to extract the raw body of a commit message from "git log", so we approximate it with "subject" and "wrapped body".

In newer versions of Git, the raw body can be extracted exactly.

Adjust how we extract messages based on the version of Git, and try to be more faithful to edge cases: particularly, be more careful to extract the correct number of trailing newlines.

Test Plan:
  - Added "var_dump()" + "die(1)" later in this method, then pushed various commit messages. Used "&& false" to force execution down the old path (either path should work in modern Git).
  - Observed more faithful extraction of messages, including a more faithful extraction of the number of trailing newlines. Extraction is fully faithful if we can go down the "%B" path, which we should be able to in nearly all modern cases.
  - Not all messages extract faithfully or consistently across the old and new versions, but the old extraction is destructive so this is likely about as close as we can realistically ever get.

Maniphest Tasks: T5028

Differential Revision: https://secure.phabricator.com/D21027
2020-02-24 12:37:45 -08:00
epriestley
84b5ad09e6 Remove all readers and all nontrivial writers for "accountType" and "accountDomain" on "ExternalAccount"
Summary:
Depends on D21018. Ref T13493. Ref T6703. The "ExternalAccount" table has a unique key on `<accountType, accountDomain, accountID>` but this no longer matches our model of reality and changes in this sequence end writes to `accountID`.

Remove this key.

Then, remove all readers of `accountType` and `accountDomain` (and all nontrivial writers) because none of these callsites are well-aligned with plans in T6703.

This change has no user-facing impact today: all the rules about linking/unlinking/etc remain unchanged, because other rules currently prevent creation of more than one provider with a given "accountType".

Test Plan:
- Linked an OAuth1 account (JIRA).
- Linked an OAuth2 account (Asana).
- Used `bin/auth refresh` to cycle OAuth tokens.
- Grepped for affected symbols.
- Published an Asana update.
- Published a JIRA link.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13493, T6703

Differential Revision: https://secure.phabricator.com/D21019
2020-02-22 17:48:46 -08:00
epriestley
faf9f06e0a Migrate all "accountID" values to "ExternalAccountIdentifier" objects
Summary: Depends on D21016. Ref T13493. This copies existing external account "accountID" values into the "ExternalAccountIdentifier" table, preparing for an authority switch.

Test Plan: Ran migration several times, looked at the data that came out of it, saw sensible results. Logged out / in with external accounts.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13493

Differential Revision: https://secure.phabricator.com/D21017
2020-02-22 17:47:37 -08:00