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. Diffusion, only, has a "Show All Context" button which expands the full context on all changes.
I don't remember the exact history on this, but it hasn't existed in Differential for some time and no one has complained. I suspect that the "View Options > Show All Context" on each file may replace it. I can't really come up with good reasons to use it, offhand. If we want to restore it, I think global options after T1591 is promising.
{F4945561}
Test Plan:
- Loaded a commit in Diffusion, no longer saw a button.
- Grepped for relevant sigils.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17843
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: Various little fixes, mostly moves information from the "Details" section either into the curtain or into the specific watchers or members list based on user viewership. I think this page is both cleaner and more informative.
Test Plan:
Lock, Unlock, Watch, Join, various projects with multiple users.
{F4959101}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17891
Summary: In remarkup lists, it can be hard to clearly see which items still need to be completed. This makes completed items a little lighter for clarity.
Test Plan:
Review a long list with checked and unchecked items in a task.
{F4938611}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17828
Summary: Moves Conpherence to use EditEngine. This removes the "First Message" field, but I think that's ok until we have direct messaging of some sort, then maybe have built-ins cover that case.
Test Plan:
- Visit /new/ and /edit/ for creating new rooms.
- Edit a room in full conpherence
- Edit a room in durable column
- grep for METADATA calls
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11729
Differential Revision: https://secure.phabricator.com/D16677
Summary: When a notice is in a table view in a two column layout, reset the margins.
Test Plan: Visit OwnerDetails with no paths set.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17792
Summary: Ref T12600. This makes the background solid when in hovercard context.
Test Plan: Close a task, view hovercard, see solid background
Reviewers: epriestley, amckinley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12600
Differential Revision: https://secure.phabricator.com/D17788
Summary:
Ref T12635. See that task for discussion.
You can currently end up with a verified primary address but no "verified" flag on your account through an unusual sequence of address mutations.
Test Plan:
- Registered without verifying, using address "A".
- Added a second email address, address "B".
- Verified B (most easily with `bin/auth verify`).
- Changed my primary email to B.
- Before patch: account not verified.
- After patch: account verified.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12635
Differential Revision: https://secure.phabricator.com/D17785
Summary: Swaps out hovercard boring view for super cool workboard card view. Will have more diffs to add additional information down the road.
Test Plan: {F4921092}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17769
Summary: Visually these are hard to see on blue backgrounds, adds a touch more contrast. Fixes T12604
Test Plan: View as pager and dialog
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12604
Differential Revision: https://secure.phabricator.com/D17755
Summary: Uses more standard objects and more padding for reading. Removes the ToC, which is visually broken anyways.
Test Plan: Review a README.md in a local repository.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17752
Summary: Adds the ability to set a pager onto an object box directly and pick up appropriate styles.
Test Plan: grep for renderTablePagerBox, test layouts with and without a pager.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12604
Differential Revision: https://secure.phabricator.com/D17754
Summary: These icons are off center currently.
Test Plan: Review a clone uri in local repository.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17753
Summary: This adds some basic per user / per room theming for Conpherence, which should hopefully let users identify rooms from just the sidebar color.
Test Plan: Lots of threads with different colors.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17747
Summary:
Ref T12562. I think the pre-send-on-enter behavior was: disable textarea, send message, clear area on response?
That got changed but not completely, maybe. There's currently an issue here:
- Add a `sleep(3)` to `UpdateController`.
- Type "AAA".
- Press enter.
- Real fast, type "BBB".
- When the "AAA" arrives, your "BBB" is lost. Sad!
Test Plan:
- Did the thing described above; no longer lost "BBB".
- Switched threads, sent messages, couldn't find anything else this breaks. It dates from a long time ago so I think it's just pre-SOE stuff.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12562
Differential Revision: https://secure.phabricator.com/D17742
Summary:
Fixes T12599. If you're faster than the network request, we don't actually resolve your query when the data arrives.
Instead, when the data shows up, run the query if the user has typed something.
Test Plan: Pasted a filename in real fast, got results. (Previously: no results until you press something else.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12599
Differential Revision: https://secure.phabricator.com/D17739
Summary: A few more mp3s to choose from for Conpherence.
Test Plan: Test each sound in a new room.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17734
Summary: Minor pixel shifts with new header ui in place.
Test Plan: Desktop, Mobile, Tablet, with and without search and participants open
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17725
Summary:
Ref T12566. Ref T12563. This fixes three bugs with Aphlict replay stuff:
First, Conphernece would try to repaint the UI even if no thread was open. Only repaint when a thread is open.
Second, although we deduplicate JX.Leader messages, we didn't deduplicate actual notification messages. If you browsed the leader window, then it re-elected itelf as a leader and replayed history, it could rebroadcast notifications and other windows could show doubles. Deduplicate notifications to prevent this.
Third, we always replayed the last 60 seconds of history. When you browsed the leader window, whichever window became the new leader (possibly the one you just browsed) could replay messages from before it had opened, leading to duplicate messages. Particularly, after receiving a message and then browsing you could see that message again. Instead, only replay history as far back as when the window first opened.
Test Plan:
- Clicked "Repaint" with a thread open, saw a repaint. Clicked "Repaint" with Conpherence open but no thread, no repaint and no 404 request to `/update/null/`.
- In browser A, opened three windows. In browser B, sent a notification. In browser A, browsed the leader window away twice in a row. Observed that the window which never became a leader doesn't duplicate notifications.
- In browser A, opened three windows. In browser B, sent a notification. In browser A, browsed the leader window away over and over again. Observed that replay requests issued with appropriate history windows.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12566, T12563
Differential Revision: https://secure.phabricator.com/D17722
Summary: Ref T12573. This sends a "ping" to the server, and a "pong" back to the client, every 15 seconds. This tricks ELBs into thinking we're doing something useful and productive.
Test Plan: Ran `bin/aphlict debug`, loaded Phabricator, saw ping/pong in logs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12573
Differential Revision: https://secure.phabricator.com/D17717
Summary: Ref T12566. When we reconnect, refresh the current thread even if we replayed notifications.
Test Plan:
- Clicked the "Repaint" button, saw the thread refresh.
- Clicked the "Reconnect" button, saw the thread reresh.
- Launched `aphlict debug`, killed it, restarted it, saw the thread refresh after reconnect.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12566
Differential Revision: https://secure.phabricator.com/D17713
Summary:
Fixes T12564. We already had some code which seems to deal with this properly, it just wasn't getting used.
Assign each application-level notification a unique ID, then ignore messages with duplicate IDs.
Test Plan:
- In browser A, loaded `/T123`.
- In browser B, loaded `/T123`.
- Made a comment as B.
- Saw notification as A.
- Mashed "Replay" a bunch.
- Before patch: piles of duplicate notifications.
- After patch: no duplicates.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12564
Differential Revision: https://secure.phabricator.com/D17710
Summary:
Fixes T12563. If we've ever seen an "open", mark all future connections as reconnects. When we reconnect, replay recent history.
(Until duplicate messages (T12564) are handled better this may cause some notification duplication.)
Also emit a reconnect event (for T12566) but don't use it yet.
Test Plan: {F4912044}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12563
Differential Revision: https://secure.phabricator.com/D17708
Summary:
Fixes T12567. We currently retry after 2s, 4s, 8s, 16s, ...
If we connected cleanly once, retry the first time right away. There are a bunch of reasonable cases where this will work fine and we don't need to wait. Then we fall back: 0s, 2s, 4s, 8s, ...
Test Plan: {F4911905}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12567
Differential Revision: https://secure.phabricator.com/D17706
Summary: Ref T12568. Ref T12567. Allows you to force a reconnect, and shows the reconnect delay on connection close/failure.
Test Plan: {F4911879}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12568, T12567
Differential Revision: https://secure.phabricator.com/D17705
Summary:
Ref T12573. `JX.Leader` synchronizes the Aphlict connection across multiple windows.
Currently, we only test to see if the leader window has been closed every 16 seconds. Instead, test every 1.5 seconds.
Also, make windows keep trying to become the leader forever. This was removed previously (in D15806) but I think that change decreased robustness here.
Test Plan:
- Opened two windows to the "Realtime" tab in DarkConsole.
- Saw one become the leader and one become a follower.
- (Optionally, wait for 10 seconds here to test the "keep trying to become the leader" behavior.)
- Closed the leader.
- Saw the follower become the leader after ~1.5 seconds.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12573
Differential Revision: https://secure.phabricator.com/D17703
Summary: Some space is bleeding in here from two-column-css. Re-scope CSS.
Test Plan: Review creating a task on mobile with document preview present.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17712
Summary: Cleans up the topic UI a little more, I think this feels nice for some reason.
Test Plan: visit a room with and without a topic, desktop, tablet, mobile
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17711
Summary: Fixes T12577. Some tweaks last week widened the default buttons, but these didn't get retouched.
Test Plan: Review calendar on desktop and mobile.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12577
Differential Revision: https://secure.phabricator.com/D17709
Summary: Fixes T12556 Uses more common components in ConpherenceThreadList by moving to PHUIListItemView. Reduces clutter by moving privacy into the header. Gets ride of "See More" double interchanges.
Test Plan:
I need to test this more, doesn't seem to auto-select top room any more, also might build a lipsum generator.
- Create lots of rooms with various policies
- Test clicking on policy object
- Click on different rooms
- Post in rooms
- Load up second account, see room numbers
- Clear room message count by clicking on room
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12556
Differential Revision: https://secure.phabricator.com/D17698
Summary: Ref T12538. I missed this in D17695, which renamed the variable. The logic was also a little off since `jj` is an index, not a count.
Test Plan: Typed `con` in global search, which hits "Con-pherence", "Con-duit" and "Con-fig", plus a bunch of other stuff. Got results after patch.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12538
Differential Revision: https://secure.phabricator.com/D17700
Summary:
Fixes T12538. Instead of hiding "closed" results unless only closed results match, show closed results but sort them to the bottom.
This fixes the actual issue in T12538, and I think this is probably the correct/best behavior for the global search.
It also makes all other typeaheads use this behavior. They currently have a "bug" where enabled user `abcd` makes it impossible to select disabled user `abc`. This manifests in some real cases, where enabled function `members(abc)` makes it impossible to disabled user `abc` in some function tokenizers.
If ths feels worse, we could go back to filtering in the simpler cases and introduce a rule like "show closed results if only closed results would be shown OR if query is an exact match for the disabled result", but that gets dicier because "exact match" is a fuzzy concept.
(There are a lot of other minor bad behaviors that this doesn't try to fix.)
Test Plan:
Enabled project "instabug" no longer prevents bot user "instabug" from being shown:
{F4903843}
Disabled user "mmaven" is sorted below enabled user "mmclewis", in defiance of the otherwise alphabetical order. There's no visual cue that this user is disabled because of T6906.
{F4903845}
Same as above, but this source renders "disabled" in a more obvious way:
{F4903848}
Function selecting members of active project `members(instabug)` no longer prevents selection of bot user `instabug`:
{F4903849}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12538
Differential Revision: https://secure.phabricator.com/D17695
Summary: Will see how this goes in practice. Uses violet where color is used for non responsive peeps.
Test Plan: Create a user without email verification, test hover card, profile, mentions and lists.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17678
Summary: This moves the count on the Conpherence Menu Item into a phui-list-item-count, and removes the CSS call to the entire Conphrence stack when durable column is open.
Test Plan: Test with and without the chat column, and a menu with a count
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17677
