Skip to content

Commit 27d6b9a

Browse files
committed
Sticky Comments: Move sorting to builder
Using a sort value for sticky comments was leading to a lot of pain when dealing with rendering edge cases (specifically hiding children). Using this approach we more directly special case the sticky comment and explicitly do not render its children when viewing top level comments, which is much cleaner. P.S. This is almost entirely @bsimpson63's code and idea, with some extra docs. Thank you Brian!
1 parent 148731a commit 27d6b9a

File tree

3 files changed

+33
-17
lines changed

3 files changed

+33
-17
lines changed

r2/r2/lib/comment_tree.py

-4
Original file line numberDiff line numberDiff line change
@@ -42,10 +42,6 @@ def parent_comments_key(link_id):
4242

4343

4444
def _get_sort_value(comment, sort, link, children=None):
45-
# Sticky comments get a very high score for all sorts.
46-
if link and comment._id == link.sticky_comment_id:
47-
return 2 ** 31 - 1
48-
4945
if sort == "_date":
5046
return epoch_seconds(comment._date)
5147
if sort == '_qa':

r2/r2/models/builder.py

+28-7
Original file line numberDiff line numberDiff line change
@@ -853,6 +853,7 @@ def _get_comments(self):
853853
dont_collapse = []
854854
candidates = []
855855
offset_depth = 0
856+
items = []
856857

857858
if self.children:
858859
# requested specific child comments
@@ -886,14 +887,39 @@ def _get_comments(self):
886887
offset_depth = depth.get(path[-1], 0)
887888

888889
else:
889-
# full tree requested, start with the top level comments
890+
# full tree requested, add all top level comments as candidates to
891+
# be considered for display
890892
top_level_comments = cid_tree.get(None, ())
893+
894+
# If we have a sticky comment and we're viewing top level comments,
895+
# we shove the sticky comment in the top and remove it from the
896+
# remainder of the top level comments list so it's displayed first.
897+
if self.link.sticky_comment_id:
898+
try:
899+
# Remove the sticky comment from the candidates list so it
900+
# isn't displayed twice
901+
top_level_comments.remove(self.link.sticky_comment_id)
902+
except ValueError:
903+
g.log.warning("Non-top-level sticky comment detected on "
904+
"link %r.", self.link)
905+
pass
906+
else:
907+
# Make the sticky comment the first item, meaning it is the
908+
# first comment displayed regardless of sort score.
909+
items.append(self.link.sticky_comment_id)
910+
911+
# Don't add its children as candidates - we don't want to
912+
# show any replies to the sticky comment when viewing a top
913+
# level list. This will still render the "load more
914+
# comments" link for viewing its children. When viewing
915+
# this comment as non-top-level (for example a permalink)
916+
# sticky comment children will render normally.
917+
891918
self.update_candidates(candidates, sorter, top_level_comments)
892919

893920
timer.intermediate("pick_candidates")
894921

895922
# choose which comments to show
896-
items = []
897923
while (self.num is None or len(items) < self.num) and candidates:
898924
sort_val, comment_id = heapq.heappop(candidates)
899925
if comment_id not in cids:
@@ -994,11 +1020,6 @@ def _make_wrapped_tree(self):
9941020

9951021
parent = wrapped_by_id.get(comment.parent_id)
9961022

997-
# Children of sticky comments are hidden by default to avoid top
998-
# comment hijacking or conversation derailing.
999-
if parent and self.link.sticky_comment_id == parent._id:
1000-
comment.hidden = True
1001-
10021023
if qa_sort_hiding:
10031024
# In the Q&A sort type, we want to collapse all comments other
10041025
# than those that are:

r2/r2/models/link.py

+5-6
Original file line numberDiff line numberDiff line change
@@ -1012,10 +1012,6 @@ def set_sticky_comment(self, comment, set_by=None):
10121012
self.sticky_comment_id = comment._id
10131013
self._commit()
10141014

1015-
# Update votes on this comment to reset its sort values (as sticky
1016-
# maxes them out in comment_tree.py)
1017-
update_comment_votes(comment)
1018-
10191015
if set_by:
10201016
ModAction.create(
10211017
self.subreddit_slow,
@@ -1047,8 +1043,11 @@ def remove_sticky_comment(self, comment=None, set_by=None):
10471043
self.sticky_comment_id = None
10481044
self._commit()
10491045

1050-
# Update votes on this comment to reset its sort values (as sticky
1051-
# maxes them out in comment_tree.py)
1046+
# LEGACY: This is to deal with the old way that sticky comments used
1047+
# to alter sort values. This is only still here so that when a comment
1048+
# gets unstickied it will have its sort value corrected. We can remove
1049+
# this line after 2016-02-01 or so when unstickying comments from
1050+
# before this update will be rare.
10521051
update_comment_votes(prev_sticky_comment)
10531052

10541053
if set_by:

0 commit comments

Comments
 (0)