Page MenuHomePhabricator

Sort inline comments by id in case of ties
ClosedPublic

Authored by sophiebits on Apr 4 2014, 1:08 AM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 14 2024, 7:58 PM
Unknown Object (File)
Feb 4 2024, 7:21 PM
Unknown Object (File)
Feb 4 2024, 7:21 PM
Unknown Object (File)
Feb 4 2024, 7:21 PM
Unknown Object (File)
Feb 4 2024, 7:21 PM
Unknown Object (File)
Feb 4 2024, 7:21 PM
Unknown Object (File)
Feb 4 2024, 3:19 AM
Unknown Object (File)
Jan 30 2024, 4:30 AM
Subscribers

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
Branch
sort-inline
Lint
Lint Passed
Unit
Tests Passed

Event Timeline

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.
src/applications/differential/storage/DifferentialTransactionComment.php
49

Are these casts unnecessary now?

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 edited edge metadata.

Remove casts

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

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