[PATCH] changeset: improve the comment count information display

[Mads Kiilerich]

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.

/Mads