Page MenuHomePhabricator

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

Authored by epriestley on Apr 20 2015, 10:12 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 27, 7:15 AM
Unknown Object (File)
Sun, Nov 27, 7:15 AM
Unknown Object (File)
Sun, Nov 27, 7:15 AM
Unknown Object (File)
Tue, Nov 15, 8:45 AM
Unknown Object (File)
Fri, Nov 11, 6:43 PM
Unknown Object (File)
Thu, Nov 10, 10:32 AM
Unknown Object (File)
Wed, Nov 9, 10:53 AM
Unknown Object (File)
Wed, Nov 9, 10:53 AM
Tokens
"Like" token, awarded by lucasmarshall."Like" token, awarded by sectioneight."Like" token, awarded by joshuaspence."Like" token, awarded by johnny-bit."Like" token, awarded by anton.vladimirov.

Details

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 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.

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

ghost-v1.jpg (688×1 px, 112 KB)

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.

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 edited edge metadata.
This revision is now accepted and ready to land.Apr 21 2015, 5:25 PM
This revision was automatically updated to reflect the committed changes.