Summary: Fixes T7539. We need to set the "with-column" css class on the document body to make things like the jx-mask style-able. Also, make the global upload control only do it for the standard phabrcator page and not the document body.
Test Plan: dragged a file to conpherence column and it worked! uploaded a file to homepage with column open and it worked! uploaded a file to /file/ with column open and it worked!
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7539
Differential Revision: https://secure.phabricator.com/D12055
Summary:
Ref T7014. This makes it so
- you can't invoke the column from a device
- if you are in desktop size and resize to tablet or phone, the column closes.
- if you resize desktop -> device -> desktop, the column closes at device size and reopens at desktop size
- if you load, then resize device -> desktop the column opens if the user has that preference
- there is a brief flicker when you load on 'device' with the column open preference. it lasts as long as the js stack takes to calculate the device css rule.
Test Plan: see summary but i did stuff to do all that
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7014
Differential Revision: https://secure.phabricator.com/D12052
Summary: Ref T7014. This changes the title and selected icon right as the user clicks it. This could //maybe// be in the "willLoadThread" callback hook, but it doesn't happen every time we load a thread, just **this** time so keep it right in the listener for now.
Test Plan: switched some threads and liked what I saw
Reviewers: epriestley, chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T7014
Differential Revision: https://secure.phabricator.com/D12043
Summary: When you open the column, keep it open on future requests.
Test Plan: Opened column, clicked to Conpherence (no column), clicked elsewhere (column again), reloaded page (column), closed column, clicked something (no column).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12038
Summary:
Fixes T7060. Removes some hard-coding.
This assumes that "pages with no durable column" and "pages with no Quicksand" are the same, but that's correct today and I can't come up with a use case where they'd be different offhand.
Test Plan:
- Clicked a revision with column open, got Quicksand navigation.
- Clicked into Conpherence with column open, got real navigation.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7060
Differential Revision: https://secure.phabricator.com/D12036
Summary: Ref T7380. This does the most basic thing ever and sticks up to 6 icons in there.
Test Plan: clicked the icons and noted new conpherences loaded in nicely
Reviewers: epriestley, chad
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7380
Differential Revision: https://secure.phabricator.com/D12037
Summary:
Ref T5369. New HTML5 version without flash dependencies.
This doesn't play any sounds.
Test Plan: Did not play any sounds.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T5369
Differential Revision: https://secure.phabricator.com/D9535
Summary: Ref T7014. The main conpherence view is kind of broken without this in subtle ways because of /conpherence/ versus /conpherence/x/ init'ing things differently; this fixes that. Moves more normal view conpherence logic into threadManager. Makes all the display code happen outside of threadManager, setting us up for some display manager later maybe.
Test Plan: sent messages, updated title, etc and the messages pane auto scrolled correctly!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7014
Differential Revision: https://secure.phabricator.com/D12035
Summary:
Ref T7014. Fixes T7473. This adds a class to handle thread state about what thread is loaded and what transaction we've seen last. It is deployed 100% in the durable column and only partially deployed in the regular view. Future diff(s) should clean up regular view. Note ConpherenceThreadManager API might change a bit at that time.
Also includes a bonus bug fix so logged out users can't toggle this column
Test Plan: tried to use durable column while logged out and nothing happened. sent messages, aphlict-received messages, added people, and changed title from both views
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7473, T7014
Differential Revision: https://secure.phabricator.com/D12029
Summary:
Ref T2009. Ref T1460.
Fixes T2618. When users hit "Delete" on inline comments, delete immediately and offer them "Undo". If they delete indirectly (e.g., by clicking "Delete" from the preview at the bottom of the page), we still prompt them, because the "Undo" action either won't be available or may not be easy to find. This is a "refdelete".
Fixes T6464. This was just a mess. Make it not as much of a mess. It should work now. Pretty sure.
Fixes T4999. We did not refresh these links often enough to find targets for them, so they could race with content. Reevaluate them after loading new changes.
Test Plan:
- Deleted and undid deletion of inlines from main view and preview.
- Clicked "View" on inlines.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6464, T4999, T2618, T1460, T2009
Differential Revision: https://secure.phabricator.com/D12032
Summary: Ref T7014.
Test Plan: changed the conpherence title from the column. since i can't get scrolling to work, i inspect the dom to verify the title change transaction showed up properly
Reviewers: epriestley, chad
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7014
Differential Revision: https://secure.phabricator.com/D12002
Summary:
Ref T2009. This is another almost-identical copy of the row scaffolding, which has the same 1up/2up bugs as the 8 other copies of this code.
Turn the "undo" element into an InlineCommentView so we can scaffold it.
Then, scaffold it with the same code as everything else.
Test Plan: Hit "Undo", swapped from 1up to 2up, hit "undo" again, swapped back, tried left/right, everything rendered with proper scaffolding.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D12019
Summary:
Ref T1460. Track and store which comments are threaded replies to other comments, vs merely appearing on the same lines.
This doesn't actually write `hasReplies` yet, since that needs to happen when we un-draft comments on submission.
Test Plan: Made inline comments in Differential and Diffusion, including replies. Replies were marked as "Is Reply".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1460
Differential Revision: https://secure.phabricator.com/D12017
Summary:
Fixes T4452. Ref T2009. There's a hierarchy of changeset rendering power: only low-level calls, use of ChangesetDetailView, then use of ChangesetListView (a list of DetailViews).
Prior to work here, the various changeset rendering controllers got their hands dirty to varying degrees, with some using only the lowest-level rendering pipeline:
- Phriction: no view (lowest level)
- Diffusion: DetailView
- Differential Changeset: DetailView
- Differential Diff: ListView
- Differential Revision: ListView
I brought Phriction up to use DetailView, but want to bring everything all the way up to use ListView. Each composition layer adds more features to diff browsing. In particular, this change enables "Highlight As", switching 1up vs 2up, adding inlines, etc., on the standalone view.
Test Plan:
- Viewed a changeset standalone. Could change highlighting, switch 1up vs 2up, add and edit inlines, etc.
- Viewed a revision; no behavioral changes.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4452, T2009
Differential Revision: https://secure.phabricator.com/D12012
Summary:
Ref T2009. Currently, the code figures out if a comment is on the left or right by looking at the `<th />` preceeding the enclosing `<td />`.
This gets the right result in 2-up, but in 1-up rows are always `<th />`, `<th />`, `<td />`, so it always detects every inline as being in the new file.
Because "old" and "new" cells aren't inherently distingushable in the 1up view, we can't use a DOM test for this at all. Instead, just track this state explicitly.
Test Plan:
- Made left/right comments in 1up view and 2up view.
- Viewed them in 1up and 2up views.
- Hovered in 1up and 2up views.
- Diff-of-diff'd and reviewed old/new comments, then made some more.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D12011
Summary:
Ref T2009. Right now, when you mouse over a line number, we change the cursor to a "pointer", but that's the only hint we provide about the existence of inline comments.
Occasionally, users have reported confusion around how to leave inline comments.
Try to increase discoverability by showing the line reticle when you hover over the line.
(I could take this or leave it, but it seems OK / not annoying after 15 seconds of playing with it.)
Test Plan: Waved cursor over line numbers, attempted to test all the editor/noncontiguous/across-files cases.
Reviewers: btrahan, chad
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D12010
Summary:
Ref T2009. Currently, the code which draws the reticle is sort of implicitly hard-coded with some of the rules for the 2up view.
Instead, use general rules:
- Start selection at the next `<td />`.
- End selection at the rightmost adjacent `<td />`.
These rules work in all cases.
Test Plan:
- Activated reticle in 1up and 2up views by clicking line numbers and hovering over comments. It now draws correctly.
- Dragged over line ranges in 1up and 2up views, saw accurate reticle.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D12009
Summary:
Ref T7014. This diff addresses
- getting it to be the right set of options
- add participant
- view in conpherence
- close window
- making those options work
- make it so if you are on /conpherence/ you can't toggle the durable column
Test Plan: inspected dom via chrome tools and found last transaction. added a participant and inspected the single new transactin added for accuracy. used view in conpherence action to view in conpherence. used close window action to close window
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7014
Differential Revision: https://secure.phabricator.com/D11991
Summary:
Ref T2009. This reduces how buggy inlines are. They're still buggy.
Specifically, the inline endpoint didn't know how to scaffold inlines before, so some of them ended up rendering in the wrong rows or breaking layouts.
This passes the current renderer through to the inline editor endpoint, so it can at least get the layout correct.
Test Plan: Interacted with inlines in unified and side-by-side views.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D11988
Summary:
These aren't being populated yet; they mostly fix some JS errors with inlines.
For example, the inline hover reticle relies on adjusting its width to account for the "copy" column, and failed when the column did not exist.
Test Plan:
- Hovering inlines in unified now works, mostly.
- Interacted with inlines in side-by-side.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11985
Summary: Ref T2009. It doesn't make sense to have these as separate behaviors. We require a ChangesetViewManager to track view parameter state.
Test Plan: Interacted with changesets in Phriction, Differential and Diffusion.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D11979
Summary:
Ref T2009. Currently, we do not persist view parameters when making context rendering requests.
The big one is the renderer (1up vs 2up). This makes context on unified diffs come in with too many columns.
However, it impacts other parameters too. For example, at HEAD, if you change highlighting to "rainbow" and then load more context, the context uses the original highlighter instead of the rainbow highlighter.
This moves context loads into ChangesetViewManager, which maintains view parameters and can provide them correctly.
- This removes "ref"; it is no longer required, as the ChangesetViewManager tracks it.
- This removes URI management from `behavior-show-more`; it is no longer required, since the ChangesetViewManager knows how to render.
- This removes "whitespace" since this is handled properly by the view manager.
Test Plan:
- Used "Show Top" / "Show All" / "Show Bottom" in 1-up and 2-up views.
- Changed file highlighting to rainbow, loaded stuff, saw rainbow stick.
- Used "Show Entire File" in 1-up and 2-up views.
- Saw loading chrome.
- No loading chrome normally.
- Made inlines, verified `copyRows()` code runs.
- Poked around Diffusion -- it is missing some parameter handling, but works OK.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D11977
Summary: Ref T2009. These aren't good enough to actually use so I won't land this yet, but it makes testing changes a lot easier.
Test Plan:
- Swapped setting.
- Loaded revisions.
- Saw setting respected.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D11972
Summary:
Ref T7014. This hooks up the durable column such that when you open it up it loads your most recent Conpherence. You can then switch amongst the various widgets and stuff and everything works nicely.
Except...
- scroll bar does not work
- also doesn't work at HEAD when I add a ton of text to the UI with no changes? (wrapped $copy in array_fill(0, 1000, $copy))
- "widget selector" does not collapse when you select something else
- this part wasn't really specified so I used the aphlict dropdown stuff. didn't want to keep working on that if this was the wrong UI choice
- can not edit title
- do we still want that to be done by clicking on the title, which pops a dialogue?
- can not add participants or calendar events
- what should this UI be? maybe just a button on the top for "participants" and a button on the bottom for calendar? both on top?
- this is not pixel perfect to the mock or two I've seen around. Aside from generally being bad at that, I definitely didn't get the name + timestamps formatting correctly, because the standard DOM of that has timestamp FIRST which appears second due to a "float right". Seemed like a lot of special-casing for what might not even be that important in the UI so I punted. (And again, there's likely many unknown ways in which this isn't pixel perfect)
There's also code quality issues
- `ConpherenceWidgetConfigConstants` is hopefully temporary or at least gets more sleek as we keep progressing here
- copied some CSS from main Conpherence app
- DOM structure is pretty different
- there's some minor CSS tweaks too given the different width (not to mention the DOM structure being different)
- copied some JS from behavior-pontificate.js to sync threads relative to aphlict updates
- JS in general is like a better version of existing JS; these should collapse I'd hope?
- maybe the aphlict-behavior-dropdown change was badsauce?
...but all that said, this definitely feels really nice and I feel like adding stuff is going to be really easy compared to how normal Conpherence is.
Also includes a bonus bug fix - we now correctly update participation. The user would encounter this issue if they were in a conpherence that got some updates and then they went to a different page; they would have unread status for the messages that were ajax'd in. This patch fixes that by making sure we mark participation up to date with the proper transaction in all cases.
Test Plan: hit "\" to invoke the column and saw nice loading UI and my latest conpherence load. sent messages and verified they received A-OK by looking in DOM console. toggled various widges and verified they rendered correctly. opened up a second browser with a second user on the thread, sent a message, and it was received in a nice asynchronous fashion
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7014
Differential Revision: https://secure.phabricator.com/D11968
Summary: This filename is wrong ("phame" should be "passphrase").
Test Plan: Read filename.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11934
Summary: This was missed in a recent rename.
Test Plan: No more console exception.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11916
Summary: Fixes T7135. Also does a bit of a javascript cleanup in that we had an event - "conpherence-selectthread" - which really didn't need to be an event.
Test Plan: selected various conpherences from the list and they loaded correctly, including putting the cursor at the end of the text as appropriate. send many messages rapid fire without ever taking my hands off the keyboard.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7135
Differential Revision: https://secure.phabricator.com/D11890
Summary: Fixes T7099, also picked some new colors. Raphael can bind the graph to a dom element, which resolved the scrolling issue.
Test Plan: Tested scrolling on my laptop, desktop. Seems resolved.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
Subscribers: Korvin, epriestley
Maniphest Tasks: T7099
Differential Revision: https://secure.phabricator.com/D11879
Summary:
Fixes T5039. The trick / possibly lame part here is we only match 1 application email and its undefined which one. e.g. if a user emails us at address x, y, and z only one of those will pick up the mail. Ergo, don't let users define non-sensical herald conditions like "matches all". Also document what I think was non-intuitive about the code with an inline comment; we have to return an array with just a phid from an object and out of context it feels very "what the...???"
Note this needs to be deployed to other applications still, but I think its okay to close T5039 aggressively here since its done from a user story perspective.
Test Plan: set up a herald rule to flag tasks created as blue via app email x. sent an email to x via `bin/mail receive-test` and verified the task had the blue flag
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5039
Differential Revision: https://secure.phabricator.com/D11564
Summary: Fixes T7084. This doesn't use the same anchor logic as other applications.
Test Plan: `$245` lines now jump to line 245 on page load.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7084
Differential Revision: https://secure.phabricator.com/D11563
Summary: Use `x.y` in favor of `x['y']` in //some// JavaScript callsites. Note that there are a bunch of places where the latter is explicitly used to trick `PhabricatorJavelinLinter`.
Test Plan: `arc lint`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11442
Summary:
Fixes T7069. When jumping to a comment anchor, we get the scroll positions wrong.
Partly this is fixing some calcaulations; partly, the "show older comments" and "scroll anchor" stuff were fighting over the scroll position. Since the anchor can take care of things on its own, just let it handle stuff.
Test Plan:
- Clicked comment anchors.
- Loaded pages with anchors in the URI.
- Loaded pages with anchors hidden behind "show older comments".
In all cases, got the right scroll position.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7069
Differential Revision: https://secure.phabricator.com/D11540
Summary:
Ref T2086. Ref T7014. With the persistent column, there is significant value in retaining chrome state through navigation events, because the user may have a lot of state in the chat window (scroll position, text selection, room juggling, partially entered text, etc). We can do this by capturing navigation events and faking them with Javascript.
(This can also improve performance, albeit slightly, and I believe there are better approaches to tackle performance any problems which exist with the chrome in many cases).
At Facebook, this system was "Photostream" in photos and then "Quickling" in general, and the technical cost of the system was //staggering//. I am loathe to pursue it again. However:
- Browsers are less junky now, and we target a smaller set of browsers. A large part of the technical cost of Quickling was the high complexity of emulating nagivation events in IE, where we needed to navigate a hidden iframe to make history entries. All desktop browsers which we might want to use this system on support the History API (although this prototype does not yet implement it).
- Javelin and Phabricator's architecture are much cleaner than Facebook's was. A large part of the technical cost of Quickling was inconsistency, inlined `onclick` handlers, and general lack of coordination and abstraction. We will have //some// of this, but "correctly written" behaviors are mostly immune to it by design, and many of Javelin's architectural decisions were influenced by desire to avoid issues we encountered building this stuff for Facebook.
- Some of the primitives which Quickling required (like loading resources over Ajax) have existed in a stable state in our codebase for a year or more, and adoption of these primitives was trivial and uneventful (vs a huge production at Facebook).
- My hubris is bolstered by recent success with WebSockets and JX.Scrollbar, both of which I would have assessed as infeasibly complex to develop in this project a few years ago.
To these points, the developer cost to prototype Photostream was several weeks; the developer cost to prototype this was a bit less than an hour. It is plausible to me that implementing and maintaining this system really will be hundreds of times less complex than it was at Facebook.
Test Plan:
My plan for this and D11497 is:
- Get them in master.
- Some secret key / relatively-hidden preference activates the column.
- Quicksand activates //only// when the column is open.
- We can use column + quicksand for a long period of time (i.e., over the course of Conpherence v2 development) and hammer out the long tail of issues.
- When it derps up, you just hide the column and you're good to go.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T2086, T7014
Differential Revision: https://secure.phabricator.com/D11507
Summary:
Fixes T7054. Fixes T7049.
- Stop scrolling Differential reticles when the page scrolls.
- Make dialogs aware of multi-panel UI.
Test Plan:
- Dialogs pop up in the right place.
- Inline + scroll now longer leaves the inline in a fixed position.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7049, T7054
Differential Revision: https://secure.phabricator.com/D11523
Summary:
Ref T7014. This is very rough and not hooked up to anything, but gets a couple of the layout pieces in place so we can (a) see that it looks like it'll kinda work; (b) look for problematic interactions and (c) you can fix my mangling of your design.
NOTE: Press "\" to toggle the column.
Test Plan:
Feels pretty good to me?
{F275722}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7014
Differential Revision: https://secure.phabricator.com/D11497
Summary: Fixes T7033. When we've reframed the main page content we need to scroll relative to the containing frame, not relative to the window.
Test Plan:
In Safari, Chrome and Firefox, used j/k/J/K keys to navigate diff content.
Tried some other scroll-based beahviors, like jump-to-anchors.
(It looks like the highlighting reticle got slightly derped a while ago, but it's still functional, so I didn't mess with it.)
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7033
Differential Revision: https://secure.phabricator.com/D11490
Summary:
Ref T7014. With a mouse plugged in, multi-panel UIs are pretty hideous on OSX. This is somewhat offputting for me in Conpherence, and really jumps out at me with the new column mocks in T7014.
Sites like Twitch and Facebook approach this by emulating the touchpad scrollbar to achieve a more aesthetic UI. Use a similar approach.
This:
- Replaces the main scrollbar with a prettier fake one.
- This prepares the standard page frame for a persistent chat column.
Test Plan:
- Seems to work properly on OSX, Chrome and Firefox. Haven't tested on IE; my Windows setup is pretty iffy at the moment.
- Tried Conpherence.
- Tried Workboards.
Reviewers: btrahan, chad
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7014
Differential Revision: https://secure.phabricator.com/D11472
Summary: Fixes T5344. Essentially, we only make the AJAX request to `/notification/individual/` if we are the leader tab (i.e. only one tab will make this request). Once a response has been received from the server (containing the contents of the notification), we broadcast the message contents back to all other tabs for rendering.
Test Plan:
Opened two tabs on `/notification/status/` and clicked "Send Test Notification".
**Before**
```lang=bash, name=tail -f /var/log/phabricator-access.log | grep /notification/individual/
[Tue, 13 Jan 2015 20:10:37 +1100] 17033 phabricator 10.0.0.1 josh PhabricatorNotificationIndividualController - /notification/individual/-200 236036
[Tue, 13 Jan 2015 20:10:37 +1100] 17657 phabricator 10.0.0.1 josh PhabricatorNotificationIndividualController - /notification/individual/-200 24130
```
**After**
```lang=bash, name=tail -f /var/log/phabricator-access.log | grep /notification/individual/
[Tue, 13 Jan 2015 20:11:15 +1100] 17657 phabricator 10.0.0.1 josh PhabricatorNotificationIndividualController - /notification/individual/-200 180217
```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5344
Differential Revision: https://secure.phabricator.com/D11360
Summary: Just some housekeeping... mostly just removing some unused variables.
Test Plan: Checked that I was still about to receive notifications from `/notification/status/`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11398
Summary:
Fixes T6559. No more flash, use Websockets. This is less aggressive than the earlier version, and retains more server logic.
- Support "wss".
- Make the client work.
- Remove "notification.user" entirely.
- Seems ok?
Test Plan:
In Safari, Firefox and Chrome, saw the browsers connect. Made a bunch of comments/updates and saw notifications.
Notable holes in the test plan:
- Haven't tested "wss" yet. I'll do this on secure.
- Notifications are //too fast// now, locally. I get them after I hit submit but before the page reloads.
- There are probably some other rough edges, this is a fairly big patch.
Reviewers: joshuaspence, btrahan
Reviewed By: joshuaspence, btrahan
Subscribers: fabe, btrahan, epriestley
Maniphest Tasks: T6713, T6559
Differential Revision: https://secure.phabricator.com/D11143
Summary:
Ref T6713. This isn't very clean, and primarily unblocks D11143.
After D11143, I have a reliable local race where I submit, get a notification immediately, then get a double update (form submission + notification-triggered update).
Instead, make the notification updates wait for form submissions.
This doesn't resolve the race completely. The notification updates don't block chat submission (only the other way around), so if you're really fast you can submit at the same time someone else sends chat and race. But this fixes the most glaring issue.
The overall structure here is still pretty shaky but I tried to improve things a little, at least.
Test Plan: Chatted with myself, saw 0 races instead of 100% races.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T6713
Differential Revision: https://secure.phabricator.com/D11277
Summary: Fixes T6179. This makes the interaction where users remove a task from a workboard much more pleasant.
Test Plan: Loaded up workboard for "A Project". Edited tasks and if / when I removed "A Project" they disappeared on save.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6179
Differential Revision: https://secure.phabricator.com/D11259
Summary: This should be a local variable, not a global variable. This silences a few JSHint warnings.
Test Plan: `arc lint`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11070
Summary: This variable should be local, not global. This silences a few JSHint warnings.
Test Plan: `arc lint`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11068
Summary: This variable should be local, not global. This silences a bunch of JSHint warnings.
Test Plan: `arc lint`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11067
Summary: This should be a fairly minor change that silences a bunch of JSHint warnings.
Test Plan: `arc lint` showed less warnings.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11064
Summary: (some) international keyboard layouts can not type "~" in a way as to trigger this so use "@" instead. Save the suggested "+" as that seems like it would be useful for some future "adding stuff" keyboard workflow. Pretty stoked to get this squared away as I am quite confident our unreleased product will now be a huge smashing success. Ref T6683.
Test Plan: made sure my choice was okay via https://en.wikipedia.org/wiki/Dead_key#Dead_keys_on_various_keyboard_layouts; used the "@" key to show all transactions
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6683
Differential Revision: https://secure.phabricator.com/D10983
Summary: I didn't get this quite right.
Test Plan:
- Clicked to open, saw white, then closed by:
- Clicking document outside menu;
- clicking menu icon again;
- clicking a different menu icon.
- In all three cases, got correct close + un-white behavior.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10967
Summary: I think this is what you're after?
Test Plan: clicky clicky
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10966
Summary: Fixes T6683.
Test Plan: clicked the yellow box and it worked! pressed '~' and it worked!
Reviewers: epriestley, chad
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6683
Differential Revision: https://secure.phabricator.com/D10932
Summary: we still need to be pager-sensitive, but otherwise this "show all" stuff is dead, dead dead...! Ref T4712. I think we can close the book on T4712 with one more diff to clean up the array_reverse / reverse paging stuff? That diff is probably a bit tricky as it involes auditing every TransactionQuery callsite...
Test Plan: viewed a task with a lot of transactions. clicked "show older" and it worked!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712
Differential Revision: https://secure.phabricator.com/D10926
Summary: Fixes T6693.
Test Plan:
Made a bunch of comments on a diff with differential, being sure to leave inlines here and there. This reproduced the issue in T6693. With this patch this issue no longer reproduces!
Successfully "showed older changes" in Maniphest too.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6693
Differential Revision: https://secure.phabricator.com/D10931
Summary: Ref T4712. This adds pagination. Future diffs will need to deploy `buildTransactionTimeline` everywhere and massage this stuff as necessary if we hit any special cases.
Test Plan: Set page size to "5" to make it need to paginate often. Verified proper transactions loaded in and the javascript actions worked.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712
Differential Revision: https://secure.phabricator.com/D10887
Summary: Fixes T5015, Allow Herald rules for Maniphest to act on task status changes.
Test Plan: Create Herald rule for Maniphest tasks to flag a task with status "wontfix". Change status of Maniphest task to "wontfix". Task should be flagged.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5015
Differential Revision: https://secure.phabricator.com/D10842
Summary: Right now, if no comment is selected the JS executes and throws an exception. Instead, if nothing is selected just do nothing. Fixes T6107.
Test Plan: opened up a commit in diffusion with an inline comment. pressed 'r' and saw no exceptions and nothing happen. pressed 'n' to select the next inline comment and then 'r' and it worked. opened up a commit in diffusion without any inline comments. pressed 'r' and saw no exceptions and nothing happen. opened up a diff in differential with an inline comment. pressed 'r' and saw no exceptons and nothing happened. pressed 'n' to select the next inline comment and then 'r' and it worked.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6107
Differential Revision: https://secure.phabricator.com/D10843
Summary: Fixes T5368. Synchronizes the page title to reflect unread counts in the notification and Conphernece messages menus.
Test Plan: {F201083}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5368
Differential Revision: https://secure.phabricator.com/D10457
Summary:
Fixes T5979. There are three issues here:
- We cache document positions when you pick an item up, but don't recalculate them after you scroll, so they get out of date. Dirty the cache when the user scrolls.
- When we rebuild the cache during a drag (previously, this never happened), the position of the object you're dragging is computed wrong (since it has been moved to be under the cursor). Adjust the effective position of the object you've picked up to put it back in the right place in the list.
- When you fiddle around at the bottom of a column you can get jumpy redraws as the height adjusts. Put `min-height` on the container during a drag to prevent this.
Test Plan: In Safari, Chrome and Firefox, dragged items around on columns before and after scrolling the workboard panel.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5979
Differential Revision: https://secure.phabricator.com/D10455
Summary: Fixes T3913.
Test Plan: messed around with a comment on a commit and saw preview still updating correctly
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T3913
Differential Revision: https://secure.phabricator.com/D10382
Summary:
Fixes T5885. This implements optional soft point limits for workboard columns, per traditional Kanban.
- Allow columns to have a point limit set.
- When a column has a point limit, show it in the header.
- If a column has too many points in it, show the column and point count in red.
@chad, this could probably use some design tweaks. In particular:
- I changed the color of "hidden" columns to avoid confusion with "overfull" columns. We might be able to find a better color.
- UI hints for overfull columns might need adjustment.
(After T4427, we'll let you sum some custom field instead of total number of tasks, which is why this is called "points" rather than "number of tasks".)
Test Plan:
{F190914}
Note that:
- "Pre-planning" has a limit, so it shows "4/12".
- "Planning" has a limit and is overfull, so it shows "5 / 4".
- Other columns do not have limits.
- "Post-planning" is a hidden column. This might be too muted now.
Transactions:
{F190915}
Error messages / edit screen:
{F190916}
Reviewers: btrahan, chad
Reviewed By: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T5885
Differential Revision: https://secure.phabricator.com/D10276
Summary: Ref T4427. This always counts 1 task = 1 point. The tricky bit is making this update in JS.
Test Plan: {F190900}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4427
Differential Revision: https://secure.phabricator.com/D10275
Summary:
Fixes T5677.
- Instead of using `sequence == 0` to mean "this is the backlog column", flag the column explicitly.
- Migrate existing sequence 0 columns to have the flag.
- Add the flag when initializing or copying a board.
- Remove special backlog logic when reordering columns.
Test Plan:
- Migrated columns, viewed some boards, they looked identical.
- Reordered the backlog column a bunch of times (first, last, middle, dragged other stuff around).
- Added tasks to a project, saw them show up in the reordered backlog.
- Initialized a new board and saw a backlog column show up.
- Copied an existing board and saw the backlog column come over.
- Tried to hide a backlog column.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5677
Differential Revision: https://secure.phabricator.com/D10189
Summary:
Ref T5024, T4427, T5474, T5523. Instead of separate icons in the column header for "Create Task" and "Edit Column Settings", use a dropdown menu.
- T5024 will likely add a "View Standalone" option.
- T4427 needs header space to show a count.
- T5474 likely needs "Edit Triggers..." (this seems reasonable to separate from editing the name, etc.)
- T5523 likely adds "Move all tasks..." eventually.
Test Plan: {F187414}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5523, T5474, T5024, T4427
Differential Revision: https://secure.phabricator.com/D10190
Summary:
Ref T4807. This doesn't actually do anything yet, but adds a dropdown menu for choosing an ordering and gets all the UI working correctly.
This also fixes a bug where column hidden state wouldn't persist across filter changes.
(I won't land this until it does something, but the next diff will probably be a mess so this seemed like a clean place to sever things.)
Test Plan:
{F187114}
- Altered sort ordering.
- Altered hidden state and filters, verified all states persisted correctly.
- Added `phlog()` to edit/create and move controllers and verified they receive sort information.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: swisspol, chad, epriestley
Maniphest Tasks: T4807
Differential Revision: https://secure.phabricator.com/D10178
Summary: Use cutlery icon for hilarity. Ref T5768.
Test Plan: made something with remarkup in it, used 'view raw' and saw the remarkup raw in a nice little dialogue.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5768
Differential Revision: https://secure.phabricator.com/D10183
Summary:
Depends on D9806. This implements the build simulator, which is used to calculate the order of build steps in the plan editor. This includes a migration script to convert existing plans from sequential based to dependency based, and then drops the sequence column.
Because build plans are now dependency based, the grippable and re-order behaviour has been removed.
Test Plan: Tested the migration, saw the dependencies appear correctly.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9847
Summary: At least on my install, sorting was pretty borked from a type issue. (e.g. "unbreak now" of 100 sorting as less than "High" of 90). Fix this with some parseInt action. Also support adding new cards with the new colsort stuff. The clever bit here is to include the task ID in the sorting vector because the task ID wins ties at the moment I think / new tasks need to show up before older tasks when they are initially created. Fixes T5716.
Test Plan: added many "normal" priority cards and saw them fly in correctly. changed priority and moved correctly. made no edits and no moves were made correctly.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5716
Differential Revision: https://secure.phabricator.com/D10081
Summary:
Ref T4420. This doesn't share all the code it really should, and renders a little odd. Make it more standard.
(Icons aren't handled totally correctly but there's no usability impact and all that code should just get cleaned up.)
Test Plan: Used custom policy typeahead, had a more standard experience.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D9902
Summary: Ref T4420. We don't currently pass placeholder text properly, but should.
Test Plan: Saw placeholder text in Herald.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D9901
Summary: Fixes T4567. This isn't going to win design awards and we have some leaky CSS, but it works fine.
Test Plan: {F176743}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4567
Differential Revision: https://secure.phabricator.com/D9905
Summary:
Ref T5476. Currently, the task edit code assumes it knows what the UI looks like and sends back where on the column an item should be inserted.
This is buggy after adding filters, and relatively complex. Instead, send down the ordering on the whole column and sort it in the UI. This is a bit simpler overall and more general. It makes it easier to further generalize this code for T5476.
Test Plan:
- Edited a task on a board, changing priority. Saw it reorder properly.
- Edited a task on a board in a field of other tasks at the same top-level priority. Saw it refresh without reordering.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5476
Differential Revision: https://secure.phabricator.com/D9832
Summary: I'm pretty sure that `@group` annotations are useless now... see D9855. Also fixed various other minor issues.
Test Plan: Eye-ball it.
Reviewers: #blessed_reviewers, epriestley, chad
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9859
Summary: Various fixes as suggested by JSHint.
Test Plan: Eye-balled it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9783
Summary:
Ref T3116. Add a Herald action "Require legal signatures" which requires revision authors to accept legal agreements before their revisions can be accepted.
- Herald will check which documents the author has signed, and trigger a "you have to sign X, Y, Z" for other documents.
- If the author has already signed everything, we don't spam the revision -- basically, this only triggers when signatures are missing.
- The UI will show which documents must be signed and warn that the revision can't be accepted until they're completed.
- Users aren't allowed to "Accept" the revision until documents are cleared.
Fixes T1157. The original install making the request (Hive) no longer uses Phabricator, and this satisfies our requirements.
Test Plan:
- Added a Herald rule.
- Created a revision, saw the rule trigger.
- Viewed as author and non-author, saw field UI (generic for non-author, specific for author), transaction UI, and accept-warning UI.
- Tried to accept revision.
- Signed document, saw UI update. Note that signatures don't currently //push// an update to the revision, but could eventually (like blocking tasks work).
- Accepted revision.
- Created another revision, saw rules not add the document (since it's already signed, this is the "no spam" case).
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: asherkin, epriestley
Maniphest Tasks: T1157, T3116
Differential Revision: https://secure.phabricator.com/D9771
Summary: See D9719.
Test Plan:
- Used hide/show columns.
- Used "add column".
- Filtered board.
{F170133}
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9726
Summary:
Fixes T5373. Ref T5281. Several changes:
- The `marshallExceptions` thing is useful if JS throws an exception when invoked from Flash, so set it. The resulting exceptions are a little odd (not escaped correctly, e.g.) but way better than nothing.
- Put connection status in the notification menu.
- When the connection fails, try to provide contextual help where we can.
Test Plan: {F169493}
Reviewers: chad, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5281, T5373
Differential Revision: https://secure.phabricator.com/D9700
Summary:
Ref T5373. The control flow between `aphlict-listener` and `JX.Aphlict` is pretty weird right now, where the listener (which is the highest-level component) has intimate knowledge of how to put the SWF on the page.
Instead:
- Make `JX.Aphlict` a real singleton.
- Instantiate it sooner.
- Have it handle the flash setup handshake.
Test Plan: Loaded page in debug mode, saw normal flow take place.
Reviewers: joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5373
Differential Revision: https://secure.phabricator.com/D9699
Summary:
Ref T5373. This seems to work pretty much correctly.
Also stop popping bubbles and just use the log, since users find the bubbles confusing/not useful and they're not great for developers either.
Future diffs will expose more user-facing stuff.
Test Plan: Added `throw` to AphlictClient.as, got a log in the parent window.
Reviewers: joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5373
Differential Revision: https://secure.phabricator.com/D9698
Summary: As advised by JSHint.
Test Plan: I'm not really sure how to comprehensively test this. It looks okay to me.
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9677
Summary: There are a bunch of unused variables in JavaScript files. These were identified with JSHint.
Test Plan: It's pretty hard to test this thoroughly... on inspection, it seems that everything //should// be okay (unless we are doing weird things with the JavaScript).
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9676
Summary: Replaces Embed hint with where the heck you are hint.
Test Plan: Tested current and previous mock images.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5384
Differential Revision: https://secure.phabricator.com/D9658
Summary:
Fixes T5424.
- One concrete issue: drafts were not being cleared properly because `__draft__` was not set on submission. This (mostly) fixes phantom drafts.
- This ajax comment magic feels weird and floaty and generally has problems. For example, if you add subscribers or inlines, all the stuff on the page which represents those won't update automatically. Instead, just reload. Maybe we'll ajax this stuff some day, but it feels like a net negative for now.
- Also remove it from other applications where it's currently used.
- Fix an issue with inline previews.
Test Plan: Made some comments on a mock, everything worked normally like I expected it to.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5424
Differential Revision: https://secure.phabricator.com/D9649
Summary: Ref T5179. Ref T4045. Ref T832. We can now write non-utf8 hunks into the database, so try to do more reasonable things with them in the UI.
Test Plan: (See screenshots...)
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T832, T4045, T5179
Differential Revision: https://secure.phabricator.com/D9294
Summary:
Minor things
- Fades out comment icon on hover
- Adds hover to inline comment images
- moves mask position to just the image, and not the transparent border
Test Plan: Tested all of these items on various mocks
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9631
Summary: n/a
Test Plan: Added title and description to 1 of 2 mocks, toggled left and right, saw correct CSS.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9580
Summary:
Ref T4986. Instead of requiring you to know engine class names and copy/paste URLs, provide select dropdowns that use SCARY JAVASCRIPT to do magical things.
I think this is mostly reasonable, the only issue is that it's hard to create a panel out of a completely ad-hoc query (you'd have to save it, then create a panel out of the saved query, then remove the saved query). Once we develop T5307 we can do a better job of this.
Test Plan: See screenshots.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4986
Differential Revision: https://secure.phabricator.com/D9572
Summary:
- When the button is clicked, actually download the file or image.
- Add aural hints for the icon-only buttons.
- Use a "photo" icon for "view raw image", so the "arrows pointing outward" icon can be used for "fullscreen" some day.
Test Plan: Clicked link, got a download.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9574
Summary: Fixes T5283.
Test Plan: See screenshots.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5283
Differential Revision: https://secure.phabricator.com/D9549
Summary:
Ref T5359. When users upload non-image file types (PDFs, text files, whatever), Pholio currently chokes in a few places. Make most of these behaviors more reasonable:
- Provide thumbs in the required sizes.
- Predict the thumb size of these files correctly.
- Disable inline comments.
- Make "View Fullsize" and "Download" into buttons. These mostly-work. Download should probaly really download, but CSRF on forms is a bit of a pain right now.
Test Plan: See screenshots.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5359
Differential Revision: https://secure.phabricator.com/D9548
Summary:
This could probably use some refinement (and, like, explanatory text, and stronger cues about what rows and columns mean) but feels fairly good to me, at least on test data.
I didn't do any scrolling for now since we have to do full height on mobile anyway I think. I did swap it so the newer ones are on top.
Left/right navigate you among current images only, but you can click any thumb to review history.
Removed history view since it's no longer useful.
Some things that would probably help:
- Some kind of header explaining what this is ("Mock History" or something).
- Stronger visual cue that columns are related by being the same image.
- Clearer cues about obsolete/deleted images (e.g., on the stage itself?)
- Maybe general tweaks.
- Maybe a placeholder (like a grey "X") for images which have been deleted.
(I'm planning to add comment counts too, which I think will be pretty useful, but that felt good to put in another diff.)
Test Plan: See screenshots.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9543
Summary:
Currently, we limit the image size to make sure that the stage has a constant height and the entire image always fits on screen.
In practice, these don't actually seem like desirable qualities. Instead, focus on giving as many pixels as possible to the image.
Test Plan: See screenshots.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9539
Summary:
- Moves the right-hand gutter under the image.
- Moves size information to the upper right.
- This is transitional, on the way toward something more like the mocks in D9534.
Test Plan: See screenshot.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9538
Summary: This greatly simplifies inline comments while retaining their functionality. This is probably not where we want to end up, but will let us figure out what we're doing with the stage without worrying about inlines.
Test Plan: See screenshots.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9537
Summary:
- Hide inline areas on mobile, and when the mouse cursor is not on the stage.
- Show small icon markers instead of areas.
- Yellow icons show draft comments; pink icons show final comments.
Test Plan: {F166544}
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9536
Summary: Uses common CSS spacing, tweaks some colors, increases viewport size to very limits.
Test Plan:
Test large and small images, various breakpoints. Able to more easily review mocks.
{F166500}
{F166501}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9534
Summary:
Ref T4083. This needs some work (mostly in the Conpherence JS itself), but is sort of functional. In particular:
- On thread pages, add the thread as a `pageObject`.
- After updating a thread, send a new "message" event to the server.
- Share a little more event posting code.
- In the browser, use event dispatch to respond to events.
- Add a listener for the new event type.
- Update conpherence threads (this part is really yucky).
Test Plan: With multiple browser windows / browsers open, posted a message to a thread, and saw it update everywhere.
Reviewers: joshuaspence
Reviewed By: joshuaspence
Subscribers: chad, epriestley
Maniphest Tasks: T4083
Differential Revision: https://secure.phabricator.com/D9486
Summary: Ref T4324. Ref T5284. This adds server-side support for keeping track of a set of PHIDs that the Aphlict clients have subscribed to. Instead of broadcasting a notification to all clients (after which the clients can poll `/notification/individual` in order to determine whether or not they are interested in the notification), transmit notifications only to clients that have subscribed to a PHID that is relevant to the notification.
Test Plan:
I opened up two clients on the same host (incognito tabs in Chrome). Here is the output from the server:
```
> sudo ./bin/aphlict debug
Starting Aphlict server in foreground...
Launching server:
$ 'nodejs' '/usr/src/phabricator/src/applications/aphlict/management/../../../../support/aphlict/server/aphlict_server.js' --port='22280' --admin='22281' --host='localhost' --user='aphlict'
[Wed Jun 11 2014 19:10:27 GMT+0000 (UTC)] Started Server (PID 4546)
[Wed Jun 11 2014 19:10:36 GMT+0000 (UTC)] <FlashPolicy> Policy Request From ::ffff:192.168.1.1
[Wed Jun 11 2014 19:10:37 GMT+0000 (UTC)] <Listener/1> Connected from ::ffff:192.168.1.1
[Wed Jun 11 2014 19:10:37 GMT+0000 (UTC)] <Listener/1> Received data: {"command":"subscribe","data":["PHID-USER-cb5af6p4oepy5tlgqypi"]}
[Wed Jun 11 2014 19:10:37 GMT+0000 (UTC)] <Listener/1> Subscribed to: ["PHID-USER-cb5af6p4oepy5tlgqypi"]
[Wed Jun 11 2014 19:10:39 GMT+0000 (UTC)] <Listener/1> Received data: {"command":"subscribe","data":["PHID-USER-kfohe3ca5oe6ygykmioq"]}
[Wed Jun 11 2014 19:10:39 GMT+0000 (UTC)] <Listener/1> Subscribed to: ["PHID-USER-kfohe3ca5oe6ygykmioq"]
[Wed Jun 11 2014 19:10:42 GMT+0000 (UTC)] notification: {"key":"6023751084283587681","type":"notification","subscribers":["PHID-USER-cb5af6p4oepy5tlgqypi"]}
[Wed Jun 11 2014 19:10:42 GMT+0000 (UTC)] <Listener/1> Wrote Message
```
I verified (using the "Network" tab in Chrome) that an AJAX request to `/notification/individual/` was only made in the tab belonging to the user that triggered the test notification.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5284, T4324
Differential Revision: https://secure.phabricator.com/D9458
Summary: Fixes T5271. This is mostly similar to normal tab panel JS, but I think we'll eventually do async rendering and/or saved tabs so it's reasonable to split it out.
Test Plan: Toggled tabs on a tab panel, saw tab selected state change.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5271
Differential Revision: https://secure.phabricator.com/D9478
Summary: Since an error is bad and means that some sort of action is required, make Aphlict error notifications persistent (i.e. they won't dismiss until being clicked).
Test Plan: Loaded a page with `notifications.enabled` set to `true` but without the Aphlict server actually running. Noticed that the error message didn't dismiss automatically.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9423
Summary:
This currently uses a hard-coded relative path, but should not, especially after D9401.
The major effect of this is that updated .swf files might not be served properly, and we were at the whims of the server configuration for caching/versioning behavior.
Test Plan: Enabled debug notifications, saw .swf load through Celerity.
Reviewers: joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9421
Summary: Currently, the Aphlict log notifications show for the same duration as normal Aphlict notifications (12 seconds). I find this to be far to long and usually find myself clicking all of the log notifications that I am not interested in.
Test Plan: Loaded a page and saw the debug notifications dismiss after roughly three seconds.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9409
Summary:
Fixes T5273. Two issues:
- If a JX.Resource callback throws, we keep running it. Instead, make sure it gets cleaned up before raising an exception.
- The countdown timer script doesn't recover gracefully if the node has been removed from the document by the time it runs. Instead, just bail if we can't find the countdown.
Test Plan: Dumped `{Cxyz}` into a preview and got a countdown.
Reviewers: yungsters
Reviewed By: yungsters
Subscribers: epriestley
Maniphest Tasks: T5273
Differential Revision: https://secure.phabricator.com/D9399
Summary: This was broken in D9380 and caused notifications to stop working.
Test Plan: Saw notifications.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9395
Summary: Ref T4324. Currently, if the `AphlictMaster` receives dodgy data from the Aphlict server (invalid JSON, for example) then a syntax error will be thrown and the `AphlictMaster` will die. Instead, catch errors and raise a notification.
Test Plan: {F163466}
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4324
Differential Revision: https://secure.phabricator.com/D9380
Summary:
Ref T5179. Currently, all the changeset rendering logic is in the "populate" behavior, and a lot of it comes in via configuration and is hard to get at.
Instead, surface an object which can control it, and which other behaviors can access more easily.
In particular, this allows us to add a "Load/Reload" item to the view options menu, which would previously have been very challenging.
Load/Reload isn't useful on its own, but is a step away from "Show whitespace as...", "Highlight as...", "Show tabtops as...", "View Unified", "View Side-By-Side", etc.
Test Plan:
- Viewed Differential.
- Viewed Diffusion.
- Viewed large changesets, clicked "Load".
- Used "Load" and "Reload" from view options menu.
- Loaded all changes in a large diff, verified "Load" and TOC clicks take precedence over other content loads.
- Played with content stability stuff.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5179
Differential Revision: https://secure.phabricator.com/D9286
Summary: Currently, debugging (log) messages from the Aphlict client trigger a visible notification in the web UI (when `notification.debug` is enabled). After D9327, the log messages can be quite verbose and seem to "block" legitimate notifications because they are not automatically dismissed.
Test Plan: Enabled `notification.debug` and observed that the debugging notifications expired and closed after a few seconds.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9339
Summary: Takes a pass at standardizing spacing and colors for lists and tokens.
Test Plan: Tested a lot of lists, policy, timeline, quick create, diffusion.
Reviewers: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9325
Summary:
Ref T4673.
IMPORTANT: I had to break one thing (see TODO) to get this working. Not sure how you want to deal with that. I might be able to put the element //inside// the workboard, or I could write some JS. But I figured I'd get feedback first.
General areas for improvement:
- It would be nice to give you some feedback that you have a filter applied.
- It would be nice to let you save and quickly select common filters.
- These would probably both be covered by a dropdown menu instead of a button, but that's more JS than I want to sign up for right now.
- Managing custom filters is also a significant amount of extra UI to build.
- Also, maybe these filters should be sticky per-board? Or across all boards? Or have a "make this my default view"? I tend to dislike implicit stickiness.
Test Plan:
Before:
{F157543}
Apply Filter:
{F157544}
Filtered:
{F157545}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: qgil, swisspol, epriestley
Maniphest Tasks: T4673
Differential Revision: https://secure.phabricator.com/D9211
Summary: To get there, upgrade "headerless" to "headerMode". Add a new removepanel controller. Fixes T5084.
Test Plan: removed some panels to much success
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5078, T5084
Differential Revision: https://secure.phabricator.com/D9156
Summary:
Updates policy, headers, typeaheads to FA over policy icons
Need advice - can't seem to place where icons come from on Typeahead? Wrong icons and wrong colors.... it is late
Test Plan:
- grepped for SPRITE_STATUS
- grepped for sprite-status
- grepped for setStatus for headers
- grepped individual icons names
Browsed numerous places, checked new dropdowns, see pudgy people.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4739
Differential Revision: https://secure.phabricator.com/D9179
Summary:
Ref T4986. This mostly just makes tab panels a little nicer.
Maybe this will be modal (header = "none", "edit", "view") in a few diffs but we can clean it up then if so.
Test Plan: {F155491}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4986
Differential Revision: https://secure.phabricator.com/D9143
Summary:
Ref T4986. Ref T4983. Panels will soon be able to contain other panels, either via Remarkup (`{W1}`) or maybe through new types of meta-panels.
Allow panels to detect that they are being rendered very deeply and/or within themselves.
Test Plan: Faked some errors, got failed panel renders. Since panels can't //really// contain other panels yet, this doesn't really have an impact.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T4983, T4986
Differential Revision: https://secure.phabricator.com/D9140
Summary:
This gets us the ability to specify a "layout mode" and which column a panel should appear in at panel add time. Changing the layout mode from a multi column view to a single column view or vice versa will reset all panels to the left most column.
You can also drag and drop where columns appear via the "arrange" mode.
We also have a new dashboard create flow. Create dashboard -> arrange mode. (As opposed to view mode.) This could all possibly use massaging.
Fixes T4996.
Test Plan:
made a dashboard with panels in multiple columns. verified correct widths for various layout modes
re-arranged collumns like whoa.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4996
Differential Revision: https://secure.phabricator.com/D9031
Summary:
Allows you to quickly search for files within a repository. Roughly:
- We build a big tree of everything and ship it to the client.
- The client implements a bunch of Sublime-ish magic to find paths.
Test Plan: {F154007}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley, zeeg
Differential Revision: https://secure.phabricator.com/D9087
Summary:
This code is currently quite complicated because we pull history data inline for SVN files, and via ajax for everything else (SVN dirs, everything in Git and Hg).
Always pull over ajax; batch some of the queries.
Test Plan: {F34860}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, vrana, aran
Maniphest Tasks: T2683
Differential Revision: https://secure.phabricator.com/D5255
Summary: Ref T2683. Instead of sending one request for each path's history, send one request for all of it. This permits optimizations which are not currently available to us. It degrades the user experience a tiny bit in theory, but on my machine it's actually way faster already.
Test Plan: Loaded a browse page.
Reviewers: vrana, btrahan
Reviewed By: btrahan
Subscribers: epriestley, aran
Maniphest Tasks: T2683
Differential Revision: https://secure.phabricator.com/D5254
Summary:
Fixes T430. Fixes T4834. Obsoletes D7641. Currently, we do some things less-well than we could:
- We just let the browser queue and prioritize requests, so if you load a revision with 50 changes and then click "Award Token", the action blocks until the changes load in most/all browsers. It would be better to prioritize this action and queue it immediately.
- Similarly, changes tend to load in order, even if the user has clicked to a specific file. When the user expresses a preference for a specific file, we should prioritize it.
- We show a spinning GIF when waiting on requests. This is appropriate for some types of reuqests, but distracting for others.
To fix this:
- Queue all (or, at least, most) requests into a new queue in JX.Router.
- JX.Router handles prioritizing the requests. Principally:
- You can submit a request with a specific priority (500 = general content loading, 1000 = default, 2000 = explicit user action) and JX.Router will get the higher stuff fired off sooner.
- You can name requests and then adjust their prorities later, if the user expresses an interest in specific results.
- Only use the spinner gif for "workflow" requests, which is bascially when the user clicked something and we're waiting on the server. I think it's useful and not-annoying in this case.
- Don't show any status for draft requests.
- For content requests, show a subtle hipster-style top loading bar.
Test Plan:
- Viewed a diff with 93 changes, and clicked award token.
- Prior to this patch, the action took many many seconds to resolve.
- After this patch, it resolves quickly.
- Viewed a diff with 93 changes and saw a pleasant subtle hipster-style loading bar.
- Viewed a diff with 93 changes and typed some draft text. Previews populated fairly quickly and there was no spinner.
- Viewed a diff with 93 changes and clicked something with workflow, saw a spinner after a moment.
- Viewed a diff with 93 changes and clicked a file in the table of contents near the end of the list.
- Prior to this patch, it took a long time to show up.
- After this patch, it loads directly.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T430, T4834
Differential Revision: https://secure.phabricator.com/D8979
Summary:
man I sure hate Javascript
I removed the ajax-edit and ajax-remove interactions, becuase they were prohibitively complex to get working given that the entire menu has to change too. Instead, the page just reloads. This works perfectly fine in practice.
If we want to restore these in the future, we should have the server re-render the entire transaction group or something. I think very little is lost here, though.
Test Plan:
- Took all the actions.
- Used existing dropdown menus.
{F150196}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8966
Summary:
See D8966, D8973. Replace PhabricatorDropdownMenu with PHUIXDropdownMenu.
These new menus look weird on mobile because all action lists pick up a bunch of weird styles on mobile and we're now reusing the CSS.
Test Plan:
{F150425}
{F150426}
Reviewers: btrahan, chad
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8974
Summary:
Dropdown menus are entirely dynamic right now and use custom CSS. Begin rebuilding them to use ActionList CSS.
This introduces PHUIX components which are basically JS copy/pastes of the PHP PHUI components, just implemented in JS.
We have two other dropdowns: policy controls and one in Conpherence. I'll convert those, then implement D8966.
Test Plan: {F150418}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8973
Summary:
Ref T4119. This is ugly for now, but technically works.
The comment area and transaction log don't realy know about each other, so for the moment the linking is a bit manual. Differential/Maniphest are special cases anyway.
Test Plan: {F149992}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4119
Differential Revision: https://secure.phabricator.com/D8957
Summary:
Fixes T4909. Adds a "remove" link next to the edit link, which permanently hides a comment. Addresses two use cases:
- Allowing administrators to clean up spam.
- Allowing users to try to put the genie back in the bottle if they post passwords or sensitive links, etc.
The user who removed the comment is named in the removal text to enforce some level of administrative accountability.
No data is deleted, but there's currently no method to restore these comments. We'll see if we need one.
This is cheating a little bit by storing "removed" as "2" in the isDeleted field. This doesn't seem tooooo bad for now.
Test Plan:
- Removed some of my comments.
- As an administrator, removed other users' comments.
- Failed to view history of a removed comment.
- Failed to edit a removed comment.
- Failed to remove a removed comment.
- Verified feed doesn't show the old comment after comment removal.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: qgil, chad, epriestley
Maniphest Tasks: T4909
Differential Revision: https://secure.phabricator.com/D8945
Summary:
Ref T3718. Ref T3644. Ref T3092. Switches from the Releeph UI elements to standard ones. I'll attach some screenshots.
Also fixes CSRF against the request action endpoint.
Test Plan:
- Viewed request details.
- Took actions on a request from detail page.
- Viewed request list.
- Took actions on a request from list page.
- Used keyboard shortcuts to navigate list.
- Used keyboard shortcuts to take actions.
- Simulated errors.
- Viewed on devices.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: grp, FacebookPOC, mattlqx, tala, beng, LegNeato, epriestley
Maniphest Tasks: T3718, T3092, T3644
Differential Revision: https://secure.phabricator.com/D8771
Summary: ...use the prefab stuff as it does fancier things than we were doing. Only trick then really is to pass username and the map of handle phids => icons to the client so prefab can work nicely. Fixes T4775.
Test Plan: made a herald rule with projects and users. Saw nice icons. Reloaded page and still saw nice icons.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4775
Differential Revision: https://secure.phabricator.com/D8749
Summary: ...the key is to move a layer lower and beam down the updated comment. There is a wee bit of Javascript gymnastics going on here. Fixes T4608.
Test Plan: made a comment + resolve. clicked edit and made changes. noted transaction updated correctly and "history" link worked. edited again to a deletion and noted the "this is deleted" looked right and history link still worked
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin, chad
Maniphest Tasks: T4608
Differential Revision: https://secure.phabricator.com/D8702
Summary: Fixes T4641.
Test Plan: Dragged a "normal" task between "high" and "low" tasks and it stayed as "normal". Generally seems correct when playing around.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: mbishopim3, Beltran-rubo, epriestley, Korvin
Maniphest Tasks: T4641
Differential Revision: https://secure.phabricator.com/D8622
Summary: Ref T1812. Moves most specialized status handling into `ManiphestTaskStatus`. The only real missing case is reports.
Test Plan:
Browsed most of the affected interfaces. Changed task status:
{F132697}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1812
Differential Revision: https://secure.phabricator.com/D8579
Summary: Useful in cases where there is an Arcanist Project but not a repository tracked by Phabricator for a particular revision.
Test Plan: Created a new rule to flag Differential revisions with a particular Arcanist project, verified that it applied as expected via the test console to revisions with the project specified and with a different project specified.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8463
Summary: Ref T2222. Makes the "lint/unit errors" warnings work again.
Test Plan: Viewed some revisions with and without these warnings.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8475
Summary: Fixes T4553, T4407.
Test Plan: created tasks and they showed up in the proper column. edited task priority and they moved about sensically.
Reviewers: chad, epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4553, T4407
Differential Revision: https://secure.phabricator.com/D8420
Summary:
Fixes T4550 by changing supportsFeed to shouldPublishFeedStory, so things can be more granular like that are with mail. Attempts to fix things generally too, filtering out xactions that have no business in feed, etc.
Also return an updated Task HTML representation on drag and drop moves, etc. This is important so if the priority changes you can see it reflected in the UI.
Test Plan: dragged tasks around. observed no feed stories on subpriority drags. observed feed stories and updated color bars on stories that changed priority
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4550
Differential Revision: https://secure.phabricator.com/D8399
Summary: This wasn't working. Create a little JS handler and server-side support for returning the Task in the "project card" format.
Test Plan: Edited tasks from the board - they worked!
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran, chad
Differential Revision: https://secure.phabricator.com/D8392
Summary:
adds ManiphestTransaction::TYPE_PROJECT_COLUMN and makes it work. Had to clean up the Javascript ever so slightly as it was sending up the string "null" when it should just omit the data in these cases. Ref T4422.
NOTE: this is overall a bit buggy - e.g. move a task Foo from column A to top of column B; refresh; task Foo is at bottom of column B and should be at top of column B - but I plan on additional diff or three to clean this up.
Test Plan: dragged tasks around columns. clicked on those tasks and saw some nice transactions.
Reviewers: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4422
Differential Revision: https://secure.phabricator.com/D8366
Summary:
A few minor fixes:
- When we build a tag with `"meta" => null`, strip the attribute like we do for all other attributes. Previously, we would actually set the metadata to `null`. This happened with the Conpherence form.
- Just respond to the draft request with an empty (but valid) response, instead of building a dialog.
- `PhabricatorShapedRequest` is confusingly named and I should have caught this in review, but the basic shape of it is:
- You make one object.
- You call `trigger()` when stuff changes (e.g., a keystroke).
- It manages making a small number of requests (e.g., one request after the user stops typing for a moment).
- The way it was being used previously would incorrectly send a request for every keystroke.
I think I'm going to simplify `ShapedRequest` and merge it into some larger queue for T430.
Test Plan: Typed some text, no longer saw a flurry of requests. Reloaded page, still saw draft text.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran, chad
Differential Revision: https://secure.phabricator.com/D8380
Summary:
Ref T3886. Ref T418. For fields like "Summary" and "Test Plan" where changes can't be summarized in one line, allow CustomField to provide a "(Show Details)" link and render a diff.
Also consolidate some of the existing copy/paste, and simplify this featuer slightly now that we've move to dialogs.
Test Plan:
{F115918}
- Viewed "description"-style field changes in phlux, pholio, legalpad, maniphest, differential, ponder (questions), ponder (answers), and repositories.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3886, T418
Differential Revision: https://secure.phabricator.com/D8284
Summary: Fixes T3497.
Test Plan: on conpherence 1, typed some stuff. clicked conpherence 2 - observed some stuff gone. clicked conpherence 1 - stuff came back! submitted conpherence 1 and reloaded - stuff did not come back. (Generally played around a bunch like this)
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3497
Differential Revision: https://secure.phabricator.com/D8266
Summary:
Ref T2222. This restores the "N older comments are hidden." shield to all ApplicationTransactions applications. Roughly the rule this uses is that transactions older than your most recent comment are hidden, under the assumption that you've already read and dealt with them, since you replied afterward. Then we show your last comment to remind/contextualize you, and anything afterward. We also don't hide transactions if we'd only be hiding a handfull, and we never hide the few most recent transactions.
This might need some #design help.
Test Plan:
The tricky part here is the anchor rule, which deals with the case where you follow a link to `T123#4`, but that would normally be hidden. We simulate a click on "show all" if you hit an anchor which is hidden. Here's what it looks like in Maniphest:
{F112891}
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8229
Summary: form.reset() resets a form to whatever values were present when the form was loaded into the DOM. Instead, grab all the pertinent form bits and set there values to "clear". I don't think there's too much utility in putting this somewhere more general, but it could be something like DOM.clearForm(form) or something. Fixes T3629.
Test Plan:
repro'd original issue
- open legalpad doc (or your favorite application transaction powered app)
- type in a comment but do not submit (you are creating a draft) e.g. "foo"
- reload page and note comment appears e.g. "foo"
- change comment e.g. "foobar"
- submit comment
- BUG - after submission, the comment reverts to the comment at initial page load e.g. "foo"
then after this patch, I can't repro it anymore with these steps - the comment is correctly blank
NOTE: this will need to be tested with more complicated forms.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3629
Differential Revision: https://secure.phabricator.com/D8220
Summary: adds a new FIELD and a new VALUE to support this. Slightly dodgy because priorities do not have phids so we have to special case how we handle this in a few spots. Ref T4294.
Test Plan: made a new rule to get cc'd on unbreak now and wishlist tasks. verified got cc'd correctly and not cc'd correctly.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4294
Differential Revision: https://secure.phabricator.com/D8156
Summary:
Ref T3623. I'm sure I didn't get the margins / drop shadow quite right, but this looks and works reasonably well:
{F105637}
Test Plan: Clicked stuff to quick create.
Reviewers: chad, btrahan
Reviewed By: chad
CC: chad, aran
Maniphest Tasks: T3623
Differential Revision: https://secure.phabricator.com/D8089
Summary:
Added yformat to ManiphestReportController. Removed [yy] from the js.
Will pull config.yformat or send []. The old way with [yy] never seemed to worked having config.yformat, also would crash if yformat was in with value
Test Plan: Loadup burn up report, hover over a given date. Number of tasks opened should be an int
Reviewers: epriestley, #blessed_reviewers
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8080
Summary: Ref T1344. Makes requests to the server, which are received and ignored. Performs appropriate locking/unlocking/enabling/disabling on the client.
Test Plan: Dragged stuff around, saw it enable/disable/send correctly.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1344
Differential Revision: https://secure.phabricator.com/D7943
Summary:
Ref T1344. Makes the UI/UX a little nicer; still no actual backend stuff. This changes:
- When you drop an item onto a different column, the item actually moves.
- Empty columns render with a special CSS class now, but no nodes in the list. This cleans up some JS jankiness. I made the "empty" columns have a light blue background for now. We could put some sort of subtle background image in them instead, or some kind of call to action if it's not redundant with other UI.
Test Plan: {F101208}
Reviewers: chad, btrahan
Reviewed By: chad
CC: chad, aran
Maniphest Tasks: T1344
Differential Revision: https://secure.phabricator.com/D7942
Summary:
Ref T1344. Allows you to drag tasks within a column and between columns, and handles all the multi-column state / targeting / ghosting stuff.
This is a UI-only change; you can't actually do anything meaningful with these yet.
Roughly, I added the idea of a DraggableList existing within a "group" of draggable lists. Normally, that group only has one item, but on boards it has all of the columns. Then I made all of the relevant operations just apply to the whole group of lists.
Test Plan:
- Verified existing funtionality in Maniphest and ApplicationSearch is unaffected, by dragging around tasks to reprioritize them and dragging around search items.
- Dragged tasks between columns on a board view.
{F101196}
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T1344
Differential Revision: https://secure.phabricator.com/D7941
Summary: This implements support for explicitly marking the sequence of build steps. Users can now drag and re-order build steps in plans, and artifact dependencies are re-calculated so that if you move "Run Command" before "Lease Host", the "Run Command" step has it's artifact setting cleared and thus the step becomes invalid.
Test Plan: Re-ordered build steps and observed dependencies being correctly recalculated.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7715
Summary:
Ref T4122.
- For Diffusion, we need "allow null" (permits selection of "No Credential") for anonymous HTTP repositories.
- For Diffusion, we can make things a little easier to configure by prefilling the username.
Test Plan: Used UIExample form. These featuers are used in a future revision.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4122
Differential Revision: https://secure.phabricator.com/D7624
Summary: Fixes T4006.
Test Plan: clicked "select all" and dragged around tasks. Noted the task remained selected as I re-ordered, thus keeping hte count accurate. Verified when I hit "batch edit" the right tasks showed up.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4006
Differential Revision: https://secure.phabricator.com/D7566
Summary:
Depends on D7500.
This seemed like a pretty good idea once I thought of it. Instead of having some custom triggering logic instead Harbormaster, I figured it best to leverage all of Herald's power so that users can create rules to apply builds to commits and differential revisions. This gives the added advantage that they can trigger off builds for particular types of revisions and commits, which seems like it could be really useful (e.g. run extra tests against revisions that touch sensitive areas of the code).
Test Plan: Ran the usual daemons + the Harbormaster daemon. Pushed a commit to the repository and saw both the buildable and build get created when the commit worked picked it up. Submitted a diff and saw both the buildable and build get created when the Herald rules were evaluated for the diff.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran, hwinkel
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7501
Summary: Makes it easy to choose distinctive icons for projects.
Test Plan:
{F71018}
{F71020}
{F71019}
{F71021}
Reviewers: btrahan, chad
Reviewed By: chad
CC: chad, aran
Differential Revision: https://secure.phabricator.com/D7333
Summary: Ref T603. When a user selects "Custom", we pop open the rules dialog and let them create a new rule or edit the existing rule.
Test Plan: Set some objects to have custom policies.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7300
Summary: Ref T3958. Adds a provider for Mozilla's Persona auth.
Test Plan:
- Created a Persona provider.
- Registered a new account with Persona.
- Logged in with Persona.
- Linked an account with Persona.
- Dissolved an account link with Persona.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3958
Differential Revision: https://secure.phabricator.com/D7313
Summary: Currently, when a dialog is submitted the Workflow itself emits an event but no DOM event is emitted. The workflow event is fine for handlers which only use JS, but there's currently no way for a handler to act more like a normal form handler. This event gives normal form handlers a way to capture Workflow submits and muck around with form contents, etc.
Test Plan: In a future diff, edited policies via a Workflow dialog.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7295
Summary: Ref T603. Make this a little easier to use by highlighting the current value.
Test Plan: See screenshot.
Reviewers: chad, btrahan
Reviewed By: chad
CC: chad, aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7289
Summary: Ref T603. After thinking about this for a bit I can't really come up with anything better than what Facebook does, so I'm going to implement something similar for choosing custom policies. To start with, swap this over to a JS-driven dropdown.
Test Plan: See screenshot.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7285
Summary:
Ref T603. This isn't remotely usable yet, but I wanted to get any feedback before I build it out anymore.
I think this is a reasonable interface for defining custom policies? It's basically similar to Herald, although it's a bit simpler.
I imagine users will rarely interact with this, but this will service the high end of policy complexity (and allow the definition of things like "is member of LDAP group" or whatever).
Test Plan: See screenshots.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran, asherkin
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7217
Summary:
Ref T1279. Although I think this is a bad idea in general (we once supported it, removed it, and seemed better off for it) users expect it to exist and want it to be available. Give them enough rope to shoot themselves in the foot.
I will probably write some lengthy treatise on how you shouldn't use this rule later.
Implementation is straightforward because Differential previously supported this rule.
This rule can also be used to add project reviewers.
Test Plan: Made some "add reviewers" rules, created revisions, saw reviewers trigger.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7235
Summary:
- Use the box view in the test console.
- Let the test console load tasks and mocks. We should move this to the adapters (`canAdaptObject($object)` or something).
- Fix a minor issue with "Always": hiding the whole cell could make the table layout weird in Safari, at least. Just hide the select instead.
Test Plan:
- Used test console on task.
- Used test console on mock.
- Created (silly) rule with "Always" and also some other conditions.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7220
Summary:
Fixes T1461.
Adds
- FIELD_ALWAYS - now you could add this to a content type to always get notified
- FIELD_REPOSITORY_AUTOCLOSE_BRANCH - solves T1461
- CONDITION_UNCONDITIONALLY - used by these two fields to not show any value for the user to select
Test Plan: made a herald rule where diffs on autoclose branches would get flagged blue. made a diff on an autoclose branch and committed it. commit was flagged!
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T1461
Differential Revision: https://secure.phabricator.com/D7210
Summary: ...and deploy on Maniphest. Ref T1638.
Test Plan: created a herald rule to be cc'd for tasks created via web. made a task via web and another via email and was cc'd appropriately. edited the herald to be cc'd for tasks created via not web. made 2 tasks again and got cc'd appropriately
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T1638
Differential Revision: https://secure.phabricator.com/D7145
Summary:
Currently, draggable lists (in Config and ApplicationSearch, for example) don't let you drag an item into the first position.
This is because the behavior is correct in Maniphest: the first position is above an initial header, like "High Prioirty", and shouldn't be targetable.
Permit the behavior in general; forbid it in Maniphest.
Test Plan:
- Dragged elements into the first position in ApplicationSearch.
- Failed to drag elements into the first position in Maniphest.
Reviewers: garoevans, btrahan
Reviewed By: garoevans
CC: aran
Differential Revision: https://secure.phabricator.com/D7128
Summary:
Fixes T3814. Broadly, remarkup tables in inline comments did not work properly. I ran into several messes here and cleaned up some of them:
- Some of this code is doing `JX.$N('div', {}, JX.$H(response.markup))`, to turn an HTML response into a node, passing that around, and then doing junk with it. This is super old and gross.
- The slightly more modern pattern is `JX.$H(response.markup).getFragment().firstChild`, but this is kind of yuck too and not as safe as it could be.
- Introduce `JX.$H(response.markup).getNode()`, which actually expresses intent here. We have a bunch of `getFragment().firstChild` callsites which should switch to this, but I didn't clean those up yet because I don't want to test them all.
- Switch the `JX.$N('div', {}, JX.$H(response.markup))`-style callsites to `JX.$H(response.markup).getNode()`.
- `copyRows()` is too aggressive in finding `<tr />` tags. This actually causes the bug in T3814. We only want to find these tags at top level, not all tags. Don't copy `<tr />` tags which belong to some deeper table.
- Once this is fixed, there's another bug with mousing over the cells in tables in inline comments. We select the nearest `<td />`, but that's the cell in the remarkup table. Instead, select the correct `<td />`.
- At this point, these last two callsites were looking ugly. I provided `findAbove()` to clean them up.
Test Plan: Created, edited, deleted, moused over, and reloaded a revision with inline comments including remarkup tables. Used "Show more context" links.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3814
Differential Revision: https://secure.phabricator.com/D6924
Summary: Adds plain support for object lists that just look like lists
Test Plan: review UIexamples and a number of other applications
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6922
Summary: Fixes T2213
Test Plan: Updated a pholio mock description. Observed that when I first showed details there was a round trip made. Toggled show / hide noting no more trips made to server.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T2213
Differential Revision: https://secure.phabricator.com/D6801
Summary: take epriestley's feedback 'cuz its good
Test Plan: collapse, expand, use undo like a rockstar. observe proper behavior
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T2258
Differential Revision: https://secure.phabricator.com/D6748
Summary: Fixes T2258.
Test Plan: collapsed and expanded file via the dropdown - good stuff. got the "undo" element into the mix - also good stuff.
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, Korvin, aran
Maniphest Tasks: T2258
Differential Revision: https://secure.phabricator.com/D6742
Summary: Fixes T3715. Makes "visible" global instead of per-menu, so all the menus share a visible state.
Test Plan: For menus A and B, clicked "A, A", "A, B, A", "A, B, B", "A, B, B, A", etc. Couldn't figure out a way to break it.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3715
Differential Revision: https://secure.phabricator.com/D6745
Summary:
companion diff to D6729. This is the back-end stuff, plus calls the JS in D6729 for when images are removed, un-removed, uploaded, or replaced.
Fixes T3640.
Test Plan: messed around with images. hit save - new order! temporarily showed these stories and got text about re-ordering stuff.
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3640
Differential Revision: https://secure.phabricator.com/D6731
Summary:
Ref T3640. JS part only, should give you a list in `imageOrder` on the server that you can read with `$request->getStrList('imageOrder')`.
NOTE: You can't drag images into the first position; this is an existing thing that I just need to fix with DraggableList.
@chad might have some design feedback.
Test Plan: Dragged images around, things seemed to work?
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3640
Differential Revision: https://secure.phabricator.com/D6729
Summary: Fixes T3641. Probably needs some @chad love though on colors and what have you. Technique was to jam this into the existing notifications stuff as much as possible. I think its "okay" but if we were to add more stuff here (like a 3rd application) this could get a quality pass to consolidate even more code.
Test Plan: played with it in Chrome and Safari - looks reasonable
Reviewers: chad, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3641
Differential Revision: https://secure.phabricator.com/D6708
Summary: Ref T3671. Depends on D6674. Continues work in D6673, D6674 and extends it into Legalpad and Phriction. Then deletes a bunch of dead code.
Test Plan: Edited documents in Legalpad and Phriction, verified I got reasonable looking previews.
Reviewers: btrahan, Firehed
Reviewed By: btrahan
CC: aran, chad
Maniphest Tasks: T3671
Differential Revision: https://secure.phabricator.com/D6675
Summary:
Ref T3578. Ref T3671. Depends on D6673. Use `PHUIRemarkupPreviewPanel` (introduced in D6673) to provide question create/edit and answer edit previews in Ponder.
Then delete a million lines of duplicate code.
Test Plan: Edited a question; edited an answer. Saw live previews.
Reviewers: btrahan, Firehed
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3578, T3671
Differential Revision: https://secure.phabricator.com/D6674
Summary:
Ref T3671. A lot of applications have pretty ad-hoc preview code. Clean it up a bit and add Summary preview to Differential.
After ApplicationTransactions we might want to try to serialize the whole form and show a preview of all the transactions, but this seems not very useful in most cases (I'd guess that Remarkup previews are 99% of the value) and tricky to get right (e.g., adding images which don't exist yet to Pholio mocks).
I think I can add this in a few other places, too.
Test Plan:
Edited Maniphest Tasks and Differential Revisions, mashed some buttons. Verified previews rendered correctly. Grepped for removed CSS classes (no hits).
{F52907}
Reviewers: btrahan, Firehed
Reviewed By: btrahan
CC: aran, chad
Maniphest Tasks: T3671
Differential Revision: https://secure.phabricator.com/D6673
Summary:
Add the ability to select singular and multiple lines in paste to highlight.
This is related to T3627
Test Plan: Create a paste, select one or more lines.
Reviewers: epriestley, tberman
Reviewed By: epriestley
CC: aran, chad
Maniphest Tasks: T3627
Differential Revision: https://secure.phabricator.com/D6668
Summary:
...bsasically add a "view mode" and play with that throughout the stack. Differences are...
- normal mode has comments; history mode does not
- normal mode has inline comments; history mode does not
- page uris are correct with respect to either mode
...and that's about it. I played around (wasted too much time) trying to make this cuter. I think just jamming this mode in here is the easiest / cleanest thing at the end. Feel free to tell me otherwise!
This largely gets even better via T3612. However, this fixes T3572.
Test Plan: played around with a mock with some history. noted correct uris on images. noted no errors in js console.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3572
Differential Revision: https://secure.phabricator.com/D6638
Summary:
Ref T3373. The submit listener doesn't properly scope the form it listens to right now, so several forms on the page mean that comments post to one of them more or less at random.
Scope it properly by telling it which object PHID it is associated with.
Test Plan: Made Question comments, saw comments Ajax in on the question itself rather than on an arbitrary answer.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3373
Differential Revision: https://secure.phabricator.com/D6611
Summary:
Ref T3373. This is still pretty messy:
- The JS bugs out a bit with multiple primary object PHIDs on a single page. I'll fix this in a followup.
- The comment form itself is enormous, I'll restore some show/hide stuff in a followup.
Test Plan: Added answer comments in Ponder.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3373
Differential Revision: https://secure.phabricator.com/D6608
Summary:
Now you can actually replace an image! Ref T3572. This ended up needing a wee bit of infrastructure to work...
- add replace image transaction to pholio
- add replacesImagePHID to PholioImage
- tweaks to editor to properly update images with respect to replacement
- add edges to track replacement
- expose getNodes on graph query infrastructure to query the entire graph of who replaced who
- move pholio image to new phid infrastructure
Still TODO - the history view should get chopped out a bit from the current view - no more inline comments / generally less functionality plus maybe a tweak or two to make this more sensical.
Test Plan: replaced images and played with history controller a little. works okay.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3572
Differential Revision: https://secure.phabricator.com/D6560
Summary: Far from perfect, but better?
Test Plan:
{F51153}
{F51154}
{F51155}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6533
Summary:
Ref T3572. Needs some CSS tweaks, but this lets you drag an image on top of another image to replace it. There's no server-side or transaction support (and I'm not planning to build that), I just wanted to clear the way on the JS side.
You'll get an additional array posted called `replaces`. Keys are old file PHIDs; values are new file PHIDs.
Note that a key may not exist yet (if a user adds an image, and then also replaces that same image). In this case, the server should just treat it as an add.
Test Plan: Dragged images on top of other images.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3572
Differential Revision: https://secure.phabricator.com/D6499
Summary: Ref T2637. Allows you to "undo" if you delete an image from a mock by accident.
Test Plan:
Deleted; undo'd.
{F50878}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2637
Differential Revision: https://secure.phabricator.com/D6498
Summary: Fixes T3553. Did it by adding some code that refreshes the File object on keyup events within a given file entry. also fixes an html derp I found trying to fix this.
Test Plan: added cool things like 'bbb' to every field and noted they were maintained when I added more files
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, Korvin, chad
Maniphest Tasks: T3553
Differential Revision: https://secure.phabricator.com/D6488
Summary: Fixes T3544. Depends on D6475. This was just a missing dependency combined with some questionable error handling which I'll maybe fix some day.
Test Plan: Loaded page, saw result.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3544
Differential Revision: https://secure.phabricator.com/D6476
Summary: When user changed his mind for voting, counting does not work properly. If user vote up first and vote down, vote count must be decreased 2. fixed in javascript file
Test Plan:
http://cihad.phabricator.pompa.la/Q11
user : demo
pass : demodemo
Reviewers: aran, Korvin, epriestley
Reviewed By: epriestley
CC: simsekburak
Differential Revision: https://secure.phabricator.com/D6446
Summary: Fixes T2652. Did some testing post T2691 to see if I could close this and noted an error in the JS if you view the page not logged in. This fixes that error. Everything else I can think of seems to work...? :D
Test Plan: played around with a mock not logged in and got sensible interactions and no errors
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2652
Differential Revision: https://secure.phabricator.com/D6438
Summary: Fixes T3509. Generally tried to make the code more consistently use get_image_scale, as well as make get_image_scale aware that sometimes images need to be scaled because they're too tall (as well as too wide).
Test Plan: used the file from T3509. noted comment box appearing correctly as clicked, or clicked and dragged. submitted some comments and noted the reticles moved / scaled correctly as I resized the browser window
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3509
Differential Revision: https://secure.phabricator.com/D6418
