Edgewall Software
Modify

Opened 9 years ago

Closed 8 years ago

Last modified 6 years ago

#9981 closed defect (fixed)

Strike for ticket comment links

Reported by: Thijs Triemstra Owned by: trac@…
Priority: normal Milestone: 1.0
Component: rendering Version: 0.12-stable
Severity: minor Keywords: bitesized
Cc: ryano@… Branch:
Release Notes:
API Changes:

Description

A link to a comment in a ticket (comment:16:ticket:5120) does not strike the link when the ticket is closed.

For example:

with comment vs ticket closed without comment.

Attachments (2)

ticket-9981-v4.patch (1.9 KB ) - added by Eli Carter 8 years ago.
fix 2 bugs, add regression test
UsernamesHaveStrikethrough.png (39.3 KB ) - added by Ryan J Ollos <ryano@…> 8 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 by Remy Blank, 9 years ago

Keywords: bitesized added
Milestone: next-minor-0.12.x

That's pretty easy to fix.

comment:2 by trac@…, 8 years ago

  • ticket/api.py

     
    493493            href = "%s#comment:%s" % (formatter.href.ticket(resource.id), cnum)
    494494            title = _("Comment %(cnum)s for Ticket #%(id)s", cnum=cnum,
    495495                      id=resource.id)
     496            if 'TICKET_VIEW' in formatter.perm(resource):
     497                for status in self.env.db_query(
     498                    "SELECT status FROM ticket WHERE id=%s",
     499                    (str(resource.id),)):
     500                    return tag.a(label, href=href, title=title, _class=status)
    496501            return tag.a(label, href=href, title=title)
    497502        else:
    498503            return label

This patch only strikes the link if you actually have the permission to see the ticket state — otherwise you could gain information you should not be allowed to know.

by Eli Carter, 8 years ago

Attachment: ticket-9981-v4.patch added

fix 2 bugs, add regression test

comment:3 by Eli Carter, 8 years ago

Resolution: fixed
Status: newclosed

Committed to trunk in [10643]. Thanks to Radomir Dopieralski for the initial patch.

comment:4 by Remy Blank, 8 years ago

Milestone: next-minor-0.12.x0.13
Owner: set to trac@…

by Ryan J Ollos <ryano@…>, 8 years ago

comment:5 by Ryan J Ollos <ryano@…>, 8 years ago

While it seems correct to have comment:1:ticket:9981 display with strike-through, I'm not sure it makes sense for comment:1 to display with strike-through.

One effect of this that I noticed while commenting on #8295 is that the usernames shown at the top of quoted text when generating replyto comments are displayed in strike-through text, when the ticket is closed.

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

comment:6 by Ryan J Ollos <ryano@…>, 8 years ago

Cc: ryano@… added

in reply to:  5 ; comment:7 by Remy Blank, 8 years ago

Replying to Ryan J Ollos <ryano@…>:

While is seems correct to have comment:1:ticket:9981 display with strike-through, I'm not sure it makes sense for comment:1 to display with strike-through.

That makes sense, and is pretty easy to fix. Would you mind posting a patch?

comment:8 by Christian Boos, 8 years ago

I think there's a problem with the strike-through style used for closed on text links in general, not only here for ticket comments, but also for closed milestones. Using strike-through is pretty much a convention for ticket numbers, but even those are sometimes hard to read when striked through: #8808

I remember having experimented different alternatives at that time I introduced closed support for milestone (r5436), and I'm still somewhat unsatisfied with the current status. One alternative would be to use a border: #8808, milestone:0.12 (the border "encloses" the link…).

We could also add a background: #8808, milestone:0.12.

Or have only the background: #8808, milestone:0.12. This is a kind of strike-through but with a highlighter pen ;-)

comment:9 by Remy Blank, 8 years ago

I agree that strike-through is difficult to read, and should be removed (pretty much everywhere in Trac). The drawback of the background-only solution is that it won't be visible when printing. The same would apply if we showed an icon next to the link. The border would highlight the link compared to others, when in principle we would rather un-highlight it (assuming closed tickets are less important that open ones).

Difficult problem indeed ;)

in reply to:  9 comment:10 by Christian Boos, 8 years ago

What about simply: #8808, milestone:0.12?

comment:11 by Dirk Stöcker, 8 years ago

Please not. Even if strike-through may be hard to read sometimes, the presented alternatives are much harder to understand. So when you change that, please at least allow each Trac maintainer to simply use the old method. Usually the fact, that a ticket is closed is much more important, than its number.

in reply to:  7 ; comment:12 by Ryan J Ollos <ryano@…>, 8 years ago

Replying to rblank:

That makes sense, and is pretty easy to fix. Would you mind posting a patch?

Yes, I can take a crack at it by this weekend. The discussion about changing the text style for closed tickets is interesting, but am I correct to think that either way, references to comments within tickets should not provide any ticket status indication, and therefore the patch is needed and will be the same regardless of any changes to text styling for closed tickets?

I sort of like the red highlight background for closed ticket, but I'm not sure it is obvious enough. Then again, I might just be so used to the strike-through that nothing else will seem quite as intuitive.

in reply to:  12 ; comment:13 by Remy Blank, 8 years ago

Replying to Ryan J Ollos <ryano@…>:

am I correct to think that either way, references to comments within tickets should not provide any ticket status indication, and therefore the patch is needed and will be the same regardless of any changes to text styling for closed tickets?

I think so, yes. Unless others have strong opinions against it.

comment:14 by Christian Boos, 7 years ago

See follow-up in #10741.

in reply to:  13 comment:15 by Ryan J Ollos <ryano@…>, 7 years ago

Replying to rblank:

I think so, yes. Unless others have strong opinions against it.

I've encountered a number of issues while trying to implement this ticket, so I've created a new dedicated ticket for the work: #10742. I think the change is still relevant even in the context of the changes proposed in #10012.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain trac@….
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


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