1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-05 05:02:44 +01:00
Commit graph

131 commits

Author SHA1 Message Date
epriestley
80c329c942 When replying to a threaded inline, put the new inline in the right place in the thread
Summary:
Fixes T10563. If you have a thread like this:

```
> A
  > B
  > C
```

...and you reply to "B", we should put the new inline below "B".

We currently do when you reload the page, but the initial edit goes at the bottom always (below "C").

Test Plan: {F4963015}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10563

Differential Revision: https://secure.phabricator.com/D17938
2017-05-17 12:27:14 -07:00
epriestley
d03a497616 Allow any inline in the document to be queried by ID
Summary:
Ref T12616. When you "Delete" a comment from the preview, we try to delete the comment on screen too.

It may or may not be present on screen: if you just added it it's usually visible. However, you might also have hidden the file it contains or it could be on an older diff in a file which is no longer present in the current diff.

After updates in T12616, we could only find the comment if you'd previously interacted with it for some reason. Update this code to be able to find all inlines present in the document.

Test Plan:
  - Write a draft comment.
  - Reload the page.
  - DO NOT INTERACT WITH THE COMMENT!
  - Delete it from the preview.
  - After patch: Comment is deleted from the document, too.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616

Differential Revision: https://secure.phabricator.com/D17937
2017-05-17 12:26:40 -07:00
epriestley
dde63af1cc Fix a JS console warning when hovering over replies to ghosts on lines which no longer exist
Summary:
Fixes T11662. In the very obscure situation described in that task, quiet a JS console warning.

The actual edit operation appears to work correctly after changes elsewhere.

There aren't really any legitimate lines for us to highlight in this case so I'm just giving up rather than trying to do something approximate.

Test Plan:
  - Wrote `long.txt`.
  - Created revision.
  - Added an inline near the bottom.
  - Removed most of `long.txt`.
  - Updated revsion.
  - Replied to the ghost inline.
  - Edited the reply to the ghost inline, worked.
  - Hovered the reply to the ghost inline: no line highlight, but no errors.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11662

Differential Revision: https://secure.phabricator.com/D17930
2017-05-17 08:55:10 -07:00
epriestley
343f7cac72 Improve mobile/device behaviors for inline comments
Summary: Fixes T1026. Ref T12616. Allows drag-to-select on devices to add inlines on a range of lines, using dark magic that I copy/pasted from StackOverflow.

Test Plan: Left a comment on a range of lines on iPhone simulator.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616, T1026

Differential Revision: https://secure.phabricator.com/D17928
2017-05-17 08:53:42 -07:00
epriestley
51df02821b Move the "select a line range" inline code to DiffInline
Summary: Ref T12616. This makes line range selection use the new code, and removes the remainder of the old "hover a line number" / "select a line range" code.

Test Plan: Hovered line numbers; selected line ranges.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616

Differential Revision: https://secure.phabricator.com/D17927
2017-05-17 08:41:26 -07:00
epriestley
e4e91ebf6f In Differential, allow "r" to create comments and "R" to quote
Summary:
Ref T11401. Fixes T5232. Ref T12616.

Partly, this moves more code over to the new stuff.

This also allows "r" to work if you have code selected (not just comments). If you "reply" to code, you start a new comment.

You can "R" a comment to quote it. This just starts a new comment normally if you "R" a block of code. This is sort of a power-user version of "quote" since it seems like it probably doesn't really make sense to put it in the UI ever (maybe).

With the new click-to-select, you can click + "R" to reply-with-quote.

Test Plan: Used "r" and "R" to reply to comments and code.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616, T11401, T5232

Differential Revision: https://secure.phabricator.com/D17920
2017-05-16 17:37:54 -07:00
epriestley
0ca49fbeb9 Move "hover over an inline to see the affected lines" code to the new class tree
Summary:
Fixes T8047. Ref T12616. Fixes T9270. This moves the "hover" part of the hover/drag behavior to the new code, leaving the "drag" part for a followup change.

The new hover UI behaves properly with Quicksand (T8047) and the filetree (T9270).

Test Plan: Hovered over inlines, saw lines select properly.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616, T9270, T8047

Differential Revision: https://secure.phabricator.com/D17919
2017-05-16 17:37:38 -07:00
epriestley
772afc5ed8 Allow cancelled inlines, edits, and replies to be undone to get the text back again
Summary: Ref T12616. The ability to do {nav Edit > Cancel > Undo} to get your text back on inlines got dropped during the conversion. Restore it.

Test Plan:
Created, replied, and edited inlines, typed text, then cancelled. Was able to undo.

Also undid normal deletion.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616

