487330 - Links in comments pointing to patches should go to the 'diff' view rather than the plain text view, by default

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, enhancement)

Description

[Uri Okrent]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.0.5) Gecko/2008120908 Red Hat/3.0.5-1.el5_2 Firefox/3.0.5
Build Identifier: bugzilla 3.2.2

It would be cool if the links to attachments (in the comments at least but also in the attachments table) link to the diff view rather than the plain text view if the attachment is a patch (ispatch is true), and the patchviewer is installed. How about it? I've got a patch.

Reproducible: Always

Comment 1

[Uri Okrent]

Attachment: adds '&action=diff' to attachment links where the attachment is a patch

Comment 2

[Frédéric Buclin]

Comment on attachment 371563 [details] [diff] [review]
adds '&action=diff' to attachment links where the attachment is a patch

No, the attachment table already had a "Diff" link to display patches in diff mode.

About comments, this means the behavior would be different for patches than for any other type of attachments. I'm not sure that's what we want (you can already use the [Details] link if you want to). I will have to think about this a bit more.

Comment 3

[Uri Okrent]

Yeah, in the attachments table it is kind of redundant.

In the comments though, I think that is what I prefer for patches, and they are already treated differently from other attachments (the ispatch field in the db). I think I personally would prefer to see the diff by default, and hit the [details] link and jump through a hoop or two to see/download the patch itself, rather than easy access to the patch itself and more hoops for the diff. Especially because while I'm reading comments I'm trying to understand what's going on so in that context the side-by-side diff is more useful.

Comment 4

[Uri Okrent]

Comment on attachment 371563 [details] [diff] [review]
adds '&action=diff' to attachment links where the attachment is a patch

newer patch obsoletes this one.

Comment 5

[Uri Okrent]

Attachment: links to patch type attaches *in comments* link to diff

Sorry for the spam, but I didn't like the first patch as you rightly pointed out. I removed the stuff that modifies the attachment table. I also fixed whitespace in the rest of the patch because it wasn't consistent with the surrounding code.

Comment 6

[Max Kanat-Alexander]

Comment on attachment 371573 [details] [diff] [review]
links to patch type attaches *in comments* link to diff

You'll have to request review to get this looked at. See our development process here:

http://wiki.mozilla.org/Bugzilla:Developers

Also, you can just my $installed = eval { require PatchReader } since all modules return 1 if they finish properly. Maybe require PatchReader;1 at the worst.

Comment 7

[Uri Okrent]

(In reply to comment #6)
> (From update of attachment 371573 [details] [diff] [review])
> You'll have to request review to get this looked at. See our development
> process here:
> 
> http://wiki.mozilla.org/Bugzilla:Developers

Cool, will do that.

> Also, you can just my $installed = eval { require PatchReader } since all
> modules return 1 if they finish properly. Maybe require PatchReader;1 at the
> worst.

That's a nifty trick--I didn't know you could do that. But, the way I did it, I lifted that exact chunk from another place in the code (I think it was that same file as a matter of fact, though I don't have it in front of me). Should I still make that change, even if it means being inconsistent with the other spot? I have to admit it is nicer though...

Comment 8

[Frédéric Buclin]

Comment on attachment 371573 [details] [diff] [review]
links to patch type attaches *in comments* link to diff

Works fine. mkanat's suggestion is indeed a good idea. I have updated the patch accordingly, for checkin. r=LpSolit

Comment 9

[Frédéric Buclin]

Attachment: checked in patch

I did a slight change compared to the initial patch: we now only look for PatchReader if the attachment is a patch. In a separate bug, we should store this information in Bugzilla->request_cache.

Comment 10

[Frédéric Buclin]

Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.100; previous revision: 1.99
done

Comment 11

[Max Kanat-Alexander]

Added to the release notes in bug 547466.

Comment 12

[Denis Roy]

I guess having a user pref for this behaviour would be out of the question?

https://bugs.eclipse.org/bugs/show_bug.cgi?id=334892

Comment 13

[Frédéric Buclin]

(In reply to comment #12)
> I guess having a user pref for this behaviour would be out of the question?

Yeah, that's not the plan.