Merge lp://qastaging/~spiv/launchpad/bmp-inline-diffs into lp://qastaging/launchpad

[tweak] Could you please change the css so you get a pointing-hand cursor (link cursor) on the expander?

http://people.canonical.com/~andrew/Inline-diff-screenshot.png

[tweak] The flag ought to be called foo.enabled

I think this is not going to work for private branches because of <https://bugs.launchpad.net/launchpad/+bug/806713>. For an initial closed beta we can probably just live with it being broken. As an interim step, I guess we would have the option of disabling it for private branches. Eventually we'd need to fix that.

I'm adding more API to the expander to allow one to linkify the "toggler" with an easy parameter to setUp in my follow-up branch for expander-anim. expander-anim should also already accept failed parameter to receive method (so it tries to re-load stuff when it fails, just use receive(node, true) in the error handler).

Hi,

There are conflicts here with Danilo's branch I think. Could you take a look?

Gavin.

Conflicts have been resolved.

Specifically, the thing with the flag is that the name in the templates is not the same as the name in the flags.py documentation.

Regarding the CSS, as far as I'm aware my patch is doing everything it's supposed to. It looks to me like a bug in the expander infrastructure rather than this patch. I've pinged Danilo on IRC about it.

I've fixed the feature flag name: I actually had it correct in the TAL, but misnamed in flags.py. Fixed now, I think.

Andrew, just a few minor comments:

 - getDiffUrl could probably be dropped and replaced with bmpGetDiffUrl use (i.e. bmpGetDiffUrl(revno, revno-1))
 - perhaps you could accept undefined (and use Y.Lang.isUndefined or Y.Lang.isValue [which is false for nulls and undefineds]) as the end_revno in bmpGetDiffUrl when you just want one revision
 - with the changes to the expander in the meantime, it'd probably be better to instanciate the expander directly instead of using createByCSS. That will allow you to call expander.setUp(true), when the content inside the "toggle" node will be turned into a proper link (and you shouldn't wrap it inside <a> in TAL, or it'll end up being <a> inside an <a>)
 - alternatively to the above, you can just add 'href="#"' to get the pointing hand cursor to appear if you feel like the link should stay in TAL or you prefer createByCSS API
 - generally, tables should not be used for non-tabular-data formatting, but if existing CSS classes you use don't work if you simply replace <td>s with <div>s, just keep it as is
 - you may also need to change test set-up (especially in HTML) because of the recent changes to the test infrastructure; try looking at other existing tests for reference and make sure the test passes when run as "bin/test -t test_branchrevisionexpander.html"

I consider most of the above minor issues, so here's a conditional approval on fixing those listed above.

FWIW, expander-anim is landing as soon as we get out of RC mode.

 - I dropped getDiffUrl (and renamed bmpGetDiffUrl to bmp_get_diff_url to be more consistent with the rest of the file)
 - I added more JS tests for bmp_get_diff_url (and fixed a bug that uncovered! — diff ranges starting with 0 other than 0..1 were wrong)
 - bmp_get_diff_url now accepts either a single revno, or a pair of revnos describing a range
 - I'm still using createByCSS, as that seems to be more concise and convenient than the alternative
 - I added href="#', which fixes the cursor, thanks!
 - The table is precisely how the existing merge proposal diff is formatted. If that should change, then that's an issue for a separate patch I think. I'm just reusing what we already do.
 - I tested with bin/test as suggested, and the test appears to run just fine. Hooray!

Thanks for the review. I've addressed all the points, so I think this is ready to land. Sweet!