[PATCH] changeset: improve the comment count information display

Tue Mar 1 12:36:54 UTC 2016

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"?

Yes. I am assuming that this would not make it in unless the other
changes made it in.

>> 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?

I think that perhaps the number of status changes could somehow convey
how "controversial" a commit has been...
That being said, the main reason why I added this count was because
the status changes are shown as "comments" on the UI. I wanted one to
be able to count the comment "boxes" and come up with the same numbers
that this patch is showing.

>> - 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.)

I could split these changes but in my mind they were all related to
changing the way comments are shown. I can split them if you think
that is best though.

>
>> 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.

This is also adding a space, which I do not think you can do directly
on the template, right?

> But perhaps better: show a 'no comments' or 'no other comments' if there are
> no other comments?

I considered that. My first shot at this change always showed all the
info as follows:

W status changes, X comments (Y inline, Z general)

It did this even if you had only one type of comments or no comments
or status changes. I felt that it took too much space. I think it is
OK to take a lot of space when there is some info (i.e. some comments)
but not when there are no comments. That is why I tried to only show
as little info as possible.

That being said, I could show a "No comments" in case there are no
comments _nor_ status changes, but keep the proposed behavior
otherwise (i.e. do not show status change counts if there are only
changes, and vice-versa and so on). What do you think?

>> +        %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