wrap author information for ticket change comments in a span to make them stylable

[dkg@…]

It would be nice to be able to to adjust the style of the author of a ticket change independently of the rest of the change header. Trac can make this easy to do by wrapping the author's identity in a <span class="author"> tag.

I think the following change does this without producing any bad side effects:

diff --git a/trac/web/chrome.py b/trac/web/chrome.py
index 3aed33d..4ec9f7d 100644
--- a/trac/web/chrome.py
+++ b/trac/web/chrome.py
@@ -1058,9 +1058,9 @@ class Chrome(Component):
         return sep.join(all_cc)
 
     def authorinfo(self, req, author, email_map=None):
-        return self.format_author(req,
+        return tag.span(self.format_author(req,
                                   email_map and '@' not in author and
-                                  email_map.get(author) or author)
+                                  email_map.get(author) or author), class="author")
 
     def get_email_map(self):
         """Get the email addresses of all known users."""

this allows someone who wants the author names to show up (for example) in bold to add the following to their local css file:

h3.change span.author { font-weight: bold; }

[Ryan J Ollos]

It sounds fine to me. I'd suggest that the class be trac-comment-author and I think we want to modify the template rather than trac.web.chrome:authorinfo.

trac/ticket/templates/ticket_change.html

@@ -50 +50 @@
     </span>
     <py:choose>
       <py:when test="'date' in change">
-        <i18n:msg params="date, author">Changed ${pretty_dateinfo(change.date)} by ${authorinfo(change.author)}</i18n:msg>
+        <i18n:msg params="date, author">Changed ${pretty_dateinfo(change.date)} by <span class="trac-comment-author">${authorinfo(change.author)}</span></i18n:msg>
       </py:when>
       <py:otherwise>
-        <i18n:msg params="author">Changed by ${authorinfo(change.author)}</i18n:msg>
+        <i18n:msg params="author">Changed by <span class="trac-comment-author">${authorinfo(change.author)}</span></i18n:msg>
       </py:otherwise>
     </py:choose>
     <span py:if="preview or show_editor" class="trac-loading"/>

Does anyone see problems with the change?

[Ryan J Ollos]

The reason I suggest not modifying authorinfo is that the function is also used to output a username in several other places:

$ grep -R "\bauthorinfo\b" trac --exclude-dir=.git --exclude=.pyc --color=always --exclude-dir=.svn
trac/ticket/templates/ticket_box.html:      v_reporter = reporter_link if defined('reporter_link') else authorinfo(ticket.reporter);
trac/ticket/templates/ticket_box.html:      v_owner = (owner_link if defined('owner_link') else authorinfo(ticket.owner)) if ticket.owner else ''
trac/ticket/templates/ticket_box.html:        <i18n:msg params="author">(last modified by ${authorinfo(description_change.author)})</i18n:msg>
trac/ticket/templates/query_results.html:                 groupname = authorinfo(groupname) if query.group in ['owner', 'reporter'] else (groupname or _('None'));
trac/ticket/templates/query_results.html:                      <py:when test="name == 'reporter'">${authorinfo(value)}</py:when>
trac/ticket/templates/query_results.html:                      <py:when test="name == 'owner' and value">${authorinfo(value)}</py:when>
trac/ticket/templates/ticket_change.html:        <i18n:msg params="date, author">Changed ${pretty_dateinfo(change.date)} by ${authorinfo(change.author)}</i18n:msg>
trac/ticket/templates/ticket_change.html:        <i18n:msg params="author">Changed by ${authorinfo(change.author)}</i18n:msg>
trac/ticket/templates/ticket_change.html:        by ${authorinfo(change.comment_history[comment_version].author)}
trac/ticket/templates/ticket_change.html:        by ${authorinfo(change.comment_history[comment_version].author)}
trac/templates/attachment.html:                (added by ${authorinfo(attachment.author)}, ${pretty_dateinfo(attachment.date)})
trac/templates/list_of_attachments.html:      added by <em>${authorinfo(attachment.author)}</em> ${pretty_dateinfo(attachment.date)}.
trac/templates/history_view.html:                           if show_ip_addresses and item.ipnr else None}">${authorinfo(item.author)}</td>
trac/templates/diff_view.html:          <py:otherwise>${authorinfo(change.author)}
trac/wiki/templates/wiki_view.html:               Version $page.version (modified by ${authorinfo(page.author)}, ${pretty_dateinfo(page.time)})
trac/web/chrome.py:            'authorinfo': partial(self.authorinfo, req),
trac/web/chrome.py:    def authorinfo(self, req, author, email_map=None):
trac/versioncontrol/templates/browser.html:                       author = authorinfo(file.changeset.author);
trac/versioncontrol/templates/changeset.html:          <dd class="author">${authorinfo(changeset.author)}</dd>

