[PATCH 0 of 7 v3] diff: add collapse to simple diff and make it configurable

[Mads Kiilerich]

On 01/02/2016 01:15 PM, Jan Heylen wrote:
> Hi,
>
> third version of this patchset, with some extra fixes and partly refactoring of the applicable code (diff_block)
> more work to get both templates uniform are WIP.

Thanks. Much better!

As you can see, I took some of your changes and reworked them to my 
liking instead of just commenting. I hope that is ok. Some of the more 
"controversial" changes have been left out, but they should be easier to 
do now and be easier to present cleanly.

General comments:
Please try to
* put one change in each changeset
* make good commit messages that explain why the change is made and what 
is achieved
* order them by "risk" and "likely to be taking without more discussion" 
- refactorings first, then features

> --- a/kallithea/templates/changeset/diff_block.html	Sat Jan 02 11:53:38 2016 +0100
> +++ b/kallithea/templates/changeset/diff_block.html	Sat Jan 02 11:54:11 2016 +0100
> @@ -31,6 +31,12 @@
>       <span target="${'diff-container-%s' % (id(change))}" class="diff-collapse-button">↑ ${_('Collapse Diff')} ↑</span>
>   </div>
>   <div class="diff-container" id="${'diff-container-%s' % (id(change))}">
> +    <span style="float:right;margin-bottom:3px;margin-right:30px">
> +        <label>
> +            ${_('Show inline comments')}
> +            ${h.checkbox('',checked="checked",class_="show-inline-comments",id_for="diff-container-%s" % (id(change)))}
> +        </label>
> +    </span>
>   %for FID,(cs1, cs2, change, path, diff, stats) in change.iteritems():
>       <div id="${FID}_target" style="clear:both;margin-top:25px"></div>
>       <div id="${FID}" class="diffblock  margined comm">

id() will essentially just take the memory address of the variable. That 
is not good to use here. Other objects might get the same address when 
this goes out of scope. Actually, it seems like you in this case only 
need a globally unique identifier?

I don't know how much more convincing the commit messages can be made, 
but I'm very reluctant to introduce code for hiding the diff after 
fetching it. I think it must be lazily loaded when the diff actually is 
shown.

I also see more use case for collapsing the individual file diffs (after 
they have been reviewed so the reviewer can focus on the rest) than for 
collapsing everything (leaving almost nothing on the page, only to guide 
the reader somewhere else).

/Mads