Diffing Extensions should have better support for images

NEW
Unassigned

Status

P5
enhancement
8 years ago
3 years ago

People

(Reporter: realRaven, Unassigned)

Tracking

Details

(Whiteboard: [ReviewTeam:P2])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.13) Gecko/20101203 Firefox/3.6.13
Build Identifier: 

When diffing previous versions during extensions review process, Images should be displayed as images (and not as binary source code). Also if a new image was uploaded (binaries differ) then the original image and the new image should be displayed side by side. 

Its not really practical to review a binary in "source code" form especially when it is a png, gif or jpeg image. A toggle from image to bytewise view would be helpful but default should always be to display an image as viewable image.

Reproducible: Always
For a diff tool in general this is a good idea.  However, for the purpose of this tool, for identifying code changes we have to inspect, I don't think it will add any benefit.  
IMO it would be better just to skip image files completely.
(Reporter)

Comment 2

8 years ago
(In reply to comment #1)
> IMO it would be better just to skip image files completely.

I do not think so, IMHO diffing tools should never skip any file. But imagine if there are hacked images in there that contain pornographic images or a screenshot of a link to a malicious page? At least we should be able to see this at a glance.

Also, when reviewing Themes it would add quite a lot of value to see how the authors have enhanced their assets. (Especially useful when you can see old and new image side by side)
(Reporter)

Comment 3

8 years ago
Plus, I would imagine this should be a relatively trivial change? Looking at the raw binary image code doesn't really add any value to me.
This already works (or used to work) for certain image types. There may be others that aren't working yet, maybe. Can you provide a link of an image that appears as a bunch of text instead of the actual image?

I agree with Andrew in general. It's not practical to check every image. Plus, if you have the opportunity to hack an add-on, there is much more damage you can do than replace an image with porn or a screenshot of a link.
Whiteboard: [required amo-editors]
(Reporter)

Comment 5

8 years ago
(In reply to comment #4)
> This already works (or used to work) for certain image types. There may be
> others that aren't working yet, maybe. Can you provide a link of an image that
> appears as a bunch of text instead of the actual image?
> 
> I agree with Andrew in general. It's not practical to check every image. 

Not so much a matter of "have to" but "be able to" :)

I like to review Themes (and if you at my statistics I have been a lot more productive in the last month) and it can be helpful to have a comparison option for something like a button graphic, just as it is helpful to have a diff of a CSS file. Also I usually make screenshots for showing the author improvements or bugs.

And the bug only appears when diffing versions, not when viewing. I will post  up a link to an example, just need to reboot into Fx4 as all current themes updates are Fx4+
(Reporter)

Comment 6

8 years ago
Created attachment 506242 [details]
The image as byte code bug

To reproduce the bug, here is one (in the rulerbar extension) to start with

<a href="javascript: void(0);" onclick="viewFile('/en-US/firefox/files/diff/108736/?compare=icon.png');">icon.png</a>

Just click on Compare with public version and click on icon.png
(Reporter)

Comment 7

8 years ago
Created attachment 506243 [details]
Same File, in "View Contents" mode

When clicking on "View Contents" the same file is rendered correctly
(Reporter)

Comment 8

8 years ago
Created attachment 506244 [details]
This is what I could expect from "view differences"

This solution would display original and changed images side by side in human readable form. Perfect for checking differences and creating screenshots for the add-on author.

Some Meta-info on the file would be an added bonus. Hope these screen shots make it easier to see what I would like. My feeling is that the fix would be relatively easy (as the "view contents" mode already works correctly), but the benefit quite big.

Just your humble themes reviewer's opinion, of course.
(Reporter)

Comment 9

8 years ago
Created attachment 506245 [details]
The Bug

First image was faulty, marked as obsolete.
Attachment #506242 - Attachment is obsolete: true
Confirming as a request for enhancement.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → amckay
Priority: -- → P5
Target Milestone: --- → Q2 2011
Target Milestone: Q2 2011 → 4.x (triaged)
(Reporter)

Comment 11

7 years ago
Any news on this? I know everybody is busy, but when reviewing themes its a bit of a showstopper - at the moment it doesn't even show the current image. Instead it shows:

"Download Filepicker.png
This file is not viewable online. Please download the file to view the contents.
Version: 3.33 • Size: 10.0 KB • MD5 hash: 2bf0e860ffc1800238e411f8767bd411 • Mimetype: image/png"

at least showing png and jpeg's inline would be nice to save the extra steps
Reclassifying editor bugs and changing to a new whiteboard flag. Spam, spam, spam, spam...
Whiteboard: [required amo-editors] → [ReviewTeam]
Matt, Kris mentioned that you have fixed this for Marketplace. Is it something that can be easily ported to the AMO file viewer / differ?
Assignee: amckay → mattbasta

Comment 14

6 years ago
I probably could. I think it just needs to have the flags for marketplace removed.
(Reporter)

Comment 15

5 years ago
(In reply to Matt Basta [:basta] from comment #14)
> I probably could. I think it just needs to have the flags for marketplace
> removed.

Could you look into this? A year has passed...
this doesn't work on Marketplace either, fwiw. bug 886272
Assignee: mattbasta → nobody
Basta already did this for Marketplace. We should just be able to port it.
Whiteboard: [ReviewTeam] → [ReviewTeam:P2]

Updated

4 years ago
Target Milestone: 4.x (triaged) → 2014-09
Target Milestone: 2014-09 → ---
(Assignee)

Updated

3 years ago
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.