Make "Reply" on ghost inlines work correctly (or, at least, consistently)
ClosedPublic
Actions

Authored by epriestley on Apr 21 2015, 2:30 PM.

Details

Reviewers

Maniphest Tasks

T7870: List of cases where ghost comments end up somewhere confusing or unexpected
T7447: Bring inline comments forward across revision updates

Commits

Restricted Diffusion Commit
rPc596c358db11: Make "Reply" on ghost inlines work correctly (or, at least, consistently)

Summary

Ref T7447. Ref T7870. When you "reply" to a ghost inline, make it work properly.

This exact behavior is arguable. In particular, when you reply to a ghost inline, we could put the reply on the same diff as the original.

I suspect it aligns better with user exepectation to put the new inline on the current (visible) diff instead, and generally for inlines to flow forward through time and all of the ghosts to pretty much be older than all of the non-ghosts in most cases. We can see how it feels and adjust things if this turns out to not make sense.

Test Plan

Replied to ghost inlines, got new inlines on the proper display line.
Replied to normal inlines, got normal behavior.
Made some new inlines.

Diff Detail

Repository

rP Phabricator

Branch

ghost7

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 5419
Build 5437: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

epriestley updated this revision to Diff 29987.Apr 21 2015, 2:30 PM

epriestley retitled this revision from to Make "Reply" on ghost inlines work correctly (or, at least, consistently).

epriestley updated this object.

epriestley edited the test plan for this revision. (Show Details)

epriestley added a reviewer: btrahan.

epriestley added a task: T7447: Bring inline comments forward across revision updates.

Herald added a subscriber: epriestley. · View Herald TranscriptApr 21 2015, 2:30 PM

epriestley mentioned this in T7870: List of cases where ghost comments end up somewhere confusing or unexpected.Apr 21 2015, 2:47 PM

epriestley updated this object.

epriestley added a task: T7870: List of cases where ghost comments end up somewhere confusing or unexpected.

I think the best behavior would probably be to have it show up in both places. That way its like comments are this threaded thing that ghostly moves around as a unit.

This revision is now accepted and ready to land.Apr 21 2015, 5:36 PM

Yeah, it will show up in both places with either approach.

With this approach, it will show up in the "submitted" colors when looking at the diff it was added to, and the "ghostly" colors when looking at the older diff.

With the other approach, it would show up with "ghostly" colors when looking at the diff it was added to, and "submitted" colors when looking at the older diff.

There might be a good reason to switch later, but adding a comment and having it immediately turn into a ghost feels weird to me in the absence of a compelling reason to do that.

In D12492#125570, @epriestley wrote:

Yeah, it will show up in both places with either approach.

With this approach, it will show up in the "submitted" colors when looking at the diff it was added to, and the "ghostly" colors when looking at the older diff.

With the other approach, it would show up with "ghostly" colors when looking at the diff it was added to, and "submitted" colors when looking at the older diff.

There might be a good reason to switch later, but adding a comment and having it immediately turn into a ghost feels weird to me in the absence of a compelling reason to do that.

Oh, gotchya.

Closed by commit rPc596c358db11: Make "Reply" on ghost inlines work correctly (or, at least, consistently) (authored by epriestley, committed by epriestley). · Explain WhyApr 21 2015, 6:07 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Path

Size

src/

infrastructure/

diff/

PhabricatorInlineCommentController.php

33 lines

Diff 29987

View Options

src/infrastructure/diff/PhabricatorInlineCommentController.php

Make "Reply" on ghost inlines work correctly (or, at least, consistently)ClosedPublicActions