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

[Jan Heylen]

On Wed, Jan 6, 2016 at 6:56 AM, Thomas De Schampheleire
<patrickdepinguin at gmail.com> wrote:
>
> On Jan 5, 2016 8:36 PM, "Jan Heylen" <heyleke at gmail.com> wrote:
>>
>> 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
>>
>
> JQueryUi  seems overkill to me since collapsing file diffs could be
> accomplished very easily with standard js/jquery, right? Attach a click
> handler to all collapse links and use 'this' (possibly one of its parents)
> to get hold of the diff that needs to be collapsed.
IMHO the overkill is doing even basic ui stuff ourselves, while there
are quite good and awesome frameworks out there.

Jan
>
> /Thomas