However, after studying the code, it really seems better to wrap the author in a span tag wherever it is presented inline. This would basically be all of the uses output by grep, except cases that it is used in tables, such as query_results.html. In a table such as that output by query_results, you could target the owner using the selector td.owner.

Proposed changes can be found in log:rjollos.git:t11145.

[Jun Omae]

However, after studying the code, it really seems better to wrap the author in a span tag wherever it is presented inline.

I feel the name of inline argument is ambiguous. I think the text of author even without span is inline. I'd like to be decorate.

This would basically be all of the uses output by grep, except cases that it is used in tables, such as query_results.html. In a table such as that output by query_results, you could target the owner using the selector td.owner.

I think that we should wrap all with <span class="trac-author">. It would be able to inject extra information of the author, e.g. avatar image, from plugins using javascript. The plugin or Trac user should determine a need for each element of author to decorate using stylesheet and inject.

[Ryan J Ollos]

Replying to jomae:

I feel the name of inline argument is ambiguous. I think the text of author even without span is inline. I'd like to be decorate.

Okay, do we even need the argument if we implement the suggestion in your second comment?

I think that we should wrap all with <span class="trac-author">. It would be able to inject extra information of the author, e.g. avatar image, from plugins using javascript. The plugin or Trac user should determine a need for each element of author to decorate using stylesheet and inject.

I like it. That seems like a good use case.

I've made the suggested changes in log:rjollos.git:t11145.2.

[Jun Omae]

I've made the suggested changes in log:rjollos.git:t11145.2.

Okay, good. Thanks. It didn't need a name instead of inline argument ;-)

Ah, I found use of <span class="author"> with format_author. Should we replace with <span class="trac-author">?

$ grep -wr --include='*.html' format_author trac tracopt
trac/wiki/templates/wiki_view.html:                   version=page.version, author=format_author(page.author), comment=page.comment) or
trac/wiki/templates/wiki_view.html:                   version=page.version, author=format_author(page.author)))
trac/templates/progress_bar_grouped.html:           py:with="obfus_name = format_author(group.name)"
trac/templates/progress_bar_grouped.html:                                                    and group.name and group.name != format_author(group.name))];
trac/timeline/templates/timeline.html:                  by <span class="author">${format_author(event.author)}</span>
trac/search/templates/search.html:                <py:if test="result.author"><span class="author" i18n:msg="author">By ${format_author(result.author)}</span> &mdash;</py:if>

[Ryan J Ollos]

Replying to jomae:

Ah, I found use of <span class="author"> with format_author. Should we replace with <span class="trac-author">?

Yeah, it seems like we should address that. Does this change make sense?: [5e102608/rjollos.git]. The resulting change to the pot file is:

trac/locale/messages.pot

@@ -8 +8 @@
 msgstr ""
 "Project-Id-Version: Trac 1.1.2\n"
 "Report-Msgid-Bugs-To: trac-dev@googlegroups.com\n"
-"POT-Creation-Date: 2013-12-11 03:25-0800\n"
+"POT-Creation-Date: 2013-12-11 03:38-0800\n"
 "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
 "Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
 "Language-Team: LANGUAGE <LL@li.org>\n"
@@ -1807 +1807 @@
 
 #: trac/search/templates/search.html:56
 #, python-format
-msgid "By %(author)s"
+msgid "By %(author)s —"
 msgstr ""
 
 #: trac/search/templates/search.html:65
@@ -4625 +4625 @@
 #, python-format
 msgid ""
 "[1:%(time)s] %(title)s\n"
-"                  by [2:%(author)s]"
+"                  by %(author)s"
 msgstr ""

