Summary: Ref T13513. Currently, if you're editing a comment, "delete" doesn't put the comment into the correct state. This action is normally only reachable from comment previews, since an editing inline has no "delete" button.
Test Plan:
- Started editing an inline, clicked "Delete", got a deletion.
- Created an inline, typed text,
- Deleted a normal comment via preview.
- Deleted a normal comment via the on-inline action.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21238
Summary:
Ref T13513. Currently, clicking "View" from the inline comment preview (below the "add comment" area at the bottom of the page) only works if the inline isn't being edited.
Update this behavior so it works on inlines in either "Viewing" or "Editing" states.
Test Plan:
- Clicked "View" on a normal inline, got jumped/selected.
- Clicked "View" on an editing inline, got jumped/selected.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21237
Summary:
Ref T13513. Currently:
- If you click the "Show Changeset" button, your state change doesn't actually get saved on the server.
- It's hard to select a changeset path name for copy/paste because the "highlight the header" code tends to eat the event.
Instead: persist the former event; make the actual path text not be part of the highlight hitbox.
Test Plan:
- Clicked "Show Changeset", reloaded, saw changeset visibility persisted.
- Selected changeset path text without issues.
- Clicked non-text header area to select/deselect changesets.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21236
Summary:
Ref T13513. Currently, inline storage objects ("TransactionComment") can't directly generate a runtime object ("InlineComment").
Allow this transformation to be performed in a genric way so clunky code which does it per-object-type can be removed, lifted, or simplified.
Simplify an especially gross callsite in preview code.
Test Plan: Previewed inline comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21226
Summary: See D21213. If there's no matching element, `findAbove()` throws. Handle these cases correctly.
Test Plan: Visited `#toc` on a revision, no longer saw a JS error.
Differential Revision: https://secure.phabricator.com/D21222
Summary:
Ref T13513. Overloading "original text" to get "edit-on-load" comments into the right state has some undesirable side effects.
Instead, provide the text when the editor opens. This fixes a cancel interaction.
Test Plan:
- Create an inline, type text, don't save.
- Reload page.
- Cancel.
- Before: cancelled into empty state.
- After: cancelled into deleted+undo state.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21219
Summary:
Ref T13513. This is a bit clumsy, but the cleanest way to implement "isEditing" inlines today is to send them down as normal inlines and then simulate clicking "edit" on them.
When we do, don't focus the resulting editor: focusing it makes the page scroll around and highlight things in essentially random order as the editors load in.
Test Plan: Reloaded a page with some open editors, wasn't scrolled to them.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21217
Summary:
Ref T13513. As users type text into inline comments, save the comment state as a draft on the server.
This has some rough edges, particularly around previews, but mostly works. See T13513 for notes.
Test Plan: Started an inline, typed some text, waited a second, reloaded the page, saw an editing inline with the saved text.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21216
Summary: Ref T13513. Refine some inline behaviors, see test plan.
Test Plan:
- Edit a comment ("A"), type text ("AB"), cancel, edit.
- Old behavior: edit and undo states (wrong, and undo does not function).
- New behavior: edit state only.
- Edit a comment ("A"), type text ("AB"), cancel. Undo ("AB"), cancel. Edit.
- Old behavior: "AB" (wrong: you never submitted this text).
- New behavior: "A".
- Create a comment, type text, cancel.
- Old behavior: counter appears in filetree (wrong, comment is undo-able but should not be counted).
- New behavior: no counter.
- Cancel editing an empty comment with no text.
- Old behavior: Something buggy -- undo, I think?
- New behavior: it just vanishes (correct behavior).
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21212
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
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
Summary:
At some point, the highlighting behavior for the timeline broke. When you follow a link to a particular timeline story, the story should be highlighted.
Prior to this change, the `<a />` tag itself highlights, but there's no associated CSS and it's too deep in the tree to do anything useful.
(Since this change is fairly straightforward, I gave up digging for the root cause before finding it.)
Test Plan:
- Clicked a timeline story anchor, saw the story highlight.
Differential Revision: https://secure.phabricator.com/D21213
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
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
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
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
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
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
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
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
Summary:
Ref T13516. Currently, the "File Tree" element is a semi-dynamic side panel that's implemented as a special mode of a side nav panel.
This implementation is fairly clunky, and arose from organic growth out of the side nav. As such, it has some weird behaviors, doesn't have builtin support for show/hide, and can't generalize easily.
Introduce a "FormationView" which supports loading a page up with piles of side panels in various modes.
Test Plan: No callers and no user-visible impact.
Maniphest Tasks: T13516
Differential Revision: https://secure.phabricator.com/D21150
Summary:
Ref T13515. This restores the "Open in Editor" behavior to Diffusion, and makes "\" work there.
The URI pattern is now sent as a structured template to the client, so the code will work properly if a file path contains "%l".
Test Plan:
- Clicked "Open in Editor" and pressed "\" in Diffusion when viewing a file.
- Clicked a line, hit "\", got the file opened to that line.
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21149
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
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
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
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
Summary:
Fixes T13452. We currently give users mixed signals about the interaction mode of this text: the cursor says "text" but the behavior is "grab".
Make the behavior "text" to align with the cursor. An alternate variation of this change is to remove the cursor, but this is preferable if it doesn't cause problems, since copying the task ID is at least somewhat useful.
Test Plan: In Safari, Firefox, and Chrome: selected and copied object names from workboard cards; and dragged workboard cards by other parts of their UI.
Maniphest Tasks: T13452
Differential Revision: https://secure.phabricator.com/D20898
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
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
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
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
Summary:
Ref T13410. We currently generate some less-than-ideal anchors in remarkup, but it's hard to change the algorithm without breaking stuff.
To mitigate this, allow `#xyz` to match any target on the page which begins with `xyz`. This means we can make anchors longer with no damage, and savvy users are free to shorten anchors to produce more presentation-friendly links.
Test Plan: Browsed to `#header-th`, was scrolled to `#header-three`, etc.
Maniphest Tasks: T13410
Differential Revision: https://secure.phabricator.com/D20820
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
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
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
Summary:
Fixes T13363. Currently, these are genuine links which we intercept events for.
Make them pseudolinks instead. Possible alternative approaches are:
- Keep them as genuine links, but mark them as non-navigation links for Quicksand. (But: yuck, weird special case.)
- Keep them as genuine links, and have the dialog handler `JX.Stratcom.pass()` to see if anything handles the event. (But: the "pass()" pattern generally feels bad.)
"Tableaus" or whatever comes out of T10469 some day will probably break everything anyway?
Test Plan:
- Opened the "Edit Related Tasks... > Edit Subtasks" dialog.
- Clicked task title links (not the "open in new window" icon, and not the "Select" button).
- Before: Dialog (sometimes) closed abruptly.
- After: Task is consistently selected as part of the attachment set.
Maniphest Tasks: T13363
Differential Revision: https://secure.phabricator.com/D20693
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
Summary:
Depends on D20382. Ref T13272. When something near the edge of the screen has a dropdown menu, we currently may render the menu offscreen.
Instead, keep the menu onscreen.
(This is happening because I'm adding dropdown menus to tab query panels.)
Test Plan:
Before:
{F6363339}
After:
{F6363340}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20383
Summary:
See PHI1185, which reports a performance issue with "(" in remarkup in certain contexts.
I can't reproduce the performance issue, but I can reproduce the autosuggester incorrectly remaining active and swallowing return characters.
When the user types `(` or `[`, we wait for a prefix for the `((` (Phurl) or `[[` (Phriction) rules. We currently continue looking for that prefix until a character is entered that explicitly interrupts the search.
For example, typing `(xxx<return>` does not insert a return character, because we're stuck on matching the prefix.
Instead, as soon as the user has entered text that we know won't ever match the prefix, deactivate the autocomplete. We can slightly cheat through this by just looking for at least one character of text, since all prefixes are exactly one character long. If we eventually have some kind of `~~@(xyz)` rule we might need to add a more complicated piece of rejection logic.
Test Plan: Typed `(xxx<return>`, got a return. Used `((` and `[[` autosuggest rules normally. Used `JX.log()` to sanity check that nothing too crazy seems to be happening.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20365
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
Summary:
See downstream <https://phabricator.wikimedia.org/T201480>. Searching for things on mobile is a significant challenge because clicking the "Magnifying Glass" icon shows and then immediately hides the menu. I believe some aspect of iOS event handling has changed since this was originally written.
At some point, I'd like to rewrite this to work more cleanly and get rid of `jx-toggle-class`. In particular, it isn't smart enough to know that it should be modal with other menus, so you can get states like this by clicking multiple things:
{F6320110}
This would also probably just look and work better if it was an inline element that showed up under the header instead of a floating dropdown element.
However, I'm having a hard time getting the Safari debugger to actually connect to the iOS simulator, so take a small step toward this bright future and fix the immediate problem for now: toggle on click instead of mousedown/touchstart.
This means the menu opens ~100ms later, but actually works. Big improvement!
I'd like to move away from "jx-toggle-class" anyway (it usually isn't sophisticated enough to fully describe a behavior) so reducing complexity here seems good. It isn't used in //too// many places so this is unlikely to have any negative effects, I hope.
Test Plan: On iOS simulator, clicked the magnifying glass icon in the main menu to get a search input. Before: got a search input for a microsecond. After: actually got a search input.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20343
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
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
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
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
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
Summary:
Ref T13074. If you hit a prompt on a drop operation (today: MFA; in the future, maybe "add a comment" or "assign this task"), we currently leave the board in a bad semi-frozen state if you cancel the workflow by pressing "Cancel" on the dialog.
Instead, put things back the way they were.
Test Plan: Dragged an MFA-required card, cancelled the MFA prompt, got a functional board instead of a semi-frozen board I needed to reload.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13074
Differential Revision: https://secure.phabricator.com/D20305
Summary:
Ref T5474. The first rough cut of triggers showed some of the trigger rules in a tooltip when you hover over the "add/remove" trigger menu.
This isn't great since we don't have much room and it's a bit finnicky / hard to read.
Since we have a better way to show effects now in the drop preview, just use that instead. When you hover over the trigger menu, preview the trigger in the "drop effect" element, with a "Trigger: such-and-such" header.
Test Plan:
- This is pretty tough to screenshot.
- Hovered over menu, got a sensible preview of the trigger effects.
- Dragged a card over the menu, no preview.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T5474
Differential Revision: https://secure.phabricator.com/D20304
Summary:
Ref T5474. This provides a Herald-like UI for editing workboard trigger rules.
This probably has some missing pieces and doesn't actually save anything to the database yet, but the basics at least roughly work.
Test Plan: {F6299886}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T5474
Differential Revision: https://secure.phabricator.com/D20301
Summary:
Ref T10335. When you (for example) drag a "Resolved" task into a column with "Trigger: change status to resolved.", don't show a hint that the action will "Change status to resolved." since this isn't helpful and is somewhat confusing.
For now, the only visibility operator is "!=" since all current actions are simple field comparisons, but some actions in the future (like "add subscriber" or "remove project") might need other conditions.
Test Plan:
Dragged cards in ways that previously provided useless hints: move from column A to column B on a "Group by Priority" board; drag a resolved task to a "Trigger: change status to as resolved" column. Saw a more accurate preview in both cases.
Drags which actually cause effects still show the effects correctly.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10335
Differential Revision: https://secure.phabricator.com/D20300
Summary:
Ref T10335. Ref T5474. When you drag-and-drop a card on a workboard, show a UI hint which lists all the things that the operation will do.
This shows: column moves; changes because of dragging a card to a different header; and changes which will be caused by triggers.
Not implemented here:
- Actions are currently shown even if they have no effect. For example, if you drag a "Normal" task to a different column, it says "Change priority to Normal.". I plan to hide actions which have no effect, but figuring this out is a little bit tricky.
- I'd like to make "trigger effects" vs "non-trigger effects" a little more clear in the future, probably.
Test Plan:
Dragged stuff between columns and headers, and into columns with triggers. Got appropriate preview text hints previewing what the action would do in the UI.
(This is tricky to take a screenshot of since it only shows up while the mouse cursor is down.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10335, T5474
Differential Revision: https://secure.phabricator.com/D20299
Summary:
Depends on D20274. Ref T10578. This is en route to an ordering by points, it's just a simpler half-step on the way there.
Allow columns to be sorted by creation date, so the newest tasks rise to the top.
In this ordering you can never reposition cards, since editing a creation date by dragging makes no sense. This will be true of the "points" ordering too (although we could imagine doing something like prompting the user, some day).
Test Plan: Viewed boards by "natural" (allows reordering both when dragging within and between columns), "priority" (reorder only within columns), and "creation date" (reorder never). Dragged cards around between and within columns, got apparently sensible behavior.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10578
Differential Revision: https://secure.phabricator.com/D20275
Summary:
Depends on D20273. Fixes T10722. Currently, we don't make it very clear when a card can't be edited. Long ago, some code made a weak attempt to do this (by hiding the "grip" on the card), but later UI changes hid the "grip" unconditionally so that mooted things.
Instead:
- Replace the edit pencil with a red lock.
- Provide cursor hints for grabbable / not grabbable.
- Don't let users pick up cards they can't edit.
Test Plan: On a workboard with a mixture of editable and not-editable cards, hovered over the different cards and was able to figure out which ones I could drag or not drag pretty easily. Picked up cards I could pick up, wasn't able to drag cards I can't edit.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10722
Differential Revision: https://secure.phabricator.com/D20274
Summary:
Depends on D20271. Ref T10333. When a column is empty but a board is grouped (by priority, owner, etc) render the headers properly.
When a column has headers, don't apply the "empty" style even if it has no cards. This style just makes some empty space so you can drag-and-drop more easily, but headers do the same thing.
Test Plan: {F6264611}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10333
Differential Revision: https://secure.phabricator.com/D20272
Summary:
Depends on D20267. Depends on D20268. Ref T10333. Currently, we support "Natural" and "Priority" orders, but a lot of the particulars are pretty hard-coded, including some logic in `ManiphestTask`.
Although it's not clear that we'll ever put other types of objects on workboards, it seems generally bad that you need to modify `ManiphestTask` to get a new ordering.
Pull the ordering logic out into a `ProjectColumnOrder` hierarchy instead, and let each ordering define the things it needs to work (name, icon, what headers look like, how different objects are sorted, and how to apply an edit when you drop an object under a header).
Then move the existing "Natural" and "Priority" orders into this new hierarchy.
This has a minor bug where using the "Edit" workflow to change a card's priority on a priority-ordered board doesn't fully refresh card/header order since the response isn't ordering-aware. I'll fix that in an upcoming change.
Test Plan: Grouped workboards by "Natural" and "Priority", dragged stuff around within and between columns, grepped for all touched symbols.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10333
Differential Revision: https://secure.phabricator.com/D20269
Summary:
Depends on D20266. Boards currently have several `whateverMap<cardPHID => stuff>` properties, but we can just move these all down into a `CardTemplate`, similar to the recently introduced `HeaderTemplate`.
The `CardTemplate` holds all the global information for a card, and then `Card` is specific for a particular copy in a column. Today, each `CardTemplate` has one `Card`, but a `CardTemplate` may have more than one card in the future (when we add subproject columns).
Test Plan: Viewed workboards in different sort orders and dragged stuff around, grepped for all affected symbols.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20267
Summary:
Depends on D20263. Ref T10333. I want to add groups like "Assignee" to workboards. This means you may have several tasks grouped under, say, "Alice".
When you drag the bottom-most task under "Alice" to the top, what does that mean?
Today, the only grouping is "Priority", and it means "change the task's secret/hidden global subpriority". However, this seems to generally be a somewhat-bad answer, and is quite complex. It also doesn't make much sense for an author grouping, since one task can't really be "more assigned" to Alice than another task.
Users likely intend this operation to mean "move it, visually, with no other effects" -- that is, user intent is to shuffle sticky notes around on a board, not edit anything substantive. The meaning is probably something like "this is similar to other nearby tasks" or "maybe this is a good place to start", which we can't really capture with any top-level attribute.
We could extend "subpriority" and give tasks a secret/hidden "sub-assignment strength" and so on, but this seems like a bad road to walk down. We'll also run into trouble later when subproject columns may appear on the board, and a user could want to put a task in different positions on different subprojects, conceivably.
In the "Natural" order view, we already have what is probably a generally better approach for this: a task display order particular to the column, that just remembers where you put the sticky notes.
Move away from "subpriority", and toward a world where we mostly keep sticky notes where you stuck them and move them around only when we have to. With no grouping, we still sort by "natural" order, as before. With priority grouping, we now sort by `<priority, natural>`. When you drag stuff around inside a priority group, we update the natural order.
This means that moving cards around on a "priority" board will also move them around on a "natural" board, at least somewhat. I think this is okay. If it's not intuitive, we could give every ordering its own separate "natural" view, so we remember where you stuck stuff on the "priority" board but that doesn't affect the "Natural" board. But I suspect we won't need to.
Test Plan:
- Viewed and dragged a natural board.
- Viewed and dragged a priority board.
- Dragged within and between groups of 0, 1, and multiple items.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10333
Differential Revision: https://secure.phabricator.com/D20265
Summary:
Ref T13074. Today, in normal task list views in Maniphest (not workboards), you can (sometimes) reorder tasks if the view is priority-sorted.
I suspect no one ever does this, few users know it's supported, and that it was basically rendered obsolete the day we shipped workboards.
This also means that we need to maintain a global "subpriority" for tasks, which distinguishes between different tasks at the same priority level (e.g., "High") and maintains a consistent ordering on workboards.
As we move toward making workboards more flexible (e.g., group by author / owner / custom fields), I'd like to try moving away from "subpriority" and possibly removing it entirely, in favor of "natural order", which basically means "we kind of remember where you put the card and it works a bit like a sticky note".
Currently, the "natural order" and "subpriority" systems are sort of similar but also sort of in conflict, and the "subpriority" system can't really be extended while the "natural order / column position" system can.
The only real reason to have a global "subpriority" is to support the list-view drag-and-drop.
It's possible I'm wrong about this and a bunch of users love this feature, but we can re-evaluate if we get feedback in this vein.
(This just removes UI, the actual subpriority system is still intact and still used on workboards.)
Test Plan: Viewed task lists, was no longer able to drag stuff. Grepped for affected symbols. Dragged stuff in remaining grippable lists, like "Edit Forms" in EditEngine config.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13074
Differential Revision: https://secure.phabricator.com/D20263
Summary:
Ref T10333. Ref T8135. Depends on D20247. Allow users to drag-and-drop cards on a priority-sorted workboard under headers, even if the header has no other cards.
As of D20247, headers show up but they aren't really interactive. Now, you can drag cards directly underneath a header (instead of only between other cards). For example, if a column has only one "Wishlist" task, you may drag it under the "High", "Normal", or "Low" priority headers to select a specific priority.
(Some of this code still feels a little rough, but I think it will generalize once other types of sorting are available.)
Test Plan: Dragged cards within and between priority groups, saw appropriate priority edits applied in every case I could come up with.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10333, T8135
Differential Revision: https://secure.phabricator.com/D20248
Summary:
Ref T10333. When workboards are ordered (for example, by priority), add headers to the various groups. Major goals are:
- Allow users to drag-and-drop to set values that no cards currently have: for example, you can change a card priority to "normal" by dragging it under the "normal" header, even if no other cards in the column are currently "Normal".
- Make future orderings more useful, particularly "order by assignee". We don't really have room to put the username on every card and it would create a fair amount of clutter, but we can put usernames in these headers and then reference them with just the profile picture. This also allows you to assign to users who are not currently assigned anything in a given column.
- Make the drag-and-drop behavior more obvious by showing what it will do more clearly (see T8135).
- Make things a little easier to scan in general: because space on cards is limited, some information isn't conveyed very clearly (for example, priority information is currently conveyed //only// through color, which can be hard to pick out visually and is probably not functional for users who need vision accommodations).
- Maybe do "swimlanes": this is pretty much a "swimlanes" UI if we add whitespace at the bottom of each group so that the headers line up across all the columns (e.g., "Normal" is at the same y-axis position in every column as you scroll down the page). Not sold on this being useful, but it's just a UI adjustment if we do want to try it.
NOTE: This only makes these headers work for display.
They aren't yet recognized as targets by the drag list UI, so you can't drag cards into an empty group. I'll tackle that in a followup.
Test Plan: {F6257686}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10333
Differential Revision: https://secure.phabricator.com/D20247
Summary:
Ref T10334. Partly, this just improves visual feedback for all drag operations. After D20242, we can have cases where you (for example) drag a low-priority node to a very tall column on a priority-ordered workboard. In this case, the actual dashed-border-drop-target may not be on screen.
We might make the column scroll or put some kind of hint in the UI in this case, but an easy starting point is just to make the "yes, you're targeting this column" state a bit more clear.
Test Plan: Dragged tasks between columns, saw the border higlight on the target columns. This is very tricky to take a screenshot of.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10334
Differential Revision: https://secure.phabricator.com/D20245
Summary:
Ref T10334. When a workboard is ordered by priority, dragging from column "A" to a particular place in column "B" currently means "move this task to column B, and adjust its priority so that it naturally sorts into the location under my mouse cursor".
Users frequently find this confusing / undesirable.
To begin improving this, make "drag from column A to column B" and "drag from somewhere in column A to somewhere else in column A" into different operations. The first operation, a movement between columns, no longer implies an ordering change. The second action still does.
So if you actually want to change the priority of a task, you drag it within its current column. If you just want to move it to a different column, you drag it between columns.
This creates some possible problems:
- Some users may love the current behavior and just not be very vocal about it. I doubt it, but presumably we'll hear from them if we break it.
- If you actualy want to move + reorder, it's a bit more cumbersome now. We could possibly add something like "shift + drag" for this if there's feedback.
- The new behavior is probably less surprising, but may not be much more obvious. Future changes (for example, in T10335) should help make it more clear.
- When you mouse cursor goes over column B, the card dashed-rectangle preview target thing jumps to the correct position in the column -- but that may not be under your mouse cursor. This feels pretty much fine if the whole column fits on screen. It may not be so great if the column does not fit on screen and the dashed-rectangle-thing has vanished. This is just a UI feedback issue and we could refine this later (scroll/highlight the column).
Test Plan:
- Created several tasks at different priority levels, sorted a board by priority, dragged tasks between columns. Dragging from "A" to "B" no longer causes a priority edit.
- Also, dragged within a column. This still performs priority edits.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10334
Differential Revision: https://secure.phabricator.com/D20242
Summary:
Ref T12822. Ref PHI878. This is some leftover code from the old selection behavior that prevented visual selection of the left side of a diff if the user clicked on the right -- basically, a much simpler attack on what ultimately landed in D20191.
I think the change from `th` to `td` "broke" it so it didn't interfere with the other behavior, which is why I didn't have to remove it earlier. It's no longer necessary, in any case.
Test Plan: Grepped for behavior name, selected stuff on both sides of a diff.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12822
Differential Revision: https://secure.phabricator.com/D20196
Summary:
Ref T12822. Ref T2495. This is the good version of D20193.
Currently, we display various nonprintable characters (ZWS, nonbreaking space, various control characters) as themselves, so they're generally invisible.
In T12822, one user reports that all their engineers frequently type ZWS characters into source somehow? I don't really believe this (??), and this should be fixed in lint.
That said, the only real reason not to show these weird characters in a special way was that it would break copy/paste: if we render ZWS as "🐑", and a user copy-pastes the line including the ZWS, they'll get a sheep.
At least, they would have, until D20191. Now that this whole thing is end-to-end Javascript magic, we can copy whatever we want.
In particular, we can render any character `X` as `<span data-copy-text="Y">X</span>`, and then copy "Y" instead of "X" when the user copies the node. Limitations:
- If users select only "X", they'll get "X" on their clipboard. This seems fine. If you're selecting our ZWS marker *only*, you probably want to copy it?
- If "X" is more than one character long, users will get the full "Y" if they select any part of "X". At least here, this only matters when "X" is several spaces and "Y" is a tab. This also seems fine.
- We have to be kind of careful because this approach involves editing an HTML blob directly. However, we already do that elsewhere and this isn't really too hard to get right.
With those tools in hand:
- Replace "\t" (raw text / what gets copied) with the number of spaces to the next tab stop for display.
- Replace ZWS and NBSP (raw text) with a special marker for display.
- Replace control characters 0x00-0x19 and 0x7F, except for "\t", "\r", and "\n", with the special unicode "control character pictures" reserved for this purpose.
Test Plan:
- Generated and viewed a file like this one:
{F6220422}
- Copied text out of it, got authentic raw original source text instead of displayed text.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12822, T2495
Differential Revision: https://secure.phabricator.com/D20194
Summary:
Ref T12822. Fixes a few things:
- Firefox selection of weird ranges with an inline between the start and end of the range now works correctly.
- "Show More Context" rows now render, highlight, and select properly.
- Prepares for nodes to have copy-text which is different from display-text.
- Don't do anything too fancy in 1-up/unified mode. We don't copy line numbers after the `content: attr(data-n)` change, but that's as far as we go, because trying to do more than that is kind of weird and not terribly intuitive.
Test Plan:
- Selected and copied weird ranges in Firefox.
- Kept an eye on "Show More Context" rows across select and copy operations.
- Generally poked around in Safari/Firefox/Chrome.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12822
Differential Revision: https://secure.phabricator.com/D20192
Summary:
Ref T12822. Ref T13161. By default, when users select text from a diff and copy it to the clipboard, they get both sides of the diff and all the line numbers. This is usually not what they intended to copy.
As of D20188, we use `content: attr(...)` to render line numbers. No browser copies this text, so that fixes line numbers.
We can use "user-select" CSS to visually prevent selection of line numbers and other stuff we don't want to copy. In Firefox and Chrome, "user-select" also applies to copied text, so getting "user-select" on the right nodes is largely good enough to do what we want.
In Safari, "user-select" is only visual, so we always need to crawl the DOM to figure out what text to pull out of it anyway.
In all browsers, we likely want to crawl the DOM anyway because this will let us show one piece of text and copy a different piece of text. We probably want to do this in the future to preserve "\t" tabs, and possibly to let us render certain character codes in one way but copy their original values. For example, we could render "\x07" as "␇".
Finally, we have to figure out which side of the diff we're copying from. The rule here is:
- If you start the selection by clicking somewhere on the left or right side of the diff, that's what you're copying.
- Otherwise, use normal document copy rules.
So the overall flow here is:
- Listen for clicks.
- When the user clicks the left or right side of the diff, store what they clicked.
- When a selection starts, and something is actually selected, check if it was initiated by clicking a diff. If it was, apply a visual effect to get "user-select" where it needs to go and show the user what we think they're doing and what we're going to copy.
- (Then, try to handle a bunch of degenerate cases where you start a selection and then click inside that selection.)
- When a user clicks elsewhere or ends the selection with nothing selected, clear the selection mode.
- When a user copies text, if we have an active selection mode, pull all the selected nodes out of the DOM and filter out the ones we don't want to copy, then stitch the text back together. Although I believe this didn't work well in ~2010, it appears to work well today.
Test Plan: This mostly seems to work in Safari, Chrome, and Firefox. T12822 has some errata. I haven't tested touch events but am satisfied if the touch event story is anything better than "permanently destroys data".
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161, T12822
Differential Revision: https://secure.phabricator.com/D20191
Summary:
Ref T13161. Ref T12822. Today, we use invisible Zero-Width Spaces to try to improve copy/paste behavior from Differential.
After D20188, we no longer need ZWS characters to avoid copying line numbers. Get rid of these secret invisible semantic ZWS characters completely.
This means that both the left-hand and right-hand side of diffs become copyable, which isn't desired. I'll fix that with a hundred thousand lines of Javascript in the next change: this is a step toward everything working better, but doesn't fix everything yet.
Test Plan:
- Grepped for `zws`, `grep -i zero | grep -i width`.
- Copied text out of Differential: got both sides of the diff (not ideal).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161, T12822
Differential Revision: https://secure.phabricator.com/D20189
Summary:
Ref T13161. Ref T12822. See PHI870. Long ago, the web was simple. You could leave your doors unlocked, you knew all your neighbors, crime hadn't been invented yet, and `<th>3</th>` was a perfectly fine way to render a line number cell containing the number "3".
But times have changed!
- In PHI870, this isn't good for screenreaders. We can't do much about this, so switch to `<td>`.
- In D19349 / T13105 and elsewhere, this `::after { content: attr(data-n); }` approach seems like the least bad general-purpose approach for preventing line numbers from being copied. Although Differential needs even more magic beyond this in the two-up view, this is likely good enough for the one-up view, and is consistent with other views (paste, harbormaster logs, general source display) where this technique is sufficient on its own.
The chance this breaks //something// is pretty much 100%, but we've got a week to figure out what it breaks. I couldn't find any issues immediately.
Test Plan:
- Created, edited, deleted inlines in 1-up and 2-up views.
- Replied, keyboard-navigated, keyboard-replied, drag-selected, poked and prodded everything.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161, T12822
Differential Revision: https://secure.phabricator.com/D20188
Summary:
Depends on D20181. Depends on D20182. Fixes T3498. Ref T13161. My claim, at least, is that D20181 can be tweaked to be good enough to throw away this "feature" completely.
I think this feature was sort of a mistake, where the ease of access to `diff -bw` shaped behavior a very long time ago and then the train just ran a long way down the tracks in the same direction.
Test Plan: Grepped for `whitespace`, deleted almost everything. Poked around the UI a bit. I'm expecting the whitespace changes to get some more iteration this week so I not being hugely pedantic about testing this stuff exhaustively.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161, T3498
Differential Revision: https://secure.phabricator.com/D20185
Summary:
Ref T13249. See PHI810. We currently show availability dots in some interfaces (timeline, mentions) but not others (typeheads/tokenizers).
They're potentially quite useful in tokenizers, e.g. when assigning tasks to someone or requesting reviews. Show them in more places.
(The actual rendering here isn't terribly clean, and it would be great to try to unify all these various behaviors some day.)
Test Plan:
{F6212044}
{F6212045}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20173
Summary: Ref T13249. Poll for Duo updates in the background so we can automatically update the UI when the user clicks the mobile phone app button.
Test Plan: Hit a Duo gate, clicked "Approve" in the mobile app, saw the UI update immediately.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20169
Summary: See PHI1017. This is a trivial fix even though these burnups are headed toward a grisly fate.
Test Plan: Moused over some January datapoints, saw "1" instead of "0".
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19967
Summary:
Ref T13222. In D19918, I refactored how timelines get "view data". Today, this is always additional data about which images/changesets/diffs are visible on the current revision/commit/mock, so we can tell if inline comments should be linked to a `#anchor` on the same page (if the inline is rendered there somewhere) or to a `/D123?id=1&vs=2` full link on a different page (if it isn't), but in general this could be any sort of state information about the current page that affects how the timeline should render.
Previously, comment previews did not use any specialized object code and always rendered a "generic" timeline story. This was actually a bug, but none of the code we have today cares about this (since it's all inline related, and inlines render separately) so it never impacted anything.
After the `TimelineEngine` change, the preview renders with Differential-specific code. This is more correct, but we were not passing the preview the "view data" so it broke.
This preview doesn't actually need the view data and we could just make it bail out if it isn't present, but pass it through for consistency and so this works like we'd expect if we do something fancier with view data in the future.
Test Plan: Viewed comment and inline comment previews in Differential, saw old behavior restored.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19943
Summary:
Depends on D19914. Ref T11351. Some of the Phoilo rabbit holes go very deep.
`PhabricatorApplicationTransactionInterface` currently requires you to implement `willRenderTimeline()`. Almost every object just implements this as `return $timeline`; only Pholio, Diffusion, and Differential specialize it. In all cases, they are specializing it mostly to render inline comments.
The actual implementations are a bit of a weird mess and the way the data is threaded through the call stack is weird and not very modern.
Try to clean this up:
- Stop requiring `willRenderTimeline()` to be implemented.
- Stop requiring `getApplicationTransactionViewObject()` to be implemented (only the three above, plus Legalpad, implement this, and Legalpad's implementation is a no-op). These two methods are inherently pretty coupled for almost any reasonable thing you might want to do with the timeline.
- Simplify the handling of "renderdata" and call it "View Data". This is additional information about the current view of the transaction timeline that is required to render it correctly. This is only used in Differential, to decide if we can link an inline comment to an anchor on the same page or should link it to another page. We could perhaps do this on the client instead, but having this data doesn't seem inherently bad to me.
- If objects want to customize timeline rendering, they now implement `PhabricatorTimelineInterface` and provide a `TimelineEngine` which gets a nice formal stack.
This leaves a lot of empty `willRenderTimeline()` implementations hanging around. I'll remove these in the next change, it's just going to be deleting a couple dozen copies of an identical empty method implementation.
Test Plan:
- Viewed audits, revisions, and mocks with inline comments.
- Used "Show Older" to page a revision back in history (this is relevant for "View Data").
- Grepped for symbols: willRenderTimeline, getApplicationTransactionViewObject, Legalpad classes.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19918
Summary:
Ref T13222. Fixes T12588. See PHI683. In several cases, we present the user with a choice between multiple major options: Alamnac service types, Drydock blueprint types, Repository VCS types, Herald rule types, etc.
Today, we generally do this with radio buttons and a "Submit" button. This isn't terrible, but often it means users have to click twice (once on the radio; once on submit) when a single click would be sufficient. The radio click target can also be small.
In other cases, we have a container with a link and we'd like to link the entire container: notifications, the `/drydock/` console, etc. We'd like to just link the entire container, but this causes some problems:
- It's not legal to link block eleements like `<a><div> ... </div></a>` and some browsers actually get upset about it.
- We can `<a><span> ... </span></a>` instead, then turn the `<span>` into a block element with CSS -- and this sometimes works, but also has some drawbacks:
- It's not great to do that for screenreaders, since the readable text in the link isn't necessarily very meaningful.
- We can't have any other links inside the element (e.g., details or documentation).
- We can `<form><button> ... </button></form>` instead, but this has its own set of problems:
- You can't right-click to interact with a button in the same way you can with a link.
- Also not great for screenreaders.
Instead, try adding a `linked-container` behavior which just means "when users click this element, pretend they clicked the first link inside it".
This gives us natural HTML (real, legal HTML with actual `<a>` tags) and good screenreader behavior, but allows the effective link target to be visually larger than just the link.
If no issues crop up with this, I'd plan to eventually use this technique in more places (Repositories, Herald, Almanac, Drydock, Notifications menu, etc).
Test Plan:
{F6053035}
- Left-clicked and command-left-clicked the new JS fanciness, got sensible behaviors.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13222, T12588
Differential Revision: https://secure.phabricator.com/D19855
Summary:
Ref T13216. See PHI985. When you click a line number to start an inline comment, we intend to initiate the action only if you used the left mouse button (desktop) or a touch (tablet/device).
We currently have a `not right` condition for doing this, but it only excludes right clicks, not middle clicks (or other nth-button clicks). The `not right` condition was sligthly easier to write, but use an `is left` condition instead of a `not right` condition.
Test Plan:
- In Safari, Firefox and Chrome:
- Used left click to start an inline.
- Used middle click to do nothing (previously: started an inline).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19836
Summary:
See PHI975. Ref T13216. Ref T2543. Previously, see D19204 and PHI433.
When you're acting on a draft revision, we change the button text to "Submit Quietly" as a hint that your actions don't generate notifications yet.
However, this isn't accurate when one of your actions is "Request Review", which causes the revision to publish.
Allow actions to override the submit button text, and make the "Request Review" action change the button text to "Publish Revision".
The alternative change I considered was to remove the word "Quietly" in all cases.
I'm not //thrilled// about how complex this change is to adjust one word, but the various pieces are all fairly clean individually. I'm not sure we'll ever be able to use it for anything else, but I do suspect that the word "Quietly" was the change in D19204 with the largest effect by far (see T10000).
Test Plan:
- Created a draft revision. Saw "Submit Quietly" text.
- Added a "Request Review" action, saw it change to "Publish Revision".
- Reloaded page, saw stack saved and "Publish Revision".
- Removed action, saw "Submit Quietly".
- Repeated on a non-draft revision, button stayed put as "Submit".
- Submitted the various actions, saw them have the desired effects.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216, T2543
Differential Revision: https://secure.phabricator.com/D19810
Summary:
See <https://hackerone.com/reports/434116>. Slowvote has a piece of Javascript that attempts to let you vote on `{V123}` polls inline.
It does not work: nothing ever triggers it (nothing renders a control with a `slowvote-option` sigil).
At least for now, just remove it. It has a completely separate pathway in the controller and both pathways are buggy, so this makes fixing them easier.
Test Plan: Voted in plurality and approval polls via Slowvote and the embedded widget.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19773
