Hi Christopher, Thanks for the prompt reply. I've made a new patch which I'll include at the end of this mail. Its also here:
http://trac-hacks.org/attachment/ticket/3226/autoquery_patch_vs_r7302.txt On Tue, Jul 01, 2008 at 10:35:01PM +0200, Christopher Lenz wrote: > > Hey Jeff, > > On 01.07.2008, at 22:11, Jeff Hammel wrote: > > Also, if I wasn't verbose enough about it in the first email, I'd > > love feedback on how to do anything I'm doing here better so that I > > can write better code in the future. > > When asking for patch review on the mailing list, including the patch > inline would be helpful ;) Thanks. Will do so in the future. > On the patch itself: > > * What's the use case for configurable default query_args? Couldn't > the default options for the query module itself be reused instead, if > possible? Or just some sane default like all open tickets? Now I'm using [query]\ndefault_anonymous_query. Is that better? I don't want to use default_query as one of these fields is (by default) owner. > * The excluded_fields option refers to fields not included with Trac > core. This should probably default to an empty list instead, if it is > even necessary at all (note how I like to avoid adding config > options :P ) I've left this in for now. This is a solution to http://trac-hacks.org/ticket/3178 which is necessary since the timingandestimation plugin uses JS to markup the fields. A couple of alternatives here: * take out the excluded_fields entirely; this will make the patch cleaner, but will break timingandestimation. If the patch is committed to core, then I'll ticket that plugin noting the issue and maybe try to make a patch. * instead of having an option, just have these hard-coded fields with a "# XXX" noting the reason that this is here. Again, its probably worth alerting timingandestimation about this so we can kill this entirely. > * The option group should probably be just "ticket" instead of > "autoquery" Done for excluded_fields, the only remaining option. > * Don't use self.env.href, use req.href. Done. > * The query_link thing is kind of brute force. Would be nicer to just > use the **kwargs of req.href to construct the URL qith query string > params. And it probably should be `query_link(owner=ticket.owner)` > instead of `query_link('owner', ticket.owner)`. It would probably be > possible to have query_link be just a partial binding of req.href. I've kept this as a function in web_ui.py, though it does feel a bit brute force. I haven't changed it to be (**kw) since it will always take one key, value pair. If its really better to have it use **kw, I can do this. Also, maybe its better just to put this function in the template? Another alternative is just to markup the fields themselves and not bother with the function at all. Any thoughts on this? > * The template has two py:if="" directives that serve as if/else. > Should be using py:choose instead. Done. > On a related note, I'd prefer if the links in the ticket properties > box (the yellow box) would be more subtle (I'm assuming they're the > default red right now), possibly even only really discoverable on > hover. Many pages in Trac have already gotten way to "linky"; at some > point you end up with content where every second word is a red link, > which is ugly and distracting :P They are indeed the default red. Just to play Devil's advocate, I don't mind super-linky pages and actually like them. But I'm not against making these links more subtle. Is there an existing class that I can use to make these more subtle? I don't mind adding a class, but I don't really have much aptitude for UI, so any CSS I would do probably wouldn't be wonderful. Still I can try if you like. > Cheers, > -- > Christopher Lenz > cmlenz at gmx.de > http://www.cmlenz.net/ Thanks again for looking at this. Please let me know any further improvements I can make to this. Patch follows signature Jeff Hammel The Open Planning Project http://topp.openplans.org Index: trac/ticket/web_ui.py =================================================================== --- trac/ticket/web_ui.py (revision 7302) +++ trac/ticket/web_ui.py (working copy) @@ -26,7 +26,7 @@ from genshi.builder import tag from trac.attachment import AttachmentModule -from trac.config import BoolOption, Option, IntOption, _TRUE_VALUES +from trac.config import BoolOption, Option, IntOption, ListOption, _TRUE_VALUES from trac.core import * from trac.mimeview.api import Mimeview, IContentConverter, Context from trac.resource import Resource, get_resource_url, \ @@ -105,6 +105,10 @@ If set to 'default', this is equivalent to 'yes' for new environments but keeps the old behavior for upgraded environments (i.e. 'no'). (''since 0.11'').""") + + excluded_fields = ListOption('ticket', 'excluded_fields', + default=['estimatedhours', 'hours', 'totalhours'], + doc="fields to exclude from AutoQuery markup") # IContentConverter methods @@ -557,10 +561,20 @@ if preserve_newlines == 'default': preserve_newlines = self.env.get_version(initial=True) >= 21 # 0.11 preserve_newlines = preserve_newlines in _TRUE_VALUES + + def query_link(x, y): + query = req.href('query', **{x:y}) + args = self.env.config.get('query', 'default_anonymous_query') + if args: + query = '%s&%s' % (query, args) + return query + return {'ticket': ticket, 'context': Context.from_request(req, ticket.resource, absurls=absurls), - 'preserve_newlines': preserve_newlines} + 'preserve_newlines': preserve_newlines, + 'query_link': query_link, + 'excluded_fields': self.excluded_fields } def _toggle_cc(self, req, cc): """Return an (action, recipient) tuple corresponding to a change Index: trac/ticket/templates/ticket.html =================================================================== --- trac/ticket/templates/ticket.html (revision 7302) +++ trac/ticket/templates/ticket.html (working copy) @@ -44,6 +44,7 @@ </head> <body> + <py:def function="commentref(prefix, cnum)"> <a href="#comment:$cnum">$prefix$cnum</a> </py:def> @@ -135,9 +136,16 @@ not in ('type', 'owner')]"> <tr> <th id="h_reporter">Reported by:</th> - <td headers="h_reporter" class="searchable">${authorinfo(ticket.reporter)}</td> + <td headers="h_reporter" class="searchable"> + <a href="${query_link('reporter', ticket.reporter)}"> + ${authorinfo(ticket.reporter)} + </a> + </td> <th id="h_owner">Owned by:</th> - <td headers="h_owner">${ticket.owner and authorinfo(ticket.owner) or ''} + <td headers="h_owner"> + <a href="${query_link('owner', ticket.owner)}"> + ${ticket.owner and authorinfo(ticket.owner) or ''} + </a> </td> </tr> <tr py:for="row in group(fields, 2, lambda f: f.type != 'textarea')" @@ -154,7 +162,21 @@ <py:if test="field"> <py:choose test=""> <py:when test="'rendered' in field">${field.rendered}</py:when> - <py:otherwise>${ticket[field.name]}</py:otherwise> + <py:otherwise> + <py:if test="ticket[field.name]"> + <py:choose test="field.name in excluded_fields"> + <py:when test="False"> + + <a href="${query_link(field.name, ticket[field.name])}"> + ${ticket[field.name]} + </a> + </py:when> + <py:when test="True"> + ${ticket[field.name]} + </py:when> + </py:choose> + </py:if> + </py:otherwise> </py:choose> </py:if> </td> --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Trac Development" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/trac-dev?hl=en -~----------~----~----~----~------~----~------~--~---
