On Mon, Feb 29, 2016 at 1:27 PM, Thomas De Schampheleire <[email protected]> wrote: > On Mon, Feb 29, 2016 at 12:55 PM, Mads Kiilerich <[email protected]> wrote: >> On 02/27/2016 12:24 PM, Angel Ezquerra wrote: >>> >>> # HG changeset patch >>> # User Angel Ezquerra <[email protected]> >>> # 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 _______________________________________________ kallithea-general mailing list [email protected] http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
