Page MenuHomePhabricator

Make "View Options" in Differential sticky across reloads
Closed, ResolvedPublic

Description

See PHI1548, which would like "Collapse File" to be sticky across reloads. There's some vague philosophical peril here because it pushes us toward file-by-file review, but I'm not overly concerned about that for now. Previously, see T8909, etc.

Generally, most of the "View Options" (highlighting, encoding, unified, etc) would benefit from being sticky across reloads, and the value increases as more options are introduced, as with the abstract block engine stuff recently.

This menu would also probably benefit from a reorganization pass and some dividers, since it has accumulated a lot of options but doesn't have very much structure.

See also PHI1660.


Completed

  • Maybe swap "View Options" entirely for a more compact "Hidden Change / Show Change" state?
  • The "View Options" menu should be reorganized.
  • The "View Options" menu should show keystrokes.
  • See PHI1706. "View in Diffusion" should have a keystroke.
  • See PHI1706 for "View Directory in Diffusion" for paths that have a parent.
  • This data should have a GC on it.

Event Timeline

epriestley created this task.

There is currently no storage unique to a <user, changeset> pair, or a <user, diff> pair, or a <user, revision> pair.

The desired behavior is probably a <userPHID, containerPHID> pair, where attaching a diff to a revision moves all the preferences up if it can. That is, users may do this:

  • Create revision R.
  • Create diff D.
  • In R, set "example.py" to "View As: Rainbow".
  • In D, set "example.py" to "View As: Magenta Text on Lime Background".
  • Attach D to R.
  • Reload R.

What is the user intent for the display behavior of "example.py"? That is, what algorithm should we use to merge conflicting settings at attachment time?

I suspect that in all cases, the answer is "who cares" and "just throw one of them away".

I think the specific "sticky" settings are:

  • Collapse state.
  • Unified diff state.
  • Text Encoding.
  • Highlight As (Syntax Engine)
  • View As (Rendering Engine)

For all options except collapse state, "last save wins" and "discard-on-attach" are reasonable rules.

For collapse state, I'd like to try to implement this rule:

  • If you previously collapsed a version of this file with the same content, collapse the file.
  • If you previously collapsed a version of this file in a later diff, collapse the file.
  • If you previously collapsed a version of this file with the same name, but different content, in an earlier diff, uncollapse the file with a "You collapsed this in diff 3 but it has changed [Collapse Again]" state.

If that proves too hard, I'm fine with last-save-wins + discard-on-attach.


This can possibly be over-engineered by storing:

{
  "<path>": {
    "<changesetPHID>": {
      "diffOrder": <diffID>,
      "epoch": <timestamp-of-last-change>,
      "value": ...
    }
  }
}

Then the runtime behavior can freely be varied without breaking things, although the cost of losing all this data is so small that it probably doesn't really matter.

None of this needs to be keyed so the actual table can be <userPHID, objectPHID, giant-blob>.

Updating renderer is a big oof. The value sometimes depends on the window dimensions on the client.

To save this setting, the shortest path is to just reload the change with an option like justSaveViewStateDoNotActuallyRenderAnything=true. But, since reloading changes is per-changeset, this is inefficient if filetree changes out of T13516 support collapsing multiple files at once.

It seems like they probably should: if you collapse a/ in the filetree, we should collapse a/x.txt and a/y.txt in the actual change view. But perhaps this should only happen on the client, and persist as a piece of filetree state -- then the actual answer to "should a/x.txt be rendered collapsed?" is "is that path, or any parent path, collapsed?".

This makes the decision on the server about whether it should render a "collapsed" or "expanded" state for a given file more difficult, although maybe it doesn't need to make this decision and can let the client deal with everything.

This becomes particularly complicated when it interacts with updates:

  • Alice collapsed a/b.txt in Diff 1.
  • Alice collapsed (clicked the disclosure triangle closed) a/ in Diff 1.
  • The change is updated. b.txt is modified.
  • Alice loads the new revision.

Per intent above, I'd like b.txt to become visible ("You collapsed this file before, but it has changed since then."). (Related issue: if another user publishes an inline comment on a collapsed file, should that reopen the file?) But all the other files in a/ should still be collapsed.

I think the filetree can do this as long as the complexity of the "collapsed" state is preserved all the way to the client, but it suggests that a filetree collapse needs to be marked with the logical clock version (diff ID) so we can tell if you closed a particular folder while looking at an older or newer diff.

Currently, I also think no part of this pipeline can actually tell if changeset X changed at diff Y or not, but this isn't conceptually hard to get right most of the time.

For inlines, I'm inclined to say "adding inlines does not ever un-collapse files" since this makes my life easier and there are other channels available to keep track of inlines.

Currently, I also think no part of this pipeline can actually tell if changeset X changed at diff Y or not, but this isn't conceptually hard to get right most of the time.

There's an existing $changeset->hasSameEffectAs($other_changeset) from T13137 that probably isn't perfect but should more or less do everything I need it to.

  • Followups on "Hidden" logic have moved to T13519.