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

1018 commits

Author SHA1 Message Date
epriestley
b2ce0844b6 When a user clicks "Cancel" on an inline comment to leave the "Editing" state, save the state change
Summary:
Ref T13513. Now that the "currently being edited" state of inlines is saved on the server side, clear the flag when the user clicks "Cancel" to leave the "editing" state on the client.

This also serves to delete empty comments.

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

Maniphest Tasks: T13513

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

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

---

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

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

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

To simplify this:

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

---

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

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

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

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

---

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

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

---

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

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

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

---

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

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

Weirdness / known issues:

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

Subscribers: jmeador

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21186
2020-05-04 13:10:30 -07:00
epriestley
d2572f8b33 Refine more Differential review state behaviors
Summary:
Ref T13516.

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

Test Plan: Fiddled with all these behaviors.

Maniphest Tasks: T13516

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

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

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

Maniphest Tasks: T13516

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

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

Test Plan: {F7375468}

Maniphest Tasks: T13455

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

Test Plan: {F7375327}

Maniphest Tasks: T13516

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

Test Plan: {F7375145}

Maniphest Tasks: T13516

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

  - a/
    - b/
      - c.txt

...render:

  - a/b/
    - c.txt

Test Plan: {F7374205}

Maniphest Tasks: T13516

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

Restores the inline tips in the path tree.

Test Plan: {F7374175}

Maniphest Tasks: T13516

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

Test Plan: {F7374096}

Maniphest Tasks: T13516

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

Test Plan: {F7373967}

Maniphest Tasks: T13516

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

Test Plan: {F7373838}

Maniphest Tasks: T13516

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

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

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

Maniphest Tasks: T13515

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

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

Some later change removed this background.

Put the background back and separate the keystrokes into groups.

Test Plan: {F7370615}

Maniphest Tasks: T13515

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

Some complexity here arises from these concerns:

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

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

Then, some bookkeeping issues:

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

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

Maniphest Tasks: T13455

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

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

Maniphest Tasks: T13515

Differential Revision: https://secure.phabricator.com/D21135
2020-04-17 10:06:46 -07:00
epriestley
e1da1d86d6 Trim and URI encode symbol names before building URIs from them
Summary:
Fixes T13437. This URI construction was just missing URI encoding.

Also, trim the symbol because my test case ended up catching "#define\n" as symbol text.

Test Plan:
  - Configured a repository to have PHP symbols.
  - Touched a ".php" file with "#define" in it.
  - Diffed the change.
  - Command-clicked "#define" in the UI, in Safari/MacOS, to jump to the definition.
    - Before: taken to a nonsense page where "#define" became an anchor.
    - After: taken to symbol search for "#define".

Maniphest Tasks: T13437

Differential Revision: https://secure.phabricator.com/D20876
2019-10-29 09:48:42 -07:00
epriestley
2c06815edb When rendering Jupyter notebook diffs, split code inputs into individual blocks
Summary:
Ref T13425. Currently, code inputs and all outputs are grouped into a single block. This is fine for display notebooks but not great for diffing notebooks.

Instead, split source code input into individual lines with one line per block, and each output into its own block.

This allows you to leave actual line-by-line inlines on source, and comment on outputs individually.

Test Plan: {F6888583}

Maniphest Tasks: T13425

Differential Revision: https://secure.phabricator.com/D20840
2019-09-25 21:05:18 -07:00
epriestley
932d829af3 Improve behavior of inline comment highlight reticle for block diffs
Summary:
Depends on D20834. Ref T13425. After the change from "th" to "td" for accessibility, the algorithm picks which cells it should highlight slightly improperly (it picks too many cells since it can no longer find the line numbers).

Ideally, it would probably highlight //only// the source content, but there isn't an easy way to do this right now. Settle for an incremental improvement for the moment.

Test Plan: Hovered over line numbers, saw a more accurate highlight area.

Maniphest Tasks: T13425

Differential Revision: https://secure.phabricator.com/D20835
2019-09-25 16:39:18 -07:00
epriestley
7ae711ed3e Add a "View as..." option to diff dropdowns for selecting between document engines
Summary:
Depends on D20831. Ref T13425. As an escape hatch to get out of future DocumentEngine rendering behavior, provide a "View As.." option.

