Page MenuHomePhabricator

Inline comment line numbers on DocumentEngine block diffs may be clamped to raw lines in the source file
Open, LowPublic

Description

See PHI2029. An install reports that Jupyter notebook diffs render all inline comments on line 1 if the raw source file is a single line (for example, because the JSON has been minified).

This is likely caused by internal logic clamping the display line to the number of lines in the file.


When you are viewing a change under engine X, and comments made under engine Y are present, they are not handled specially.

Event Timeline

epriestley created this task.

In D21435, inlines were clamped. Adjusting this code is necessary, but not sufficient, to correct this problem.

Generally, that code is part of a larger block in ChangesetParser, starting with this and extending for about 120 lines:

if ($this->comments) {
  // If there are any comments which appear in sections of the file which
  // we don't have, we're going to move them backwards to the closest
  // earlier line. Two cases where this may happen are:

...

All of this logic references $this->old and $this->new, which are the raw text of the change. In the best case, this is incorrect. In cases like a minified JSON source, it clamps every comment to line 1.

This code needs to be modified to work on some abstraction across line-based and block-based diffs.

A likely adjacent condition to consider here is display of inline comments left on a particular of a document when the current view is some other view. For example, if you leave a comment on block 10 of a Jupyter block diff and then view the file as raw text, the comment should be preserved and the lack of a place to anchor it should be clearly identified.