Port comments through time and space in the common/best case
ClosedPublic
Actions

Authored by epriestley on Apr 20 2015, 10:12 PM.

Details

Reviewers

btrahan
chad

Maniphest Tasks

T7447: Bring inline comments forward across revision updates

Commits

Restricted Diffusion Commit
rPb2d280ff5142: Port comments through time and space in the common/best case

Summary

Ref T7447. This ports comments forward and backward in the best case:

The old comment is on a changeset with the same filename.
The old and new files are pretty much the same, line-for-line.

This will fail to port a lot of comments around and probably port a lot of comments into goofy places. We can see how bad it is in practice.

Errata:

Design is me cobbling something together, probably worth tweaking.
"Old Comment" should, at a minimum, say "Newer Comment" sometimes, or we should come up with some better name for this stuff.

Test Plan

Screen_Shot_2015-04-20_at_3.05.43_PM.png (278×819 px, 27 KB)

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

epriestley updated this revision to Diff 29971.Apr 20 2015, 10:12 PM

epriestley retitled this revision from to Port comments through time and space in the common/best case.

epriestley updated this object.

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

epriestley added reviewers: btrahan, chad.

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

Herald added subscribers: epriestley, yelirekim. · View Herald TranscriptApr 20 2015, 10:12 PM

anton.vladimirov awarded a token.Apr 20 2015, 11:33 PM

Wonder if we can shrink old comments down to a single line, with an option to expand?

In D12484#125423, @chad wrote:

Wonder if we can shrink old comments down to a single line, with an option to expand?

This is great idea.

johnny-bit awarded a token.Apr 21 2015, 7:44 AM

joshuaspence awarded a token.Apr 21 2015, 8:01 AM

I'd like to see a stronger need for comment shrinking before pursuing it. It seems like it's probably worse on the balance for every revision on this install, for example. If the major use cases are the extreme/crazy cases and we want to work to improve the experience of reviewing a 17,000 line diff with 400 comments, alternatives like "hide all comments" might be better.

Technically, it won't do a great job in a lot of cases (e.g., if the comment is youself or a table or a block of code); some of them can be improved but this isn't trivial or entirely straightforward (we already do a sort of mediocre job at the easier task of reducing a comment to a single paragraph in Feed).

Do you think the auto-shrinking is likely to be useful even on this install? Or what sort of situation are you imagining the full-sized comments being problematic in?

btrahan accepted this revision.Apr 21 2015, 5:25 PM

btrahan edited edge metadata.

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

Closed by commit rPb2d280ff5142: Port comments through time and space in the common/best case (authored by epriestley, committed by epriestley). · Explain WhyApr 21 2015, 6:07 PM

This revision was automatically updated to reflect the committed changes.

sectioneight awarded a token.Apr 24 2015, 8:56 PM

• lucasmarshall awarded a token.Apr 24 2015, 9:02 PM