The dash could be kept outside the translatable string if that is better. With the dash inside the string the markup is a bit more concise.

There are also uses of authorinfo_short that might need to be wrapped in a span: branches/1.0-stable/trac/web/chrome.py@12260:1091-1097#L1088​.

The latest changes are in log:rjollos.git:t11145.3.

[Jun Omae]

Replying to rjollos:

Yeah, it seems like we should address that. Does this change make sense?: [5e102608/rjollos.git]. The resulting change to the pot file is:

#!diff...

The dash could be kept outside the translatable string if that is better. With the dash inside the string the markup is a bit more concise.

I think the translations should have no changes if possible. But, if with the dash inside the string, msgid "By %(author)s — [1:%(date)s]" is better.

trac/search/templates/search.html

@@ -52 +52 @@
             <py:for each="result in results">
               <dt><a href="${result.href}" class="searchable">${result.title}</a></dt>
               <dd class="searchable">${result.excerpt}</dd>
-              <dd>
-                <i18n:msg params="author" py:if="result.author">By ${authorinfo(result.author)} &mdash;</i18n:msg>
-                <span class="date">${result.date}</span>
-              </dd>
+              <py:choose>
+                <dd py:when="result.author" i18n:msg="author,date">
+                  By ${authorinfo(result.author)} &mdash; <span class="date">${result.date}</span>
+                </dd>
+                <dd py:otherwise=""><span class="date">${result.date}</span></dd>
+              </py:choose>
             </py:for>
           </dl>
         </div>

Also, we should replace .author with .trac-author in stylesheets.

There are also uses of authorinfo_short that might need to be wrapped in a span: branches/1.0-stable/trac/web/chrome.py@12260:1091-1097#L1088​.

Yes, we should wrap those.

[Ryan J Ollos]

I've added the changes suggested in comment:9 to log:rjollos.git:t11145.3.

[Ryan J Ollos]

[a977c05b/rjollos.git#file0] adds the styling to the span tag, but I missed removal of the em tags, which can now be found in [77810c5f/rjollos.git].

[Ryan J Ollos]

I made a few more changes to remove newlines and whitespace from translatable messages that are being changed ([84d8266e/rjollos.git]). The resulting diff of the pot file is:

trac/locale/messages.pot

@@ -8 +8 @@
 msgstr ""
 "Project-Id-Version: Trac 1.1.2\n"
 "Report-Msgid-Bugs-To: trac-dev@googlegroups.com\n"
-"POT-Creation-Date: 2013-12-15 13:31-0800\n"
+"POT-Creation-Date: 2013-12-15 15:13-0800\n"
 "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
 "Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
 "Language-Team: LANGUAGE <LL@li.org>\n"
@@ -1807 +1807 @@
 
 #: trac/search/templates/search.html:56
 #, python-format
-msgid "By %(author)s"
+msgid "By %(author)s — [1:%(date)s]"
 msgstr ""
 
-#: trac/search/templates/search.html:65
+#: trac/search/templates/search.html:67
 #: trac/ticket/templates/report_view.html:97
 #: trac/ticket/templates/report_view.html:208
 msgid "No matches found."
 msgstr ""
 
-#: trac/search/templates/search.html:69
+#: trac/search/templates/search.html:71
 msgid ""
 "[1:Note:] See [2:TracSearch]\n"
 "        for help on searching."
@@ -2313 +2313 @@
 
 #: trac/templates/list_of_attachments.html:18
 #, python-format
-msgid ""
-"[1:%(file)s][2:<U+200B>]\n"
-"       ([3:%(size)s]) -\n"
-"      added by [4:%(author)s] %(date)s."
+msgid "[1:%(file)s][2:<U+200B>] ([3:%(size)s]) - added by %(author)s %(date)s."
 msgstr ""
 
 #: trac/templates/list_of_attachments.html:28
@@ -4624 +4621 @@
 
 #: trac/timeline/templates/timeline.html:48
 #, python-format
-msgid ""
-"[1:%(time)s] %(title)s\n"
-"                  by [2:%(author)s]"
+msgid "[1:%(time)s] %(title)s by %(author)s"
 msgstr ""
 
 #: trac/timeline/templates/timeline.html:64
@@ -5965 +5960 @@
 msgid "Genshi %(error)s error while rendering template %(location)s"
 msgstr ""
 
-#: trac/web/chrome.py:1101 trac/web/chrome.py:1109
+#: trac/web/chrome.py:1102 trac/web/chrome.py:1111
 msgid "anonymous"
 msgstr ""

The latest changes can be found in log:rjollos.git:t11145.4. Do the changes to the translatable strings look okay?

[Jun Omae]

Looks good and works fine with translations! However, I think we should use Genshi comment instead HTML comment.

trac/templates/list_of_attachments.html

@@ -17 +17 @@
   <py:def function="show_one_attachment(attachment)">
     <i18n:msg params="file, size, author, date">
       <a href="${url_of(attachment.resource)}" title="View attachment">${attachment.filename
-        }</a><a href="${url_of(attachment.resource, format='raw')}" class="trac-rawlink" title="Download">&#8203;</a><!--
+        }</a><a href="${url_of(attachment.resource, format='raw')}"
+                class="trac-rawlink" title="Download">&#8203;</a><!--!
       --> (<span title="${_('%(size)s bytes', size=attachment.size)}">${pretty_size(attachment.size)
         }</span>) - added by ${authorinfo(attachment.author)} ${pretty_dateinfo(attachment.date)}.
     </i18n:msg>