Now I can break DocumentEngine real bad and no one can complain.

Test Plan: Used "View As" to swap document engines for image files.

Maniphest Tasks: T13425

Differential Revision: https://secure.phabricator.com/D20832
2019-09-25 16:29:21 -07:00
epriestley
d4ed5d0428 Make various UX improvements to charts so they're closer to making visual sense
Summary: Ref T13279. Fix some tabular stuff, draw areas better, make the "compose()" API more consistent, unfatal the demo chart, unfatal the project burndown, make the project chart do something roughly physical.

Test Plan: Looked at charts, saw fewer obvious horrors.

Subscribers: yelirekim

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20817
2019-09-17 09:43:21 -07:00
epriestley
f529abf900 In stacked area charts, group nearby points so they don't overlap
Summary: Ref T13279. We currently draw a point on the chart for each datapoint, but this leads to many overlapping circles. Instead, aggregate the raw points into display points ("events") at the end.

Test Plan: Viewed a stacked area chart with many points, saw a more palatable number of drawn dots.

Subscribers: yelirekim

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20814
2019-09-17 09:26:54 -07:00
epriestley
d60d4e6a05 Don't present users with Herald fields/actions for uninstalled applications, unless the rule already uses them
Summary:
Fixes T7961. Currently, we present Herald users with actions like "Require legalpad signatures" and "Run build plans" even if Legalpad and Harbormaster are not installed.

Instead, allow fields and actions to be made "unavailable", which means that we won't present them as options when adding to new or existing rules.

If you edit a rule which already uses one of these fields or actions, it isn't affected.

Test Plan:
  - Created a rule with a legalpad action, uninstalled legalpad, edited the rule. Action remained untouched.
  - Created a new rule, wasn't offered the legalpad action.
  - Reinstalled the application, saw the action again.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T7961

Differential Revision: https://secure.phabricator.com/D20808
2019-09-12 14:33:28 -07:00
epriestley
7d41535010 When a task card is edited, emit update events for old boards and parent boards
Summary:
Ref T4900. When a card is edited, we currently emit an update notification for all the projects the task is tagged with. This isn't quite the right set:

  - We want to emit notifications for projects the task //was previously// tagged with, so it can be removed from boards it should no longer be part of.
  - We want to emit notifications for ancestors of projects the task is or was tagged with, so parent project boards can be updated.
  - However, we don't need to emit notifications for projects that don't actually have workboards.

Adjust the notification set to align better to these rules.

Test Plan:
  - Removal of Parent Project: Edited a task on board "A > B", removing the "B" project tag. Saw board A update in another window.
  - Normal Update: Edited a task title on board X, saw board X update in another window.
  - Used `bin/aphlict debug` to inspect the notification set, saw generally sensible-seeming data going over the wire.

Reviewers: amckinley

Maniphest Tasks: T4900

