652332 - bugzilla.mozilla.org should not make 'diff' view the default for attachments that are patches

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: General, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This has been bugging me since the last Bugzilla upgrade, but I hadn't gotten around to filing a bug on it; I'd been hoping somebody else would.

It's been really annoying since the last upgrade that the 'diff' view is the default view on patches in the bug comments, and in order to view the actual patch that was attached, you either:
 * need to scroll up to the attachments table at the top, or
 * need to manually edit the URL

I basically never use the diff view (and I've talked to some other platform developers who have the same opinion) because:
 * when reviewing patches, the diff view hides metadata that needs to be reviewed, such as the patch header and commit message
 * the diff view has known bugs that cause it to omit parts of the patch, which means we don't believe it's reliable.  (For example, it still doesn't show hg rename metadata, which means if you just look at the diff view, you wouldn't see that files are getting renamed in bug 651013 comment 4.  I've heard it has worse, too.)  This means it's not safe to use for reviews.
 * in many cases, the diff view makes it harder to read patches, since it's often useful to see the new and old code right next to each other.  (That said, there are cases where it makes it easier, and then I use it.)

This has been a pain point since the view of attachments in the comments part of the bug stopped.  Can we switch b.m.o to not default to the diff view in the comments link to attachments?  (I wouldn't mind if there's a [diff] link in the same format as in the attachments table at the top, though.  It also seems confusing that the way we link to attachments in the two is inconsistent.)

Steps to reproduce:
  1. go to bug 647421 comment 6
  2. click on "attachment 527719 [details] [diff] [review]"

Actual results: get the diff viewer
Expected results: see the attachment

Comment 1

[:Gavin Sharp [email: [e-mail]]]

I agree, the inconsistency between the comment links and attachment links bothers me. I review a lot of patches, and I really never want the diff view.

Comment 2

[Daniel C]

(In reply to comment #0)

> It's been really annoying since the last upgrade that the 'diff' view is the
> default view on patches in the bug comments, and in order to view the actual
> patch that was attached, you either:
>  * need to scroll up to the attachments table at the top, or
>  * need to manually edit the URL

Or click on "View" from the diff view. It's much quicker than scrolling up or editing the URL.

Comment 3

[Frédéric Buclin]

(In reply to comment #1)
> I agree, the inconsistency between the comment links and attachment links
> bothers me. I review a lot of patches, and I really never want the diff view.

I review a lot of patches too, and I really want the diff view. So it all depends who you are. That being said, I don't want this behavior to be reversed. In case you didn't notice, when you read "attachment 527719 [details] [diff] [review]", clicking on the attachment ID will display the diff view, but clicking on [details] displays the usual edit form (which is the one you want).

Comment 4

[Boris Zbarsky [:bzbarsky]]

Actually, I do NOT want the edit form when reviewing in most cases.  Or rather, I want one instance of the edit form and 2-3 instances of the normal patch view; I do my reviewing in the latter and the commenting in the former.  I agree with dbaron; this has been a pretty annoying usability regression for me; it's a constant time-waster.

As far as comment 2 goes, clicking "View" from the diff view means an extra pageload, which is slower than scrolling, given Bugzilla's pageload times.

I honestly couldn't care less what the default behavior is if we think that the majority of people prefer the diff view (though given that as dbaron pointed out that view is a blatant lie a large fraction of the time I don't see how anyone could prefer it by default), but I'd really appreciate a preference I could set that would make _my_ patch links always go to the normal patch view.  The current situation is just really unfortunate.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

The inconsistency between in-comment links and attachment table links is the main problem, IMO.

The secondary problem is that no one doing Mozilla development wants the diff view by default, because it doesn't properly handle our git-style diffs (as explained by dbaron in comment 0). Maybe it's fine for Bugzilla development, but that's not the majority use case on b.m.o. It needs to either be a per-install pref (if it already is, we should change it on bmo), or a per-user pref.

Comment 6

[David Lawrence [:dkl]]

What if we were to make this a user preference and default it back to the 3.6 method of showing the attachment if clicking on the id link, and then those that want to see the diff view, can set it that way.

dkl

Comment 7

[Gervase Markham [:gerv]]

LpSolit: do you at least agree that the inline links and the attachment table links should be consistent?

If so, I'd propose we change the inline version to:

attachment X [details] [diff] [review]

to match the attachment table version (i.e. the "attachment X" text links to the raw attachment, as it does in the attachment table). Of course, upstream Bugzilla would not have the "[review]" link.

That way, all possible views are accessible from both the table and from the inline, and there is no need for Yet Another User Preference.

How does that sound?

Gerv

Comment 8

[Frédéric Buclin]

(In reply to comment #7)
> attachment X [details] [diff] [review]

I would prefer attachment X [details | diff] in that case. Otherwise, yes, I would be fine with this proposal. Diff must remain, especially since PatchReader is again under active development (a new release is available since yesterday!).

Comment 9

[Gervase Markham [:gerv]]

The reason I chose that layout is that it's easier for addons to add additional links to the end (such as [review]) without having to figure out where the closing bracket is and stick something in before it.

Gerv

Comment 10

[David Lawrence [:dkl]]

Attachment: Patch to change the attachment linking in comments (v1)

Here is a patch that changes the way attachments are linked in the bug comments. It takes on the format of 

attachment X [diff] [details]

Of course with Splinter enabled it will also have a [review] on the end. Is
this acceptable?

dkl

Comment 11

[Frédéric Buclin]

Comment on attachment 531651 [details] [diff] [review]
Patch to change the attachment linking in comments (v1)

This patch regresses bug 474766.

Comment 12

[David Lawrence [:dkl]]

Attachment: Patch to change the attachment linking in comments (v2)

Thanks for the review. This new patch address the regression as well.

dkl

Comment 13

[Frédéric Buclin]

Comment on attachment 536377 [details] [diff] [review]
Patch to change the attachment linking in comments (v2)

r=LpSolit

Comment 14

[David Lawrence [:dkl]]

(In reply to comment #13)
> Comment on attachment 536377 [details] [diff] [review] [review]
> Patch to change the attachment linking in comments (v2)
> 
> r=LpSolit

Should I move this bug to Bugzilla/Attachments and you can approve it for trunk or was your intent for this stay as a BMO only change?

dkl

Comment 15

[Frédéric Buclin]

(In reply to comment #14)
> Should I move this bug to Bugzilla/Attachments and you can approve it for
> trunk or was your intent for this stay as a BMO only change?

When I reviewed it, my idea was to limit this patch to bmo only.

Comment 16

[David Lawrence [:dkl]]

Committed. Will be in the next code update.

Committing to: bzr+ssh://[e-mail]/bmo/4.0
modified Bugzilla/Template.pm
Committed revision 7802.

dkl