[Ryan J Ollos]

Changes, including suggestion from comment:13, committed to trunk in [12340].

[Ryan J Ollos]

See also related ticket #11474.

[Ryan J Ollos]

While working on #11474 some additional refactoring were found that fit well with the other work in this ticket: log:rjollos.git:t11145.5

After these changes:

• Chrome.get_email_map is not used outside of the Chrome class.
• After the proposed changes in #11474, Chrome.show_email_addresses will only be used outside the Chrome class in trunk/trac/ticket/notification.py@12751:465#L460​. After #11474 is committed, we could simplify the API for plugins by deprecating show_email_addresses for removal from the Chrome data dictionary.

On a related note, there is some odd-looking code in the preparation of show_email_addresses. Does anyone have an idea of what type of Exceptions might be encountered here?: trunk/trac/web/chrome.py@12930:878#L875​. The code was added in [8785#file4].

There is a possible problem that became more clear after these changes. For whatever reason, the usernames are replaced with email addresses in the RSS feed when possible. However, EMAIL_VIEW permission is not checked when rendering the RSS feed. Only the show_email_addresses option is considered: trunk/trac/web/chrome.py@12930:1118#L1115​. The email addresses won't be added to the map if show_email_addresses is false. Is this a defect?

[Peter Suter]

Replying to rjollos:

On a related note, there is some odd-looking code in the preparation of show_email_addresses. Does anyone have an idea of what type of Exceptions might be encountered here?: trunk/trac/web/chrome.py@12930:878#L875​. The code was added in [8785#file4].

I was wondering about that as well. I'm not sure, but comment:1:ticket:8585 suggests maybe OperationalError: database is locked.

There is a possible problem that became more clear after these changes. For whatever reason, the usernames are replaced with email addresses in the RSS feed when possible. However, EMAIL_VIEW permission is not checked when rendering the RSS feed. Only the show_email_addresses option is considered: trunk/trac/web/chrome.py@12930:1118#L1115​. The email addresses won't be added to the map if show_email_addresses is false Is this a defect?

comment:1:ticket:6582 suggests it is intentional, but I suspect this could still be improved.

Edit: Or maybe I misunderstood. After skimming #153 superficially I think the discussion there also touched this topic (as well as moving EMAIL_VIEW to chrome, as you now did in #11474…) but I still don't see a clear reason.

[Ryan J Ollos]

Replying to psuter:

comment:1:ticket:6582 suggests it is intentional, but I suspect this could still be improved.

Edit: Or maybe I misunderstood. After skimming #153 superficially I think the discussion there also touched this topic (as well as moving EMAIL_VIEW to chrome, as you now did in #11474…) but I still don't see a clear reason.

Thanks, I'm still unsure of what the behaviour should be, but I'll revisit the issue in the future.

[Ryan J Ollos]

Changes from comment:16 committed to trunk in [12991:12992].