430638 - Provide source diff between versions

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Public Pages, enhancement)

Description

[Carlo Alberto Ferraris]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; it; rv:1.8.1.14) Gecko/20080404 Firefox/2.0.0.14
Build Identifier: 

It may be useful (especially for editors) to be able to have a visual diff between versions when viewing the source code of an extension.

Reproducible: Always

Comment 1

[u278084]

Attachment: Sample output

I've already started this, so I might as well finish it. I have attached what I am currently working on and integrating it with remora. This doesn't have to be final, but it is what I think works best.

The final UI should probably be similar to what browsing through file contents currently is. There should be a drag-able widget that lists the xpi structure. Clicking on a file would diff the sandbox version with the latest public version.

I don't see any sort of scenario where the editor would need to diff two old versions of the addon. And if there is, it's probably not common enough for the added complexity.

Comment 2

[u278084]

Attachment: v1.0

Attached is a patch that I think does what I/we want it to do. This requires the xdiff module of php to be installed. I could also use the /usr/bin/diff (or anything that outputs a unified diff) if getting that module isn't an option.

FYI, I added a string to en_US that needs to be merged with other locales.

I kept the output mostly the same from the sample output, but merged it with the editor's browse page. I had to add bits of extra logic, but I think it's ok.

Right now, it's accessible from an addon editor's review page only if there is a public version available (since that was the last addon to be reviewed by an editor). Supposedly, the diff could also be accessed if the addon author specified that the source is viewable online, but there is no user interface to get to it and I haven't tested it. But it should follow the same permissions as browsing the source code.

Comment 3

[Fred Wenzel [:wenzel]]

Comment on attachment 329119 [details] [diff] [review]
v1.0

I have a few things (so far ;)).

- I think you want to wrap your functions parse_diff etc. in a component (controllers/components/diff.php  or so) so they don't clutter the files controller.
- You may want to add some CSS to a CSS file instead of using inline CSS for the changed/removed lines
- yellow and pink is scary ;)
- and the most important thing: Can you make it a config option which kind of diff you want to use? I have trouble installing pecl extensions into my MAMP instance. If you allow choosing a path (or empty, meaning "use xdiff"), it'll be flexible enough for me and everybody else who wants to use it.

Comment 4

[u278084]

Attachment: version 1.1

strike 1. Here we go again :)

(In reply to comment #3)
> - I think you want to wrap your functions parse_diff etc. in a component
> (controllers/components/diff.php  or so) so they don't clutter the files
> controller.

Yes. I knew these belonged somewhere else, I wasn't sure if a component was it. I thought they were for frequently reused functions, and this certainly won't be. But reducing clutter is also a good reason :)

> - You may want to add some CSS to a CSS file instead of using inline CSS for
> the changed/removed lines
Done.

> - yellow and pink is scary ;)
Heh. I changed those to wikipedia's colours (brighter yellow + light green)

> - and the most important thing: Can you make it a config option which kind of
> diff you want to use? I have trouble installing pecl extensions into my MAMP
> instance. If you allow choosing a path (or empty, meaning "use xdiff"), it'll
> be flexible enough for me and everybody else who wants to use it.

Done. I included an option in config.php called DIFF_PATH. if you define it and set it's value to the path to a diff program, it will be called instead. Only works with GNU's diff program so far, but I think it'll be sufficient.

Comment 5

[Fred Wenzel [:wenzel]]

Comment on attachment 331146 [details] [diff] [review]
version 1.1

I hand-crafted two different versions of adblock plus on my local box, uploaded, then diffed them using your tool, and it works great.

Something to be considered would be indicating which files even changed at all? For big add-ons you have to click and scroll through all files otherwise. Anyway that's an additional feature request of mine, not a flaw in your implementation.

Well done, Cesar.

Comment 6

[Fred Wenzel [:wenzel]]

I checked in the code for you: r17536. The localization is in r17538. I'll notify the localizers shortly.

Comment 7

[Fred Wenzel [:wenzel]]

Btw: When pushing, IT needs instructions on defining the diff path in the config file.

Comment 8

[u278084]

Filed bug 449537 to bring this to IT's attention.

Comment 9

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED on https://preview.addons.mozilla.org/en-US/firefox/files/diff/27074/; looks like this is up and running there.