[PATCH] changeset: improve the comment count information display

[Angel Ezquerra]

On Mon, Feb 29, 2016 at 1:27 PM, Thomas De Schampheleire
<patrickdepinguin at gmail.com> wrote:
> 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.

I'm not quite sure, from your comment, whether you think I should
change the patch to put the logic on a separate function or not.
Please advise! :-)

Cheers,

Angel