Differential Revision: https://secure.phabricator.com/D17916
2017-05-16 13:12:14 -07:00
epriestley
325682248a If there's an anchor in the URL in Differential, don't stick to the bottom of the page as content loads
Summary:
Fixes T11784. A lot of things are interacting here, but this probably gets slightly better results slightly more often?

Basically:

  - When we load content, we try to keep the viewport "stable" on the page, so the page doesn't jump around like crazy.
  - If you're near the top or bottom of the page, we try to stick to the top (e.g., reading the summary) or bottom (e.g., writing a comment).
  - But, if you followed an anchor to a comment that's close to the bottom of the page, we might stick to the bottom intead of staying with the anchor.

Kind of do a better job by not sticking to the bottom if you have an anchor. This will get things wrong if you follow an anchor, scroll down, start writing a comment, etc. But this whole thing is a pile of guesses anyway.

Test Plan:
  - Followed an anchor, saw non-sticky stabilization.
  - Loaded the page normally, scrolled to the bottom real fast, saw sticky stabilization.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11784

Differential Revision: https://secure.phabricator.com/D17911
2017-05-16 11:06:52 -07:00
epriestley
8052ab84bf Remove "^" (Prev) and "V" (Next) actions on Differential inline comments
Summary:
Ref T12616. Fixes T12715. I suspect these are very rarely used. (I think you tried to get rid of them before but I pushed back since we couldn't really offer great alternatives at the time?)

Now that the code is in a better place:

  - Click an inline's header (just the colored part) to select it with the keyboard selection cursor.
  - Click again to deselect it.
  - You can use "n" and "p" to jump to comments, so "click + n" is the same as the old "V" action.
  - This also makes it easier to swap between keyboard and mouse workflows, since you can jump into things with the keyboard at any inline.

Also, make "Reply" render more consistently.

Test Plan:
  - Did all that stuff, things seemed to work OK.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12715, T12616

Differential Revision: https://secure.phabricator.com/D17908
2017-05-16 09:44:00 -07:00
epriestley
1b5a276a02 Add Differential keyboard shortcuts for "mark done" and "hide/show"
Summary:
Fixes T8130. Allows selected comments to be shown/hidden (with "q") or marked done/not-done (with "w").

(These key selections are because "qwer" are right next to each other on QWERTY keyboards, and now mean "hide, done, edit, reply".)

Also, allow "N" and "P" to do next/previous inline, including hidden inlines. This makes "q" to hide/show a little more powerful and a little easier to undo.

Test Plan: Used "q", "w", "N" and "P" to navigate and interact with comments.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8130

Differential Revision: https://secure.phabricator.com/D17906
2017-05-16 08:23:22 -07:00
epriestley
a154407efb Retain keyboard cursor state across more inline edit operations in Differential
Summary:
Ref T12634. Fixes T8131. Currently, most edit operations (edit, reply, collapse, mark done) lose the keyboard cursor state.

Instead, bind the state more tighlty to the inline object itself (instead of the rows which happen to be in the document), and then do a bit of recalculation to try to keep it selected across edits.

Test Plan:
  - Used "n" to select an inline.
  - Clicked "Done" checkbox.
  - Pressed "n".
  - Went to the next inline (previously: lost position in document).
  - Behavior is also better for: edit, reply, collapse/expand.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12634, T8131

Differential Revision: https://secure.phabricator.com/D17905
2017-05-16 08:23:04 -07:00
epriestley
1493f08272 Emit resize events after making document changes during inline editing
Summary:
Ref T12634. Fixes T12633. These events allow the keyboard reticle to resize properly.

(I expect to possibly hide/disable the reticle in the future during edits, but at least make the behavior sensible for now.)

Test Plan:
  - Used "n" to select a block.
  - Clicked a line number in that block to start a new inline comment.
  - Saw reticle resize properly.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12634, T12633

Differential Revision: https://secure.phabricator.com/D17904
2017-05-16 08:22:43 -07:00
epriestley
665ff4fdf6 Redraw the Differential keyboard reticle after collapsing/un-collapsing an inline
Summary: Ref T12634. Fixes T10049. Toggling an inline currently leaves the reticle oddly-positioned.

Test Plan:
  - Selected a comment with the keyboard.
  - Collapsed it.
  - Saw reticle behave reasonably.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12634, T10049

Differential Revision: https://secure.phabricator.com/D17903
2017-05-16 08:22:24 -07:00
epriestley
7d6133929a Resize the Differential keyboard focus reticle when the window is resized
Summary: Fixes T12632. Ref T12634. Currently, the keyboard focus reticle does not redraw properly after a window resize.

Test Plan:
  - Used "n" to select a block.
  - Resized the window.
  - Saw the reticle also resize properly.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12634, T12632

Differential Revision: https://secure.phabricator.com/D17901
2017-05-16 08:21:32 -07:00
epriestley
5d7202526f Hide the Differential keyboard focus reticle after Quicksand navigation
Summary: Ref T8047. Ref T12634. When we sleep, hide the reticle. Restore it when we wake.

Test Plan:
  - With Quicksand enabled..
  - Used "j" to select a change in a revision.
  - Navigated away by clicking a link.
  - WOW! Reticle vanished properly!
  - Used "back" to return.
  - Reticle returned properly.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12634, T8047

Differential Revision: https://secure.phabricator.com/D17900
2017-05-16 08:21:11 -07:00
epriestley
06c933781e Move keyboard focus reticle code to Differential
Summary:
Ref T12634. Using keyboard shortcuts in Differential currently relies on focus behavior in `KeyboardShortcutManager`.

This possibly made sense long ago, but no longer does, and leads to a whole slew of bugs where the reticle doesn't interact properly with anything else.

Move it to Differential so it can be made reasonably aware of edit operations, Quicksand navigation, etc.

This just moves the code; future diffs will actually fix bugs.

Test Plan: Used "n", "j", etc., saw the same behavior as before.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12634

Differential Revision: https://secure.phabricator.com/D17899
2017-05-16 08:20:50 -07:00
epriestley
2fb1edfeb1 Restore the Differential "edit" and "reply" keyboard shortcuts
Summary: Ref T12616. This makes "edit" and "reply" work again.

Test Plan:
Used "e" and "r" to edit and reply.

Also used them in bogus ways and got useful UI feedback.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616

Differential Revision: https://secure.phabricator.com/D17895
2017-05-16 06:25:35 -07:00
epriestley
588a66c04d Move most Differetial keyboard shortcuts into DiffChangesetList
Summary: Ref T12616. This moves most keyboard shortcuts into DiffChangesetList. It breaks some shortcuts that I plan to restore later, noted in T12616 (toggle file, edit inline, reply to inline), since I think ripping them out now and rebuilding them in a little bit will make things much simpler.

Test Plan:
  - Used j, k, n, p, J, K shortcuts to navigate a revision.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616

Differential Revision: https://secure.phabricator.com/D17859
2017-05-16 06:24:42 -07:00
epriestley
41379f39de Move inline replies to new code and remove DifferentialInlineEditor
Summary:
Ref T12616. This moves "reply" to the new stuff and deletes DifferentialInlineEditor, which no longer does anything.

(This breaks some keyboard shortcuts, but I'll rebase D17859 shortly.)

Test Plan: Replied to inlines; things seemed to work properly.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616

Differential Revision: https://secure.phabricator.com/D17894
2017-05-16 06:23:51 -07:00
epriestley
58dded555b Move inline comment creation to new DiffInline code
Summary:
Ref T12616. This makes creating inlines use the new code.

Creation and editing is now slightly more consistent in how it uses nodes. This will simplify the next change (replies), which I ran into some trouble with in an earlier iteration.

Note that this (and other changes in the series) allow you to create and edit multiple inlines simultaneously. This is mostly a feature, although I expect we'll need to lock it down a little bit. I have some UI ideas to help avoid errors.

Test Plan: Created inlines on a single line; on a range of lines; on the same line; multiple inlines at the same time.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616

Differential Revision: https://secure.phabricator.com/D17893
2017-05-16 06:22:00 -07:00
epriestley
d97f80bc90 Make new DiffInline code handle most "delete" operations
Summary:
Ref T12616. This moves the delete actions to the new, more stateful way of doing things.

These are a little tricky because you can click "Delete" on an inline, but you can also click "Delete" from the preview area at the bottom of the page. If you do, the inline you are deleting may or may not be present on the page.

This has a few bugs -- notably, deleting from the preview without interacting with the on-page inline first won't actually delete the on-page inline yet -- but nothing too serious.

Test Plan: Deleted inlines, undid deletion, deleted from preview.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616

Differential Revision: https://secure.phabricator.com/D17890
2017-05-16 06:21:37 -07:00
epriestley
3c18cb77fb Move inline "done" checkboxing to DiffInline
Summary:
Ref T12616. This updates clicking the "Done" checkbox for the new stuff.

This one is pretty clean since the "Done" checkbox doesn't do too much weird magic.

Test Plan: Clicked the box a few times.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616

Differential Revision: https://secure.phabricator.com/D17888
2017-05-16 06:21:00 -07:00
epriestley
798c8ba696 Mostly move inline editing to DiffInline
Summary:
Ref T12616. This doesn't pull over everything (some UI feedback didn't make it yet, and you can't cancel + undo cancelling edits yet) but editing comments technically works.

This is a little shaky, but feels less shaky than every other approach I've tried, so I think I'm finally on a reasonable track here.

Test Plan: Edited some inline comments.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616

Differential Revision: https://secure.phabricator.com/D17887
2017-05-16 06:20:26 -07:00
epriestley
4fd4ec3d27 Hide inlines one-by-one, instead of in a big group
Summary:
Ref T12616. Fixes T12153. Currently, when you hide inlines, they hide completely and turn into a little bubble on the previous line.

Instead, collapse them to a single line one-by-one. Narrowly, this fixes T12153.

In the future, I plan to make these changes so this feature makes more sense:

  - Introduce global "hide everything" states (T8909) so you can completely hide stuff if you want, and this represents more of a halfway state between "nuke it" and "view it".
  - Make the actual rendering better, so it says "epriestley: blah blah..." instead of just "..." -- and looks less dumb.

The real goal here is to introduce `DiffInline` and continue moving stuff from the tangled jungle of a million top-level behaviors to sensible smooth statefulness.

Test Plan:
  - Hid and revealed inlines in unified and two-up modes.
  - These look pretty junk for now:

{F4948659}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616, T12153

Differential Revision: https://secure.phabricator.com/D17861
2017-05-16 06:19:56 -07:00
epriestley
fe44e987fb Translate "Loading..." text in inline comments
Summary: Ref T12616. This cements the relationship between ChangesetList (parent container) and Changeset (child) and passes translations down so Changeset can use them to translate the text "Loading..."

Test Plan: Viewed loading changes.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616

Differential Revision: https://secure.phabricator.com/D17846
2017-05-16 06:19:01 -07:00
epriestley
64a54aac9d Merge "differential-dropdown-menus" behavior into DiffChangesetList
Summary: Ref T12616. This ends up being a little messy ("one giant function") and maybe I'll clean it up a bit later, but continue consolidating the wild jungle of behaviors into a smaller set of responsible objects.

Test Plan: Clicked all the menu options, saw them work properly. Grepped for removed methods.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616

Differential Revision: https://secure.phabricator.com/D17845
2017-05-16 06:18:26 -07:00
epriestley
2bd25d7399 Rename "DifferentialChangesetViewManager" to "DiffChangeset"
Summary: Ref T12616. This class is already mostly-reasonable as a representation of an individual changeset, so I plan to just adjust it a little bit.

Test Plan:
  - Used `git grep` to search for `ChangesetViewManager`.
  - Used `git grep` to search for `changeset-view-manager`.
  - Browsed around and interacted with changesets.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616

Differential Revision: https://secure.phabricator.com/D17842
2017-05-16 06:16:49 -07:00
epriestley
993d942117 Gently move some listeners into DiffChangesetList
Summary: Ref T12616. Put these listeners in DiffChangesetList so the wake/sleep properly for Quicksand.

Test Plan:
  - Added some logging.
  - With quicksand, moved between diffs.
  - Saw "load" and "show more" fire exactly once on each page, with the correct changeset list listener.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616

Differential Revision: https://secure.phabricator.com/D17841
2017-05-16 06:16:26 -07:00
epriestley
404fe482d9 Add a Quicksand-aware page-level container to diffs
Summary:
Ref T12616. Ref T8047. Ref T11093. We currently have several bugs where diff state sticks across Quicksand pages.

Add a new top-level object to handle this, with `sleep()` and `wake()` methods. In `sleep()`, future changes will remove/deacivate all the reticles/editors/etc.

See T12616 for high-level discussion of plans here.

This general idea is likely to become more formal eventually (e.g. for "sheets" or whatever we call them, in T10469) but I think this is probably a reasonable place to draw a line for now.

Test Plan:
  - Added some logging to sleep(), wake() and construct().
  - Viewed changes in Differential.
  - With Quicksand on, browsed around; saw state change logs fire properly.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12616, T11093, T8047

Differential Revision: https://secure.phabricator.com/D17840
2017-05-16 06:15:58 -07:00
epriestley
00313094d3 Make "View" links on Differential inline comment previews work again
Summary:
Ref T11114. Recent changes broke the links to jump to inline comments from the previews because they get hooked up with JS.

Restore the linking behavior.

Test Plan: Clicked "View" on an inline comment preview, jumped to that comment.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17131
2017-01-02 13:24:02 -08:00