Hi Griffin,

Thanks for the e-mail. Comments inline.

On Fri, Aug 30, 2013 at 11:03 AM, Griffin Myers

> I'm a very new RB user and have been tasked with evaluating it to see if
> it is a tool that we want to roll out for the development team at my
> workplace.  As such, I've spent the last few days experimenting and trying
> to think of the concerns that other team members may have.
> One item I immediately suspected as an annoyance for some team members was
> the fact that (often lengthy) review entries will always be expanded,
> particularly when they have already been viewed and there are no new
> comments.  As I dug in, I discovered the new feature from UCOSP 2010 by
> Kevin Quinn related to Collapsible Reviews (
> http://www.reviewboard.org/news/2010/12/11/ucosp-2010-project-screencasts/).
> At first I was convinced that this feature did not actually work or had
> been removed, because I was not seeing reviews collapse as I anticipated.
> It wasn't until I checked out the actual code in reviewboard/views.py that
> it made sense.  Here is my interpretation of the code and I'd like to see
> if someone can confirm this:
> 1) All reviews since the last update to the review request (or since
> initial creation if there are no updates) will *always* be *expanded 
> *regardless
> of whether they have already been viewed.  Furthermore, all reviews prior
> to the last update will be *collapsed *unless #2 applies.
> 2) Reviews prior to the last update will be *expanded *if a comment/reply
> has been added since the review request was last viewed.  On a subsequent
> refresh of the request, these reviews will again be *collapsed *unless
> additional comments have been added.

That's correct.

The sticking point for me was the condition on being prior to the last
> update which triggers the ability for something to be collapsible.  I was
> working with a sample request where I had entered several reviews, but had
> not updated the request, and was confounded by the fact that none of the
> old reviews were auto-collapsing.
> So, if my understanding of this functionality is correct, then I will
> raise the issue of why does it behave this way?  Why make collapsiblity
> conditioned on an update?  Since I'm new to RB, I feel like I must be
> missing something conceptual about why you wouldn't want to make all viewed
> reviews (without new comments/replies) auto collapse?  I made the following
> simple replacement to line 444 and it seems to achieve the functionality I
> am speaking of:
>     if last_visited and review.timestamp < last_visited
The idea is that the current discussion(s) center around the last update,
and there may be many such discussions. By collapsing anything since your
last visit, you potentially collapse things that the user has not yet seen.
Say, for example, I'm the developer (or even another reviewer) and I view
the review request. There are two reviews. The first one is lengthy, and
I'm reading through it, closing issues or maybe replying to the discussion.
Right now, replying to the discussion causes the page to reload (being
changed in the next version). That would result in reviews I have not yet
read being collapsed. I may not even notice.

Another situation is that I see a pop-up notification saying that someone
else has replied, perhaps to something I had said earlier. So I click the
Update Page link, which reloads the page. The review where the discussion
took place will be expanded, but again, anything I had not yet actually
read but has not had activity since my reload will be collapsed.

It's not perfect, though, because a second reload could collapse the
reviews again. We're, I guess, pushing back the problem a reload.

One might argue that I could just expand those reviews, but it's harder to
know what I've seen and what I haven't, particularly on longer changes.

> Would a change like this, perhaps selectable as a user config option, be
> something that would be considered for inclusion in RB?  I'd be happy to
> open a feature request ticket.  Additionally are there any other
> improvements to collapsible reviews in the pipeline?  Kevin Quinn's video
> mentions cookie based tracking of expanded/collapsed as a item for future
> work.

Given the above, we won't be changing the logic to collapse since the last
page visit at this point. This logic was discussed when this feature went
in, and we decided against it.

Cookie-based expand/collapse was discussed, but doesn't work well in
practice. That's a lot of state to store, and we don't want to litter the
browser with cookies. It's also a lot of state to store in the database,
which then has to be cleaned up, or left there to take up space. We'd then
have to decide whether that or the auto-expand/collapse logic takes
precedence, and when. Not really worth it, in my opinion.

The next major version of Review Board reloads pages less often. We do more
dynamic updating. If I reply to a review, and publish, the page won't
reload. That helps a lot. It still doesn't solve the other cases, though.
Someday, we'll be doing more intelligent updating of the page. If we never
have to trigger an actual reload, we can start being smarter about
expand/collapse logic.

I can also see us being smarter about whether you've read a review, based
on scroll position, length of time a review is on the screen, and
interactivity with the review.

I will say though that after having done thousands of reviews through
Review Board, many of which were long, I haven't hit many issues with the
current logic. I think in practice, it works alright. Not perfect, but well
enough in most cases. There's definitely some room for improvement in the
case of large reviews, but that has more to do with showing where
discussions take place. And that brings us to the next topic...

> On a related, but somewhat orthogonal, note, another feature I'd like to
> see is an improved highlighting of what has changed and/or what is new when
> viewing a review request.  Now, with the current collapsibility logic, I
> may enter a request to find that a prior review has been expanded which
> indicates that a new comment has been added.  This review may be quite
> lengthy and span several screens.  As best as I can tell, I now have to
> scroll through this review manually looking at the timestamps of all
> comments to determine which are indeed new.  If these new comments could be
> highlighted, starred, or somehow otherwise made more obvious then it would
> make this a much quicker process.  It might even be nice to have keyboard
> shortcuts here to navigate between new comments, similar to what is
> available on the reviewing page.  Again I'd be happy to open a feature
> request ticket if necessary.

All this is on my personal todo list. It bothers me, too, and I think
anybody who has done massive reviews has had the "where did the comment
go?" problem.

The upcoming version of RB has had a full JavaScript rewrite, with the end
goal of making it easier to do intelligent things like this. While that
version will not be solving this problem, we'll be closer to being able to
solve it. I have some thoughts on it, but I also have a couple dozen other
things I have to do before I can prioritize that.

We might be able to make it a student project, though. Of course, patches
are also welcome, so long as we discuss the design before-hand (best done
on reviewboard-...@googlegroups.com).



Want to help the Review Board project? Donate today at 
Happy user? Let us know at http://www.reviewboard.org/users/
To unsubscribe from this group, send email to 
For more options, visit this group at 
You received this message because you are subscribed to the Google Groups 
"reviewboard" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to reviewboard+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Reply via email to