Edgewall Software
Modify

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#11145 closed enhancement (fixed)

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

Reported by: dkg@… Owned by: Ryan J Ollos
Priority: normal Milestone: 1.1.2
Component: ticket system Version:
Severity: minor Keywords: authorinfo styling
Cc: Branch:
Release Notes:

Author names are decorated in a span with class trac-author.

API Changes:
  • The authorinfo and authorinfo_short methods of the Chrome class in trac.web.chrome return the author wrapped in a span with class trac-author.
  • Added author_email method of the Chrome class which returns the author email from the email_map dictionary if author doesn't already look like an email address.
  • author_email is passed to templates in the Chrome data dictionary after partial evaluation with the email_map as a parameter, resulting in the signature author_email(author).
  • The template author_or_creator.rss was refactored so that email_map doesn't need to be passed as a parameter.

Description

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; }

Attachments (0)

Change History (19)

comment:1 by Ryan J Ollos, 5 years ago

Milestone: next-stable-1.0.x
Owner: set to Ryan J Ollos
Status: newassigned

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

    diff --git a/trac/ticket/templates/ticket_change.html b/trac/ticket/templates/ticket_change.html
    index 7b9ea36..b37f3bc 100644
    a b Arguments:  
    5050    </span>
    5151    <py:choose>
    5252      <py:when test="'date' in change">
    53         <i18n:msg params="date, author">Changed ${pretty_dateinfo(change.date)} by ${authorinfo(change.author)}</i18n:msg>
     53        <i18n:msg params="date, author">Changed ${pretty_dateinfo(change.date)} by <span class="trac-comment-author">${authorinfo(change.author)}</span></i18n:msg>
    5454      </py:when>
    5555      <py:otherwise>
    56         <i18n:msg params="author">Changed by ${authorinfo(change.author)}</i18n:msg>
     56        <i18n:msg params="author">Changed by <span class="trac-comment-author">${authorinfo(change.author)}</span></i18n:msg>
    5757      </py:otherwise>
    5858    </py:choose>
    5959    <span py:if="preview or show_editor" class="trac-loading"/>

Does anyone see problems with the change?

Last edited 5 years ago by Ryan J Ollos (previous) (diff)

comment:2 by Ryan J Ollos, 5 years ago

Milestone: next-stable-1.0.x1.0.2

comment:3 by Ryan J Ollos, 5 years ago

Milestone: 1.0.21.1.2

comment:4 by Ryan J Ollos, 5 years ago

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.

in reply to:  4 ; comment:5 by Jun Omae, 5 years ago

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.

in reply to:  5 ; comment:6 by Ryan J Ollos, 5 years ago

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.

in reply to:  6 ; comment:7 by Jun Omae, 5 years ago

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>

in reply to:  7 ; comment:8 by Ryan J Ollos, 5 years ago

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

    diff --git a/trac/locale/messages.pot b/trac/locale/messages.pot
    index 4cf73c2..2be30d0 100644
    a b msgid ""  
    88msgstr ""
    99"Project-Id-Version: Trac 1.1.2\n"
    1010"Report-Msgid-Bugs-To: trac-dev@googlegroups.com\n"
    11 "POT-Creation-Date: 2013-12-11 03:25-0800\n"
     11"POT-Creation-Date: 2013-12-11 03:38-0800\n"
    1212"PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
    1313"Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
    1414"Language-Team: LANGUAGE <LL@li.org>\n"
    msgstr ""  
    18071807
    18081808#: trac/search/templates/search.html:56
    18091809#, python-format
    1810 msgid "By %(author)s"
     1810msgid "By %(author)s —"
    18111811msgstr ""
    18121812
    18131813#: trac/search/templates/search.html:65
    msgstr ""  
    46254625#, python-format
    46264626msgid ""
    46274627"[1:%(time)s] %(title)s\n"
    4628 "                  by [2:%(author)s]"
     4628"                  by %(author)s"
    46294629msgstr ""

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.

in reply to:  8 comment:9 by Jun Omae, 5 years ago

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

    diff --git a/trac/search/templates/search.html b/trac/search/templates/search.html
    index d296b84..9c84610 100644
    a b  
    5252            <py:for each="result in results">
    5353              <dt><a href="${result.href}" class="searchable">${result.title}</a></dt>
    5454              <dd class="searchable">${result.excerpt}</dd>
    55               <dd>
    56                 <i18n:msg params="author" py:if="result.author">By ${authorinfo(result.author)} &mdash;</i18n:msg>
    57                 <span class="date">${result.date}</span>
    58               </dd>
     55              <py:choose>
     56                <dd py:when="result.author" i18n:msg="author,date">
     57                  By ${authorinfo(result.author)} &mdash; <span class="date">${result.date}</span>
     58                </dd>
     59                <dd py:otherwise=""><span class="date">${result.date}</span></dd>
     60              </py:choose>
    5961            </py:for>
    6062          </dl>
    6163        </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.

