Page MenuHomePhabricator

Modernize display code for inline comments
Closed, ResolvedPublic

Description

The display layer for inline comments hasn't been updated in a long time is currently under-engineered.

We have a large number of glitchy interactions because the various components are too loosely organized. For example, the keyboard selection reticle and inline edit workflow basically break every time they interact between them because they aren't really aware of one another.

I plan to rework the display layer to generally follow the patterns present in the Workboards code. (Conpherence will also get an update in this vein in T12562.)


TODOS

  • Determine if we need to cache cursor state DOM information for keyboard navigation ("n", "p", etc) for performance? (Seems OK so far.)

TODONES

  • Restore "r" to reply to inlines.
  • Restore "e" to edit inlines.
  • Toggle file action doesn't yet interact appropriately with inlines: you can still "j" and "k" yourself over hidden blocks.
  • After a keyboard-based "reply" or "edit" operation completes, we don't put the keyboard state back where it was, exactly. We're close on "reply" but less close on "edit". We're also far off on "hide", where we lose comment state.
  • The ability to undo a discarded edit has been lost.
  • Fetching comments by ID doesn't find comments which haven't been interacted with yet.
  • Restore "h" to collapse or expand files.

Revisions and Commits

rP Phabricator
D17940
D17937
D17928
D17927
D17920
D17919
D17916
D17913
D17908
D17895
D17859
D17894
D17893
D17890
D17888
D17887
D17861
D17846
D17845
D17843
D17842
D17841
D17840

Event Timeline

@chad, some design stuff I'm thinking for later on, about in case you have feedback/ideas/objections:

Further down this path are T8909 and some related technical issues, like "if you send someone an anchor link to a ghost inline, but they have ghosts off, the anchor link doesn't work". To address these, I think we need to add a modal selector to the entire revision page with inline display options.

  • To address T8909, users would switch this selector between "All Comments" and "New Comments" or similar.
  • To address "anchors to inlines you can't see with current settings", the page would automatically switch this selector into the right mode to make the comment visible (and maybe provide a UI hint that it did so).

We could implement this selector by putting it in the curtain menu (but then it will be hard to access during review, since you'd have to scroll up), or by making it a fairly hidden keyboard-only thing (but then it will be hard to discover), but I'm inclined to try to revisit the global sticky floating bar (T1591) so we end up with this element:

+-------------------------------------------+
| path/to/current/file.c              [= v] |
+-------------------------------------------+

The [=] thing on the right would be a dropdown menu with options like "Inlines: Off" and "Inlines: On".

We could also move "Whitespace" options there: they got sort of buried when we collapsed them into the "History" tab and I think putting them here would make a little more sense.

And we could move "Show Older Inlines" out of SettingsDiff Preferences into this menu to simplify settings a little bit. If we do, I guess this menu state is probably temporary (that is, changes are not persisted by default) with a "Save Defaults" option.

(On mobile, I'd probably just keep this element in a fixed position on the page and not try to have it float or track scroll position, but we could see how things work.)

On mobile, I'd probably just keep this element in a fixed position on the page

That is, just have it be a normal static element at the top of the changes section, not a floating bar. "fixed" in the sense of "static", not position: fixed.

Just some notes for myself:

  • DifferentialChangesetListView is used by both Differential and Diffusion, and should perhaps be renamed (Diff vs Differential).
    • It invokes the differential-populate behavior.
      • This behavior is mostly-sensible and builds a ChangesetViewManager for each changeset. It doesn't build a containing object. Renaming ChangesetViewManager to DiffChangeset and introducing DiffChangesetList here might be the easiest attack.
      • This behavior has some event handling which should move to DiffChangeset or DiffChangesetList.
      • This behavior has some coverage hovering stuff which should also move.
      • This behavior is not Quicksand aware.
      • There is possible interaction here with T11784 and T5030, but things don't seem too bad/buggy overall.
  • differential-dropdown-menus is a separate behavior which has a weird set of responsibilities that should probably merge into differential-populate -- but maybe there are cases where dropdown menus appear after a load and this needs to fire separately?
  • differential-edit-inline-comments is in worse shape, and does a lot of implicit state management with a global singleton DifferentialInlineCommentEditor.
  • DifferentialInlineCommentEditor sort of awkwardly represents the overall state of the current edit operation, and is sort of a singleton. This state management should move to each individual DiffInline so that multiple inlines can be edited simultaneously if we want to support this. We may not, but it's currently impossible to support. The class itself isn't too terrible.
  • differential-comment-jump is a tiny behavior which only handles "next" and "prev" links.
  • keyboard-nav is another separate behavior which handles some of the keyboard behaviors.

Possible approach:

  • Introduce DiffChangesetList, build it in behavior-populate, make it Quicksand-aware.
  • Either rename ChangesetViewManager to DiffChangeset or build the latter out of the former. When these objects are created, enroll them in DiffChangesetList and/or make DiffChangesetList responsible for building them.
  • Build DiffInline out of DifferentialInlineCommentEditor and the large amount of behavior in behavior-edit-inline-comments. An incremental attack on this remains a bit unclear to me. CommentEditor is a weird state-encapsulating object that should become an inline-representing object.
  • We're probably in good shape once we get this far, and cleaning up everything else should be clearcut? Mostly?
  • Then, introduce the new buoyant behavior and collapse behaviors.

I'm having a tough time picking pieces apart here so that the transition is gradual -- many of the remaining behaviors are highly connected to one another and hard to tease apart. Things feel fine up to D17846, but even D17859 breaks enough stuff that I don't feel great about it, and doesn't seem to have pushed me through things enough to emerge on the other side.

The next thing I tried was differential-edit-inline-comments, but I'm going to walk things back a little and maybe see if there's a more gradual approach available by attacking InlineEditor instead, since I feel like I'm just digging deeper and deeper without a real end in sight on my current heading.

epriestley updated the task description. (Show Details)
epriestley updated the task description. (Show Details)

I think we're mostly in the clear, now. This stuff likely introduces some bugs beyond the (mostly minor) ones I'm aware of, but I think it's all correct enough in the substantial ways. I'm going to take a break for a bit; I may get back to it tonight and land everything or may wait until tomorrow.

epriestley claimed this task.

I eventually muddled though things, and I believe work here has now substantially concluded.

Is this open for test/feedback (I think it's usually called "Errata" here)?

We haven't started the new features yet

Yeah, give us a bit longer for general feedback -- I think I'll file a feedback task tomorrow with the release cut.

If you've caught any "this is obviously completely broken" stuff, feel free to yell though. I don't expect anything to be crippled and could have missed some stuff like "collapsing files doesn't work at all any more on Chrome" or "leaving a comment with the word 'bread' fatals" or something.

If it's more feedback-ey like "the old way that stuff highlighted was better" or "now that I can open 30 editors you have given us too much power" or "letting every comment enter an undo state makes a gigantic mess", give me to tomorrow and we can start collecting it alongside the "drop shadow has wrong color" sorts of feedback that we might get.

Err yea I didn't mean feedback as in "this is what I'd like to see" but more of bug reports. I did come across a bug but it's neither earth-shattering nor consistently reproducible.

you take your made-up ghost bugs and go sit in the corner and think about what you've done

I'll try to figure out more details/reproducibility for tomorrow, but through some combination of J+h+@ the highlight reticle will jump somewhere in the timeline

Proof of sanity

(the diff has multiple diffs in history, older inline comments of which I hid/collapsed some - I also have default (Enabled) value for showing older inline diffs)

I tried to go to D17840 to test on this install but @chad's inline comment seems to have mysteriously disappeared