Summary: Ref T12733. This adds classes for unsubmitted/unsaved changes, and lays some groundwork for additional buttons.
Test Plan:
- Added, edited, deleted comments.
- Saw bar background color update appropriately.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D18045
Summary: Ref T12733. This ultimately conflicts with scrolling and took about two days to get reported as a bug/regression. See T12733 for a bunch of additional discussion. See T1026 for original discussion.
Test Plan:
- Left single-line and multi-line comments on desktop.
- Tapped to leave single-line comments on mobile.
- Dragged lines on mobile, got a scroll instead of a range comment.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D18044
Summary: Ref T12733. Completely removes the objectives UI.
Test Plan:
- Grepped for `objective`, etc.
- Browsed revisions, no JS errors / broken stuff.
- (If I missed anything, it's likely to turn up in followup changes.)
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D18043
Summary:
Fixes T12779. Currently, we scroll down if the midline of the changeset is above the midline of the viewport.
This rule can cause us to scroll improperly when loading changesets //after// jumping to their anchors, since the changeset we want to look at will likely have a midpoint above the document midline. That is, we follow an anchor to `X.c`, then it loads, then we scroll past it.
Instead, scroll only if the changeset is (almost) entirely above the viewport.
Test Plan:
Followed an anchor to `PHUIFeedStoryExample`:
{F4984154}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12779
Differential Revision: https://secure.phabricator.com/D18052
Summary: Ref T12733. When an inline is selected, make it stand out so you can see where you are in the document more clearly.
Test Plan: {F4968509}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D17982
Summary: Ref T12733. Show a "reply" icon for replies, and make them stack directly under their parent.
Test Plan: {F4968500}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D17981
Summary: Ref T12733. Shows a comment snippet when hovering inlines in the objective list.
Test Plan: {F4968490}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D17980
Summary: Ref T12733. Currently, creating a new inline and then canceling it leaves a marker in the objective list. Instead, remove the marker.
Test Plan:
- Created an empty inline, cancelled. Created a non-empty inline, cancelled. No objective marker in either case.
- Created a new normal inline, objective marker.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D17979
Summary:
Ref T12733. When creating a new comment, the objective could appear to far up in the scrollbar because we were anchoring it to an invisible row.
Instead, anchor to the "edit" row while editing.
Test Plan: Created a new comment at the very top of a file, saw "File, Star" icons instead of "Star, File".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D17978
Summary:
Ref T12733.
- While editing a comment, show a pink star ({icon star, color=pink}) with a tooltip.
- Slight UI tweaks, including draft comments getting an indigo pencil ({icon pencil, color=indigo}).
Test Plan: {F4968470}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D17977
Summary: Ref T12733. Currently, long filenames get cut off at 160px. Instead, don't cut them off.
Test Plan:
Before:
{F4968401}
After:
{F4968402}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D17975
Summary:
Ref T12733. In the longer run I'd like to just push this out from the edge, but that currently gets us into trouble since we start bumping into content. On my system, the trackpad scrollbar also expands in size when moused over, so the minimum number of pixels we need to push it out is approximatley 15px. This hits body content and the persistent chat.
For now, just disable this element on trackpad systems.
Test Plan:
Disconnected all USB peripherals, quit and relaunched Safari, saw no objective list.
Reconnected mouse, relaunched Safari, saw objective list.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D17974
Summary: See D17955.
Test Plan: Loaded a revision, no longer saw annotations with prototypes off. Still saw annotations with prototypes on.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17983
Summary:
Ref T12733. Currently, we detect the changeset which is in the middle of the screen as the current changeset.
This doesn't always get us the most intuitive changeset, particularly after a navigation from the scroll objective list: when you jump to changeset "X", you'd tend to expect "X" to be shown in the header, but the //next// changeset may be shown if "X" is too short.
Instead, select the changeset near the top of the screen (spanning an invisible line slightly below the banner).
Test Plan: Scrolled and jumped through a document with long and short changesets, saw a more intuitive changeset selected by the banner.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D17976
Summary:
Minor UI tweaks:
- Use the dynamic icon for each file (e.g., image, text), not a hard-coded icon.
- Render the path (less important) in grey and the filename (more important) in black.
Test Plan: {F4966176}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17957
Summary:
Add important objectives (like waygates and quest markers) to the minimap.
This also probably fixes @cspeckmim's bug with the {key @} keyboard shortcut.
Test Plan:
(This is probably easier to undestand if you `arc patch` + click around.)
{F4966037}
Reviewers: chad, amckinley
Reviewed By: chad
Subscribers: cspeckmim
Differential Revision: https://secure.phabricator.com/D17955
Summary:
Fixes T1591. This was removed long ago because it was a mess to implement and caused a bunch of weird issues, and also my tolerance for dealing with weird JS issues was much, much lower.
I have now survived the fires of JX.Scrollbar and would love to address 200 small nitpicks about obscure browser behaviors on Linux, so open the floodgates again.
A secondary goal here is to create room to add a global view state menu on the right, with 300 options like "hide all inlines", "hide done inlines", "hide collapsed inlines", "hide ghosts", "show ghosts", "enable filetree", "disable filetree", etc, etc. Not sure how much of this I'll actually do. I have one more experiment I want to try first.
Test Plan: {F4963294}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T1591
Differential Revision: https://secure.phabricator.com/D17945
Summary:
Ref T12616. This puts "h" back to collapse or expand the current file.
This removes some very complicated/messy code around following links in the table of contents and getting files auto-expanded. I suspect no one will miss this, but we can restore it if ayone notices.
Test Plan: Pressed "h" to collapse/expand a file. Also used the menu items.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17940
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
