Cache diff fragments by URL and preserve diff options while paging

Review Request #2652 — Created Oct. 10, 2011 and discarded


Review Board


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:
Used on our own RB instance
  1. 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.
    1. 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.
    2. What would you like me to do so this patch could be accepted?
    3. 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.
Review request changed

Status: Discarded