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

[Jan Heylen]

On Tue, Jan 5, 2016 at 5:12 PM, Mads Kiilerich <mads at kiilerich.com> wrote:
> 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!
I'm on a learning curve here :-)

>
> 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
I tried to split into small changesets, but I'll try to do even better
splitting, I tried to give enough info in the commit, but I'll try to
give even more info, I ordered them in order of "taking in risk", but
I apparently have a different judgement ;-)

>
>> --- 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?
Indeed, I thought I get a unique identifier by "id(change)", but I'm
not sure what to use in replacement?

>
> 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 agree, but as said, that is not the goal of this changeset. And
indeed, changing the default collapse behaviour upstream is not my
goal.

>
> 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).
I agree, but the current collapse/expand code isn't functional for
this at this moment, maybe we can change the div box css class
changeset-header or code-header to be collapseble or expandable, with
the use of an existing javascript thing. Would it be feasible to
include jqueryui? We could use https://jqueryui.com/accordion

>
> /Mads

Jan