Page MenuHomePhabricator

Sort inline comments by id in case of ties
ClosedPublic

Authored by sophiebits on Apr 4 2014, 1:08 AM.

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Commits
Restricted Diffusion Commit
rPf9a92c7631aa: Sort inline comments by id in case of ties
Summary

This ensures that two comments by the same author on the same line are sorted properly.

Test Plan

Before this patch, made two comments that appeared in the wrong order. With this patch, they sort correctly.

Diff Detail

Repository
rP Phabricator
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

sophiebits updated this revision to Diff 20626.Apr 4 2014, 1:08 AM
sophiebits retitled this revision from to Sort inline comments by id in case of ties.
sophiebits updated this object.
sophiebits edited the test plan for this revision. (Show Details)
sophiebits added a reviewer: epriestley.
Herald added subscribers: Korvin, epriestley.
sophiebits added inline comments.Apr 4 2014, 1:08 AM
src/applications/differential/storage/DifferentialTransactionComment.php
48

Are these casts unnecessary now?

epriestley edited edge metadata.Apr 4 2014, 1:36 AM

I think the inlines passed in here always share the same author, so sorting by length doesn't actually make a ton of sense. What do you think about just <num, id>?

(The casts are unnecessary with sprintf().)

(Even if, in the future, inlines by different authors are passed in here, sorting by id seems better in all cases than len to me, I think?)

Well, you can have overlapping comment ranges, right? So I could have four comments like:

num  len  id
10   0    1001
10   0    1003
10   5    1002
10   5    1004

and they should be displayed in that order.

sophiebits updated this revision to Diff 20628.Apr 4 2014, 1:40 AM
sophiebits edited edge metadata.

Remove casts

epriestley accepted this revision.Apr 4 2014, 1:41 AM
epriestley edited edge metadata.

mkay

This revision is now accepted and ready to land.Apr 4 2014, 1:41 AM
epriestley closed this revision.Apr 4 2014, 1:42 AM
epriestley updated this revision to Diff 20629.

Closed by commit rPf9a92c7631aa (authored by @spicyj, committed by @epriestley).