[PATCH] changeset: improve the comment count information display

[Thomas De Schampheleire]

On Mon, Feb 29, 2016 at 12:55 PM, Mads Kiilerich <mads at kiilerich.com> wrote:
> On 02/27/2016 12:24 PM, Angel Ezquerra wrote:
>>
>> # HG changeset patch
>> # User Angel Ezquerra <angel.ezquerra at gmail.com>
>> # Date 1456572240 -3600
>> #      Sat Feb 27 12:24:00 2016 +0100
>> # Branch stable
>> # Node ID 58ee8a76b6a8ae0a69d3f50b2e3e3357780598c9
>> # Parent  a13394a91f352a982afcf0b9141ece5537caacb1
>> changeset: improve the comment count information display
>>
>> Some recent revisions have changed the way comments are counted.
>
>
> That is a reference to "summary, changelog, compare: make the comments
> counts more accurate"?
>
>> We no longer
>> count "status change only comments" as comments on the changelog, etc.
>> This
>> change tries to make the way we count comments on the changest details
>> page
>> match the way we count them everywhere else. At the same time this tries
>> to
>>
>> Change the way the comment count information is displayed as follows:
>>
>> - Separately count the number of status changes and the number of comment
>> changes (to match how comments are counted everywhere else)
>
>
> By changing how it is displayed, it seems like it also kind of redefines the
> data model from "some of the comments also change status" to "some status
> changes also have comments".
>
> That seems to raise and answer the question of whether such status change
> comments should be counted twice.
>
> I agree that status changes without comments shouldn't count as comments
> (and probably also not send notifications as wide as it does). But is the
> number of status changes without comments ever relevant?
>
>> - Only show the status and comment counts when needed (i.e. show nothing
>> when
>> there are no comments or status changes)
>> - Visually separate the counts from the "First comment" link by adding a
>> "|"
>> separator.
>
>
> That seems like separate changes? (We are not so strict about that ... but
> splitting things up makes it much easier to review changes and get them in.)
>
>
>> diff --git a/kallithea/templates/changeset/changeset.html
>> b/kallithea/templates/changeset/changeset.html
>> --- a/kallithea/templates/changeset/changeset.html
>> +++ b/kallithea/templates/changeset/changeset.html
>> @@ -72,7 +72,7 @@
>>                     ${c.context_url(request.GET)}
>>                   </div>
>>                   <div class="comments-number"
>> style="float:right;padding-right:5px">
>> -                    ${comment.comment_count(c.inline_cnt,
>> len(c.comments))}
>> +                    ${comment.comment_count(c.inline_cnt, c.comments)}
>>                   </div>
>>               </div>
>>           </div>
>> diff --git a/kallithea/templates/changeset/changeset_file_comment.html
>> b/kallithea/templates/changeset/changeset_file_comment.html
>> --- a/kallithea/templates/changeset/changeset_file_comment.html
>> +++ b/kallithea/templates/changeset/changeset_file_comment.html
>> @@ -100,13 +100,33 @@
>>   </%def>
>>     -## show comment count as "x comments (y inline, z general)"
>> -<%def name="comment_count(inline_cnt, general_cnt)">
>> -    ${'%s (%s, %s)' % (
>> -        ungettext("%d comment", "%d comments", inline_cnt + general_cnt)
>> % (inline_cnt + general_cnt),
>> -        ungettext("%d inline", "%d inline", inline_cnt) % inline_cnt,
>> -        ungettext("%d general", "%d general", general_cnt) % general_cnt
>> -    )}
>> +## show comment count as "w comments (x inline, y general), z status
>> changes"
>> +<%def name="comment_count(inline_cnt, comments)">
>> +    <%
>> +    general_cnt = len([cm for cm in comments if cm.text])
>> +    status_cnt = len([cm for cm in comments if cm.status_change])
>> +    comment_cnt = inline_cnt + general_cnt
>> +    %>
>> +    %if status_cnt:
>> +        ${ungettext("%d status change", "%d status changes", status_cnt)
>> % status_cnt}
>> +        %if comment_cnt:
>> +            ${", "}
>
>
> I doubt there is any advantage of doing it this way instead of just putting
> a bare ',' in the template.
>
> But perhaps better: show a 'no comments' or 'no other comments' if there are
> no other comments?
>
>> +        %endif
>> +    %endif
>> +    %if inline_cnt > 0 and general_cnt > 0:
>> +        ${'%s (%s, %s)' % (
>> +            ungettext("%d comment", "%d comments", inline_cnt +
>> general_cnt) % (inline_cnt + general_cnt),
>> +            ungettext("%d inline", "%d inline", inline_cnt) % inline_cnt,
>> +            ungettext("%d general", "%d general", general_cnt) %
>> general_cnt,
>> +        )}
>> +    %elif inline_cnt > 0:
>> +        ${ungettext("%d inline comment", "%d inline comments",
>> inline_cnt) % inline_cnt,}
>> +    %elif general_cnt > 0:
>> +        ${ungettext("%d general comment", "%d general comments",
>> general_cnt) % general_cnt,}
>> +    %endif
>> +    %if status_cnt + comment_cnt:
>> +        |
>> +    %endif
>
>
> Thomas: you implemented this helper function. Now it is growing a bit more
> complex - please review and approve these changes ... and comment on the
> general thoughts on when to count what.

Yes, I was planning to test and review these changes.

Since I wrote these changes, I am more and more convinced that the
amount of logic in templates should be as limited as possible, and
instead the model or controller should provide the information. Right
now this is possible by creating some function/property; but note that
it may become trickier when we actually want to properly count inline
comments which are added via ajax.

/Thomas