Summary

Cache diff fragments by URL and preserve diff options while paging

Review Request #2652 — Created Oct. 10, 2011 and discarded Sept. 29, 2012, 10:16 a.m.

Information

Owner

bhollis

Repository

Review Board

Branch

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

Thess changes take all the diff viewing options (syntax highlighting, expand/collapse) and embeds them into the URL of the diff fragment (instead of relying on cookies). This accomplishes two things - one, we now preserve diff preference as we page around the diff, and two, individual diff fragments can now be entirely cached by their URL using the @cache_page directive.

On GitHub as individual commits:

https://github.com/bhollis/reviewboard/commit/f5268174871ab5f5f30d560e592d2e53570d5780
https://github.com/bhollis/reviewboard/commit/58a2c8c6dc22b49e56e2e6cd4dc1d2367c412a93
https://github.com/bhollis/reviewboard/commit/4fc6775b62aa66b391dc9e95789b0f3891226d93

Testing Done

Used on our own RB instance

The general concept seems pretty good, and there's certainly a lot of benefits we'd get from this, but the use of cookies isn't arbitrary. We want to remember if a user last looked at some diff expanded or collapsed. Whether that's done through cookies or stored info in account preferences doesn't matter to me, but I believe people today are enabling expanded diffs and choosing to stick with that, which the cookies today give us.

I could be wrong, and maybe it's worth revisiting how expand/collapse even work today, but I don't want to give up the behavior without being sure.

bhollis Oct. 11, 2011, 3:37 a.m.

That's an interesting position - I thought it was just a bug that diffs stayed expanded. Whenever I'm using the diff view I usually want to see just the changed bits, then expand as-needed. I could see retaining the cookie if you thought people really wanted it that way, but still generate unique URLs instead of changing content based on the cookie.

bhollis Oct. 12, 2011, 10:11 a.m.

What would you like me to do so this patch could be accepted?

chipx86 Nov. 7, 2011, 11:10 a.m.

Sorry Ben, this slipped past me.

I think we need more information. I'm not opposed to changing this behavior, but I don't really want a lot of fallout. So maybe a discussion on the mailing list, see if admins can ask their users?

If we can determine the URL partially based on the cookie, in order to keep that behavior for now, I'm satisfied.

Status:: Discarded