Differential Revision: https://secure.phabricator.com/D20680
2019-07-30 13:16:33 -07:00
epriestley
17caecdda3 Make workboard real-time updates mostly work
Summary:
Depends on D20654. Ref T4900. When a task is edited, emit a "workboards" event for all boards it appears on (in a future change, this should also include all boards it //previously// appeared on, and all parents of both sets of boards -- but I'm just getting things working for now).

When we receive a "workboards" event, check if the visible board should be updated.

Aphlict has a complicated intra-window leader/follower election system which could let us process this update event exactly once no matter how many windows a user has open with the same workboard. I'm not trying to do any of this since it seems fairly rare. It makes sense for events like "you have new notifications" where we don't want to generate 100 Ajax calls if the user has 100 windows open, but very few users seem likely to have 100 copies of the same workboard open.

Test Plan:
  - Ran `bin/aphlict debug`.
  - Opened workboard A in two windows, X and Y.
  - Edited and moved tasks in window X.
  - Saw "workboards" messages in the Aphlict log.
  - Saw window Y update in nearly-real-time (locally, this is fast enough that it feels instantaneous).

Then:

  - Stopped the Aphlcit server.
  - Edited a task.
  - Started the Aphlict server.
  - Saw window Y update after a few moments (i.e., update in response to a reconnect).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T4900

Differential Revision: https://secure.phabricator.com/D20656
2019-07-18 10:00:17 -07:00
epriestley
d02beaf816 Make reloading workboards with "R" respect workboard ordering
Summary:
Depends on D20653. Ref T4900. Pass ordering details to the reload endpoint so it can give the client accurate ordering/header information in the response.

The removed comment mentions this, but here's why this is a difficult mess:

  - In window A, view a board with "Group by: Owner" and no tasks owned by "Alice". Since "Alice" owns no tasks, this means the columns do not have an "Assigned to: Alice" header!
  - In window B, edit task T and assign it to Alice.
  - In window A, press "R".

Window A now not only needs to update to properly reflect the state of task T, it actually needs to draw a new "Assigned to: Alice" header in every column.

Fortunately, the "group by" code anticipates this being a big mess, is fairly careful about handling it, and the client can handle this state change and the actual code change here isn't too involved. This is just causing a lot of not-very-obvious indirect effects in the pipeline to handle these situations that need complex redraws.

Test Plan:
  - After making various normal edits/creates/moves in window A, pressed "R" in window B. Saw ordering reflected correctly after sync.
  - Went through the whole "Group by: Owner" + assign to unrepresented owner flow above. After pressing "R", saw "Assigned to: Alice" appear on the board.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T4900

Differential Revision: https://secure.phabricator.com/D20654
2019-07-17 13:17:00 -07:00
epriestley
8669c3c0d2 When updating a workboard with "R", send the client visible set with version numbers
Summary:
Depends on D20652. Ref T4900. When the user presses "R", send a list of cards currently visible on the client and their version numbers.

On the server:

  - Compare the client verisons to the server versions so we can skip updates for objects which have not changed. (For now, the client version is always "1" and the server version is always "2", so this doesn't do anything meaningful, and every card is always updated.)
  - Compare the client visible set to the server visible set and "remove" any cards which have been removed from the board.

I believe this means that "R" always puts the board into the right state (except for some issues with client orderings not being fully handled yet). It's not tremendously efficient, but we can make versioning better (using the largest object transaction ID) to improve that and loading the page in the first place doesn't take all that long so even sending down the full visible set shouldn't be a huge problem.

Test Plan:
  - In window A, removed a card from a board.
  - In window B, pressed "R" and saw the removal reflected on the client.
  - (Also added cards, edited cards, etc., and didn't catch anything exploding.)

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T4900

Differential Revision: https://secure.phabricator.com/D20653
2019-07-17 13:16:03 -07:00
epriestley
1ee6ecf397 Move "BoardResponseEngine" toward a more comprehensive update model
Summary:
Depends on D20639. Ref T4900. Currently, "BoardResponseEngine" has a `setObjectPHID()` method. This is called after edit operations to mean "we just edited object X, so we know it needs to be updated".

Move toward `setUpdatePHIDs(...)` in all cases, with `setUpdatePHIDs(array(the-object-we-just-edited))` as a special case of that. After this change, callers pass:

  - An optional list of PHIDs they know need to be updated on the client. Today, this is always be a card we just edited (on edit/move flows), or a sort of made-up list of PHIDs for the moment (when you press "R"). In the future, the "R" endpoint will do a better job of figuring out a more realistic update set.
  - An optional list of PHIDs currently visible on the client. This is used to update ordering details and mark cards for removal. This is currently passed by edit/move, but not by pressing "R" (it will be in the future).
  - An optional list of objects. The "R" workflow has to load these anyway, so we can save a couple queries by letting callers pass them. For now, the edit/move flows still rely on the engine to figure out what it needs to load.

This does very little to actually change client behavior, it mostly just paves the way for the next update to the "R" workflow to make it handle add/remove cases properly.

Test Plan:
  - Edited and moved cards on a workboard.
  - Pressed "R" to reload a workboard.

Neither of these operations seem any worse off than they were before. They still don't fully work:

  - When you edit a card and delete the current workboard project from it, it remains visible. This is also the behavior on `master`. This is sort of intentional since we don't necessarily want to make these cards suddenly disappear? Ideally, we would probably have some kind of "tombstone" state where the card can still be edited but can't be dragged, and the next explicit user interaction would clean up old tombstones. This interaction is very rare and I don't think it's particularly important to specialize.
  - When a card is removed from the board, "R" can't currently figure out that it should be removed from the client. This is because the client does not yet pass a "visiblePHIDs" state. It will in an upcoming change.
  - The "R" flow always sends a full set of card updates, and can not yet detect that some cards have not changed.
  - There's a TODO, but some ordering stuff isn't handled yet.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T4900

Differential Revision: https://secure.phabricator.com/D20652
2019-07-17 13:13:15 -07:00
epriestley
db69686927 Make pressing "R" on your keyboard reload the card state on workboards
Summary:
Depends on D20638. Ref T4900. This is an incremental step toward proper workboard updates.

Currently, the client can mostly update its view because we do updates when you edit or move a card, and the client and server know how to send lists of card updates, so a lot of the work is already done.

However, the code assumes we're only updating/redrawing one card at a time. Make the client accept and process multiple card updates.

In future changes, I'll add versioning (so we only update cards that have actually changed), fix the "TODO" around ordering, and move toward actual Aphlict-based real-time updates.

Test Plan:
  - Opened the same workboard in two windows.
  - Edited cards in one window, pressed "R" (capital letter, with no modifier keys) to reload the second window.
  - Saw edits and moves reflected accurately after sync, except for some special cases of header/order interaction (see "TODO").

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T4900

Differential Revision: https://secure.phabricator.com/D20639
2019-07-17 13:11:26 -07:00
epriestley
fc795994f3 Remove obsolete "options" from workboard "updateCard()" call
Summary: Depends on D20637. Ref T4900. This is some ancient dead code that nothing uses.

Test Plan: Grepped for `updateCard()` to verify it's private. Searched for "options" and "dirtyColumn" and got no hits.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T4900

Differential Revision: https://secure.phabricator.com/D20638
2019-07-03 10:06:13 -07:00
epriestley
a80426b339 Provide chart function labels over the wire instead of making them up
Summary: Ref T13279. Makes charts incrementally more useful by allowing the server to provide labels and colors for functions.

Test Plan: {F6438872}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20501
2019-05-22 05:22:59 -07:00
epriestley
c6052b41a6 Label important data on charts
Summary:
Ref T13279. Adds client-side support for rendering function labels on charts, then labels every function as important data.

Works okay on mobile, although I'm not planning to target mobile terribly heavily for v0.

Test Plan: {F6438860}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20500
2019-05-22 05:21:26 -07:00
epriestley
81456db559 Roughly support stacked area charts
Summary:
Ref T13279. This adds support for:

  - Datasets can have types, like "stacked area".
  - Datasets can have multiple functions.
  - Charts can store dataset types and datasets with multiple functions.
  - Adds a "stacked area" dataset.
  - Makes D3 actually draw a stacked area chart.

Lots of rough edges here still, but the result looks slightly more like it's supposed to look.

D3 can do some of this logic itself, like adding up the area stacks on top of one another with `d3.stack()`. I'm doing it in PHP instead because I think it's a bit easier to debug, and it gives us more options for things like caching or "export to CSV" or "export to API" or rendering a data table under the chart or whatever.

Test Plan: {F6427780}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20498
2019-05-22 05:19:41 -07:00
epriestley
146317f2c4 Remove the legacy chart behavior from Maniphest
Summary: Depends on D20486. Ref T13279. Now that the "Reports" UI uses a panel to draw a real chart from Facts, throw away the copy of the old code.

Test Plan: `grep`

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20487
2019-05-22 04:48:24 -07:00
epriestley
e1076528ef Copy the "line-chart" behavior to "line-chart-legacy" to keep "Maniphest > Reports" working
Summary:
Ref T13279. Charting changes alter how the "line-chart" behavior works, but the "Burnup Chart" still relies on the old behavior.

Although I'm intending to remove "Maniphest > Reports" once Facts is a minimally sufficient replacement, copy this behavior to keep it working until we're ready to pull the trigger.

Also fix a leftover typo from D20435.

Test Plan: Viewed a legacy Maniphest burnup rate report.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20449
2019-04-19 07:05:37 -07:00
epriestley
45b3c23148 Fetch chart data via async request and redraw charts when the window is resized
Summary:
Depends on D20439. Ref T13279. Some day, charts will probably need to reload themselves or do a bunch of defer/request-shaping magic when they're on a dashboard with 900 other charts.

Give the controller separate "HTML placeholder" and "actual data" modes, and make the placeholder fetch the data in a separate request.

Then, make the chart redraw if you resize the window instead of staying at whatever size it started as.

Test Plan:
  - Loaded a chart, saw it load data asynchronously.
  - Resized the window, saw the chart resize.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20440
2019-04-18 07:10:05 -07:00
epriestley
044c6fbc19 Support drawing multiple functions on the same chart
Summary: Depends on D20438. Ref T13279. Widgets produced vs widgets sold, etc.

Test Plan: {F6381609}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20439
2019-04-18 07:07:33 -07:00
epriestley
02981f0add Fix negative chart values, add storage for charts
Summary:
Ref T13279. I think I'm going to fling some stuff at the wall for a bit here and hope most of it sticks, so this series of changes may not be terribly cohesive or focused. Here:

The range of the chart is locked to "[0, 105% of max]". This is trying to make a pleasing extra margin above the maximum value, but currently just breaks charts with negative values. Later:

    - I'll probably let users customize this.
    - We should likely select 0 as the automatic minimum for charts with no negative values.
    - For charts with positive values, it would be nice to automatically pick a pleasantly round number (25, 100, 1000) as a maximum by default.

We don't have any storage for charts yet. Add some. This works like queries, where every possible configuration gets a short URL slug. Nothing writes or reads this yet.

Rename `fn()` to `css_function()`. This builds CSS functions for D3. The JS is likely to get substantial structural rewrites later on, `fn()` was just particularly offensive.

Test Plan: Viewed a fact series with negative values. Ran `bin/storage upgrade`.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20438
2019-04-18 06:59:41 -07:00
epriestley
9532bfbb32 Improve trigger editor behavior when switching to/from tokenizers
Summary:
Ref T13269. See D20329. When we switch trigger rule control types, reset the rule value.

Also, pick slightly nicer defaults for status/priority.

Test Plan:
  - Created a "Change Status To: X" rule.
  - Saved it.
  - Edited it.
  - Selected "Assign to" for the existing action's dropdown.
  - Before: tokenizer filled with nonsense.
  - After: tokenizer cleared.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13269

Differential Revision: https://secure.phabricator.com/D20416
2019-04-17 12:24:23 -07:00
epriestley
b8551bb5f9 Reduce drag-and-drop jank on dashboards
Summary:
Depends on D20414. Ref T13272. Several minor things here:

  - Currently, you can drag panels underneath the invisible "there are no items in this column" div and the "Create Panel / Add Existing Panel" buttons. This is silly; stop it.
  - Currently, when viewing a tab panel on a dashboard, you can drag the panels inside it. This is extremely silly. Make "movable" off by default and pass it through the async flow only when we actually need it.
  - Make the whole "Add Tab..." virtual tab clickable to open the dropdown. This removes the rare exception/todo combo I added earlier. {key F}
  - Add or remove some icons or something.

Test Plan: Moved panels around on dashboards. Tried to drag panels inside tab panels. Added tab. Things were less obviously broken.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13272

Differential Revision: https://secure.phabricator.com/D20415
2019-04-17 12:20:44 -07:00
epriestley
fb994909cf Make "Move Panel" on dashboards use the new storage and transactions
Summary: Depends on D20408. Ref T13272. The actual JS is still a little bit iffy, but this makes the server side "move" operation work correctly by updating it to use the same code as everything else.

Test Plan: Moved panels around on single-column and multi-column dashboards, saw them move to reasonable places and stay there when I reloaded the page.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13272

Differential Revision: https://secure.phabricator.com/D20409
2019-04-14 10:25:22 -07:00
epriestley
82c46f4b93 Update "add panel" and "remove panel" Dashboard flows to the new panel storage format
Summary:
Depends on D20407. Ref T13272. This updates the "add panel" (which has two flavors: "add existing" and "create new") and "remove panel" flows to work with the new duplicate-friendly storage format.

  - We now modify panels by "panelKey", not by panel PHID, so one dashboard may have multiple copies of the same panel and we can still figure out what's going on.
  - We now work with "contextPHID", not "dashboardID", to make some flows with tab panels (or other nested panels in the future) easier.

The only major remaining flow is the Javascript "move panels around with drag-and-drop" flow.

Test Plan:
  - Added panels to a dashboard with "Create New Panel".
  - Added panels to a dashboard with "Add Existing Panel".
  - Removed panels from a dashboard.
  - Added and removed duplicate panels, got a correctly-functioning dashboard that didn't care about duplicates.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13272

Differential Revision: https://secure.phabricator.com/D20408
2019-04-14 10:24:58 -07:00
epriestley
cbe13b3065 When editing a tab panel from a dashboard, redirect back to the dashboard
Summary:
Depends on D20396. Ref T13272. Currently, using the dropdowns to edit a tab panel from a dashboard redirects you to the tab panel page.

Instead, redirect back to the context page (usually, a dashboard -- but theoretically a containing tab panel, since you can put tab panels inside tab panels).

Also, fix some JS issues with non-integer panel keys. I've moved panel keys from "0, 1, 2, ..." to having arbitrary keys to make some operations less flimsy/error-prone, but this needs some JS tweaks.

Test Plan: Edited a tab panel from a dashboard, got sent sensibly back to the dashboard.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13272

Differential Revision: https://secure.phabricator.com/D20397
2019-04-12 06:13:12 -07:00
epriestley
9ad9ac9be6 On Dashboard tab panels in edit mode, make the "Tab Name" and the "Dropdown Edit Caret" into different links
Summary:
Ref T13272. In edit mode, tab panels now have a dropdown menu. However, this sort of overrlaps with the actual action of clicking the tab to select it.

Separate these into different click targets so that "select tab X" and "open dropdown menu for X" are different operations.

This is more work than it appears because:

  - We have an "action icon" already, used when you put a dashboard on a portal/home to create an "Edit" link. It makes sense to attach dropdowns to this, but it has some hard-coded stuff.
  - In applications with a "Create <thing>" in the crumbs (like Maniphest), we may use a dropdown menu if there are multiple create forms available. However, this menu renders in a weird way by reading all the properties out of an actual "View" object and building something else.
  - The "list of tabs" stuff shares code with different "list of tabs" navigation used by Diffusion and Instances.

..but I think I fixed everything and didn't break anything.

Test Plan:
  - Clicked "select tab" and "open dropdown menu" as separate actions.
  - Viewed Diffusion, Maniphest with multiple create forms, Instances.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13272

Differential Revision: https://secure.phabricator.com/D20396
2019-04-12 06:08:32 -07:00
epriestley
cec779cdab When drawing a very wide graph line diagram, smush it together a bit
Summary: Depends on D20345. Use a narrower layout for very large graphs to save some space.

Test Plan:
Before:

{F6320215}

After:

{F6320216}

This does not affect smaller graphs.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20346
2019-03-28 21:18:52 -07:00
epriestley
f6658bf391 When changing the trigger type in the trigger editor, properly redraw the control
Summary: Ref T13269. I refactored this late in the game to organize things better and add table cells around stuff, and accidentally broke the relationship between the "Rule Type" selector and the value selector.

Test Plan: Switched rule type selector from "Change Status" to "Play Sound", saw secondary control update properly.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13269

Differential Revision: https://secure.phabricator.com/D20326
2019-03-26 11:50:31 -07:00
epriestley
71c89bd057 Pass all adjacent card PHIDs from the client to the server when moving a card
Summary:
Depends on D20321. Fixes T12175. Ref T13074. Now that before/after PHIDs are suggestions, we can give the server a more complete view of what the client is trying to do so we're more likely to get a good outcome if the client view is out of date.

Instead of passing only the one directly adjacent card PHID, pass all the card PHIDs that the client thinks are in the same group.

(For gigantic columns with tens of thousands of tasks this might need some tweaking -- like, slice both lists down to 10 items -- but we can cross that bridge when we come to it.)

Test Plan:
  - Dragged some cards around to top/bottom/middle positions, saw good positioning in all cases.
  - In two windows, dragged stuff around on the same board. At least at first glance, conflicting simultaneous edits seemed to do reasonable things.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13074, T12175

Differential Revision: https://secure.phabricator.com/D20322
2019-03-26 07:46:05 -07:00
epriestley
6138e50962 When moving cards on workboards, treat before/after cards as optional hints, not strict requirements
Summary:
Depends on D20320. Ref T12175. Ref T13074. Currently, when you move a card between columns, the internal transaction takes exactly one `afterPHID` or `beforePHID` and moves the card before or after the specified card.

This is a fairly strict interpretation and causes a number of practical issues, mostly because the user/client view of the board may be out of date and the card they're dragging before or after may no longer exist: another user might have moved or hidden it between the last client update and the current time.

In T13074, we also run into a more subtle issue where a card that incorrectly appears in multiple columns fatals when dropped before or after itself.

In all cases, a better behavior is just to complete the move and accept that the position may not end up exactly like the user specified. We could prompt the user instead:

> You tried to drop this card after card X, but that card has moved since you last loaded the board. Reload the board and try again.

...but this is pretty hostile and probably rarely/never what the user wants.

Instead, accept a list of before/after PHIDs and just try them until we find one that works, or accept a default position if none work. In essentially all cases, this means that the move "just works" like users expect it to instead of fataling in a confusing/disruptive/undesirable (but "technically correct") way.

(A followup will make the client JS send more beforePHIDs/afterPHIDs so this works more often.)

We could eventually add a "strict" mode in the API or something if there's some bot/API use case for precise behavior here, but I suspect none exist today or are (ever?) likely to exist in the future.

Test Plan:
  - (T13074) Inserted two conflicting rows to put a card on two columns on the same board. Dropped one version of it underneath the other version. Before: confusing fatal. After: cards merge sensibly into one consistent card.
  - (T12175) Opened two views of a board. Moved card A to a different column on the first view. On the second view, dropped card B under card A (still showing in the old column). Before: confusing fatal. After: card ended up in the right column in approximately the right place, very reasonably.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13074, T12175

Differential Revision: https://secure.phabricator.com/D20321
2019-03-26 07:45:24 -07:00
epriestley
686b03a1d5 Dim the action drop preview element when the cursor approaches
Summary:
Depends on D20308. Ref T5474. The element which previews what will happen when you drop a task somewhere can cover the bottom part of the rightmost column on a workboard.

To fix this, I'm trying to just fade it out if you put your cursor over it. I tried to do this in a simple way previously (":hover" + "opacity: 0.25") but it doesn't actually work because "pointer-events: none" stops ":hover" from working.

Instead, do this in Javascript. This is a little more complicated but: it works; and we can do the fade when you get //near// the element instead of actually over it, which feels a little better.

Test Plan:
  - Shrank window to fairly small size so that the preview could cover up stuff on the workboard.
  - Dragged a card toward the rightmost column.
  - Before: drop action preview covered some workboard stuff.
  - After: preview faded out as my cursor approached.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T5474

Differential Revision: https://secure.phabricator.com/D20320
2019-03-25 14:35:55 -07:00
epriestley
bfa5ffe8a1 Add a "Play Sound" workboard trigger rule
Summary:
Ref T5474. Allow columns to play a sound when tasks are dropped.

This is a little tricky because Safari has changed somewhat recently to require some gymnastics to play sounds when the user didn't explicitly click something. Preloading the sound on the first mouse interaction, then playing and immediately pausing it seems to work, though.

Test Plan: Added a trigger with 5 sounds. In Safari, Chrome, and Firefox, dropped a card into the column. In all browsers, heard a nice sequence of 5 sounds played one after the other.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T5474

Differential Revision: https://secure.phabricator.com/D20306
2019-03-25 14:03:57 -07:00