comment:10 by Ryan J Ollos, 5 years ago

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

comment:11 by Ryan J Ollos, 5 years ago

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

comment:12 by Ryan J Ollos, 5 years ago

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

    diff --git a/trac/locale/messages.pot b/trac/locale/messages.pot
    index 98db418..adf783c 100644
    a b msgid ""  
    88msgstr ""
    99"Project-Id-Version: Trac 1.1.2\n"
    1010"Report-Msgid-Bugs-To: trac-dev@googlegroups.com\n"
    11 "POT-Creation-Date: 2013-12-15 13:31-0800\n"
     11"POT-Creation-Date: 2013-12-15 15:13-0800\n"
    1212"PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
    1313"Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
    1414"Language-Team: LANGUAGE <LL@li.org>\n"
    msgstr ""  
    18071807
    18081808#: trac/search/templates/search.html:56
    18091809#, python-format
    1810 msgid "By %(author)s"
     1810msgid "By %(author)s — [1:%(date)s]"
    18111811msgstr ""
    18121812
    1813 #: trac/search/templates/search.html:65
     1813#: trac/search/templates/search.html:67
    18141814#: trac/ticket/templates/report_view.html:97
    18151815#: trac/ticket/templates/report_view.html:208
    18161816msgid "No matches found."
    18171817msgstr ""
    18181818
    1819 #: trac/search/templates/search.html:69
     1819#: trac/search/templates/search.html:71
    18201820msgid ""
    18211821"[1:Note:] See [2:TracSearch]\n"
    18221822"        for help on searching."
    msgstr ""  
    23132313
    23142314#: trac/templates/list_of_attachments.html:18
    23152315#, python-format
    2316 msgid ""
    2317 "[1:%(file)s][2:<U+200B>]\n"
    2318 "       ([3:%(size)s]) -\n"
    2319 "      added by [4:%(author)s] %(date)s."
     2316msgid "[1:%(file)s][2:<U+200B>] ([3:%(size)s]) - added by %(author)s %(date)s."
    23202317msgstr ""
    23212318
    23222319#: trac/templates/list_of_attachments.html:28
    msgstr ""  
    46244621
    46254622#: trac/timeline/templates/timeline.html:48
    46264623#, python-format
    4627 msgid ""
    4628 "[1:%(time)s] %(title)s\n"
    4629 "                  by [2:%(author)s]"
     4624msgid "[1:%(time)s] %(title)s by %(author)s"
    46304625msgstr ""
    46314626
    46324627#: trac/timeline/templates/timeline.html:64
    msgstr ""  
    59655960msgid "Genshi %(error)s error while rendering template %(location)s"
    59665961msgstr ""
    59675962
    5968 #: trac/web/chrome.py:1101 trac/web/chrome.py:1109
     5963#: trac/web/chrome.py:1102 trac/web/chrome.py:1111
    59695964msgid "anonymous"
    59705965msgstr ""

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

comment:13 by Jun Omae, 5 years ago

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

  • trac/templates/list_of_attachments.html

    diff --git a/trac/templates/list_of_attachments.html b/trac/templates/list_of_attachments.html
    index 8e50cce..da5c048 100644
    a b Arguments:  
    1717  <py:def function="show_one_attachment(attachment)">
    1818    <i18n:msg params="file, size, author, date">
    1919      <a href="${url_of(attachment.resource)}" title="View attachment">${attachment.filename
    20         }</a><a href="${url_of(attachment.resource, format='raw')}" class="trac-rawlink" title="Download">&#8203;</a><!--
     20        }</a><a href="${url_of(attachment.resource, format='raw')}"
     21                class="trac-rawlink" title="Download">&#8203;</a><!--!
    2122      --> (<span title="${_('%(size)s bytes', size=attachment.size)}">${pretty_size(attachment.size)
    2223        }</span>) - added by ${authorinfo(attachment.author)} ${pretty_dateinfo(attachment.date)}.
    2324    </i18n:msg>

comment:14 by Ryan J Ollos, 5 years ago

API Changes: modified (diff)
Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

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

comment:15 by Ryan J Ollos, 5 years ago

API Changes: modified (diff)

See also related ticket #11474.

comment:16 by Ryan J Ollos, 5 years ago

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?

Last edited 5 years ago by Ryan J Ollos (previous) (diff)

in reply to:  16 ; comment:17 by Peter Suter, 5 years ago

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.

Last edited 5 years ago by Peter Suter (previous) (diff)

in reply to:  17 comment:18 by Ryan J Ollos, 5 years ago

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.

comment:19 by Ryan J Ollos, 5 years ago

API Changes: modified (diff)

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted.
to The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.