launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing

What will happen if I switch to another diff? And how can we integrate inline comment navigation?

Also a few inline comments.

Diff comments:

> === modified file 'lib/canonical/launchpad/icing/style.css'
> --- lib/canonical/launchpad/icing/style.css	2014-11-27 19:57:29 +0000
> +++ lib/canonical/launchpad/icing/style.css	2015-04-06 23:17:39 +0000
> @@ -597,6 +597,11 @@
>     background-color: white;
>     margin-bottom: 0.5em;
>  }
> +table.diff .cursor {
> +    float: left;
> +    font-weight: bold;
> +    margin: 0 0 0 1em;
> +}
>  table.diff .text { padding-left: 0.5em; }
>  table.diff .line-no {
>    text-align: right;
> @@ -604,6 +609,7 @@
>    color: #999;
>    border-right: solid 4px white;
>    width: 4em;
> +  cursor: pointer;
>  }
>  table.diff .line-no.active {
>    background: url(/@@/add) #f6f6f6 center left no-repeat;
> 
> === modified file 'lib/lp/code/javascript/branchmergeproposal.inlinecomments.js'
> --- lib/lp/code/javascript/branchmergeproposal.inlinecomments.js	2014-11-30 22:03:05 +0000
> +++ lib/lp/code/javascript/branchmergeproposal.inlinecomments.js	2015-04-06 23:17:39 +0000
> @@ -9,6 +9,16 @@
>  
>  YUI.add('lp.code.branchmergeproposal.inlinecomments', function(Y) {
>  
> +if (typeof window.KeyEvent === "undefined") {
> +    window.window.KeyEvent = {
> +        DOM_VK_RETURN: 13,
> +        DOM_VK_J: 74,
> +        DOM_VK_K: 75,
> +        DOM_VK_N: 78,
> +        DOM_VK_P: 80
> +    };
> +}
> +
>  // Grab the namespace in order to be able to expose the connect methods.
>  var namespace = Y.namespace('lp.code.branchmergeproposal.inlinecomments');
>  
> @@ -88,9 +98,8 @@
>              draft_div.one('.editorShortcutTip').remove();
>          });
>          widget.editor.on('keydown', function(e) {
> -           var enterKeyCode = 13;
>             if (e.domEvent.ctrlKey === true &&
> -               e.domEvent.button === enterKeyCode) {
> +               e.domEvent.button === window.KeyEvent.DOM_VK_RETURN) {
>                 this.save();
>             }
>          });
> @@ -118,7 +127,6 @@
>          namespace.delete_draft(draft_div);
>          namespace.flush_drafts_to_server();
>      };
> -
>      // The editor can be invoked by double-clicking a diff line or
>      // clicking the line number. Hovering over a line shows an edit icon
>      // near the line number, to hint that it's clickable.
> @@ -706,6 +714,112 @@
>  
>  });
>  
> +namespace.getHeaders = function() {
> +    return document.querySelectorAll(
> +    'table.diff td.diff-file, table.diff td.diff-chunk');

Indentation.

> +};
> +
> +namespace.headers = namespace.getHeaders();
> +namespace.scrollPos = -1;
> +
> +namespace.addNavCursor = function(headers, row) {
> +    var prevHeader = headers[namespace.scrollPos];
> +    if (prevHeader) {

We tend to use_underscores for JS variables.

> +        var cursorDiv = document.getElementsByClassName('cursor');
> +        while(cursorDiv.length > 0){

Spaces on the outside of the parens.

> +            cursorDiv[0].parentNode.removeChild(cursorDiv[0]);
> +        }
> +    }
> +    row.childNodes[0].innerHTML =
> +        '<div class="cursor">&#10095;</div>' + row.childNodes[0].innerHTML;
> +};

I think this might be nicer as a simple CSS class with a :before rule. If not, the getElementsByClassName needs to be fixed such that it won't remove every element in the document with class cursor, as that's very non-specific.

> +
> +namespace.getHeaderRow = function(headers, pos) {
> +    var id;
> +    // XXX blr 20150407: Workaround for rare case where the headers collection
> +    // inexplicably loses parentNodes.
> +    if (!headers[pos].parentNode) {
> +        namespace.headers = namespace.getHeaders();
> +        headers = namespace.headers;
> +    }

Do you have a simple reproducer for this misbehaviour?

> +    id = headers[pos].parentNode.id;
> +    return document.getElementById(id);

Why not just return parentNode directly? Then this won't break if they end up without IDs.

> +};
> +
> +namespace.updateScrollState = function(headers, pos) {
> +    var row = namespace.getHeaderRow(headers, pos);
> +    namespace.addNavCursor(headers, row);
> +    namespace.scrollPos = pos;
> +    return row;
> +};
> +
> +namespace.headerExists = function(header, type) {
> +    return header.className.indexOf(type) > -1;

This checks whether a header is of the given type.

> +};
> +
> +namespace.getNextHeader = function(headers, type, direction) {

I'd probably make this findNextHeader, so we can avoid duplicating the updateScrollState calls everywhere.

> +    var cursor = namespace.scrollPos;
> +    var i;
> +    if (headers.length <= 1) {
> +        return namespace.getHeaderRow(headers, 0);
> +    }
> +    if (direction === 'forwards') {
> +        for (i=cursor + 1; i < headers.length; i++) {
> +            if (namespace.headerExists(headers[i], type)) {
> +                return namespace.updateScrollState(headers, i);
> +            }
> +        }
> +    } else {
> +        if (cursor === 1 || cursor === 0) { cursor = headers.length; }
> +        for (i=cursor - 1; i > -1; --i) {
> +            if (namespace.headerExists(headers[i], type)) {
> +                return namespace.updateScrollState(headers, i);
> +            }
> +        }
> +    }
> +    if (type === 'file') {
> +        return namespace.updateScrollState(headers, 0);
> +    } else {
> +        return namespace.updateScrollState(headers, 1);
> +    }

Is this the fallback case, assuming the first file header is the zeroth header and the first hunk header is the first? That's probably reasonable, but could do with a comment.

> +};
> +
> +namespace.scrollToHeader = function(type, direction) {
> +    if (namespace.headers.length > 0) {
> +        var header = namespace.getNextHeader(
> +            namespace.headers, type, direction);
> +        header.scrollIntoView({block: 'start', behavior: 'smooth'});

I don't think scrollIntoView takes options in WebKit or Blink (yet?), so it'll end up jumping straight there.

> +    }
> +};
> +
> +/*
> + * Vim style bindings for diff previews. n/p (hunks), j/k (files)
> + */
> +namespace.handleNavKeys = function(e) {
> +    // Do not respond to keydown events when inline editor is open.
> +    var editableCommentBox = Y.one('div.yui3-editable_text-edit_mode');
> +    if (!(editableCommentBox ||
> +          document.activeElement.className === 'comment-text')) {
> +        if (e.button === window.KeyEvent.DOM_VK_J) {
> +            namespace.scrollToHeader('file', 'forwards');
> +        }
> +        if (e.button === window.KeyEvent.DOM_VK_K) {
> +            namespace.scrollToHeader('file', 'backwards');
> +        }
> +        if (e.button === window.KeyEvent.DOM_VK_N) {
> +            namespace.scrollToHeader('chunk', 'forwards');
> +        }
> +        if (e.button === window.KeyEvent.DOM_VK_P) {
> +            namespace.scrollToHeader('chunk', 'backwards');
> +        }
> +    }
> +};

This probably wants to skip any input/textarea/select/other-text-accepting-element.

It also captures eg. Ctrl+K, so the page jumps back as I switch focus to Firefox's search bar. Modifiers should probably cause the keystroke to be ignored.

> +
> +var diffNavKeys = [window.KeyEvent.DOM_VK_J, window.KeyEvent.DOM_VK_K,
> +                   window.KeyEvent.DOM_VK_N, window.KeyEvent.DOM_VK_P];
> +Y.one('body').on('key', namespace.handleNavKeys,
> +                 'down:' + diffNavKeys.join(','));

This should probably only be added in some setup function, rather than whenever the module is loaded. handleNavKeys might get a bit angry if there is no diff initialised.

> +
>  namespace.DiffNav = DiffNav;
>  
>  }, '0.1', {requires: ['datatype-date', 'event', 'io', 'node', 'widget',
> 
> === modified file 'lib/lp/code/stories/branches/xx-branchmergeproposals.txt'
> --- lib/lp/code/stories/branches/xx-branchmergeproposals.txt	2014-02-25 16:53:24 +0000
> +++ lib/lp/code/stories/branches/xx-branchmergeproposals.txt	2015-04-06 23:17:39 +0000
> @@ -622,7 +622,7 @@
>  The text of the review diff is in the page.
>  
>      >>> print repr(extract_text(get_review_diff()))
> -    u'Preview Diff\nDownload diff\n1\n...Fake Diff\u1010'
> +    u'Preview Diff\n[J/K] Next/Prev File, [N/P] Next/Prev Hunk\nDownload diff\n1\n---\n2\n+++\n3\n@@ -0,0 +1 @@\n4\n+Fake Diff\u1010'
>  
>  There is also a link to the diff URL, which is the preview diff URL plus
>  "+files/preview.diff". It redirects logged in users to the file in the
> 
> === modified file 'lib/lp/code/templates/branchmergeproposal-diff.pt'
> --- lib/lp/code/templates/branchmergeproposal-diff.pt	2014-05-15 12:53:25 +0000
> +++ lib/lp/code/templates/branchmergeproposal-diff.pt	2015-04-06 23:17:39 +0000
> @@ -6,6 +6,7 @@
>         tal:define="attachment view/preview_diff/diff_text">
>      <tal:real-diff condition="attachment">
>        <div class="boardCommentDetails filename">
> +        <div class="editorShortcutTip">[J/K] Next/Prev File, [N/P] Next/Prev Hunk</div>
>          <ul class="horizontal">
>            <li>
>              <a tal:attributes="href view/preview_diff/fmt:url">
> 
> === added directory 'lib/lp/testing/data'
> === added file 'lib/lp/testing/data/large.diff'
> --- lib/lp/testing/data/large.diff	1970-01-01 00:00:00 +0000
> +++ lib/lp/testing/data/large.diff	2015-04-06 23:17:39 +0000
> @@ -0,0 +1,191 @@
> +=== zbqvsvrq svyr 'yvo/yc/nafjref/grfgf/grfg_dhrfgvba_jrofreivpr.cl'
> +--- yvo/yc/nafjref/grfgf/grfg_dhrfgvba_jrofreivpr.cl    2015-03-13 04:07:31 +0000
> ++++ yvo/yc/nafjref/grfgf/grfg_dhrfgvba_jrofreivpr.cl    2015-03-17 15:34:05 +0000
> +@@ -1,12 +1,18 @@
> +-# Pbclevtug 2011 Pnabavpny Ygq.  Guvf fbsgjner vf yvprafrq haqre gur
> ++# Pbclevtug 2011-2015 Pnabavpny Ygq.  Guvf fbsgjner vf yvprafrq haqre gur
> + # TAH Nssreb Trareny Choyvp Yvprafr irefvba 3 (frr gur svyr YVPRAFR).
> + 
> + """Jrofreivpr havg grfgf eryngrq gb Ynhapucnq Dhrfgvbaf."""
> + 
> + __zrgnpynff__ = glcr
> + 
> ++sebz qngrgvzr vzcbeg (
> ++    qngrgvzr,
> ++    gvzrqrygn,
> ++    )
> ++
> + sebz OrnhgvshyFbhc vzcbeg OrnhgvshyFbhc
> + sebz ynme.erfgshypyvrag.reebef vzcbeg UGGCReebe
> ++vzcbeg clgm
> + sebz fvzcyrwfba vzcbeg qhzcf
> + sebz grfggbbyf.zngpuref vzcbeg Rdhnyf
> + vzcbeg genafnpgvba
> +@@ -30,6 +36,7 @@
> +     erpbeq_gjb_ehaf,
> +     GrfgPnfr,
> +     GrfgPnfrJvguSnpgbel,
> ++    gvzr_pbhagre,
> +     jf_bowrpg,
> +     )
> + sebz yc.grfgvat.ynlref vzcbeg (
> +@@ -257,7 +264,11 @@
> +     ynlre = QngnonfrShapgvbanyYnlre
> + 
> +     qrs grfg_frnepuDhrfgvbaf(frys):
> +-        perngrq = [frys.snpgbel.znxrDhrfgvba(gvgyr="sbb") sbe v va enatr(10)]
> ++        qngr_tra = gvzr_pbhagre(
> ++            qngrgvzr(2015, 01, 01, gmvasb=clgm.HGP), gvzrqrygn(qnlf=1))
> ++        perngrq = [
> ++            frys.snpgbel.znxrDhrfgvba(gvgyr="sbb", qngrperngrq=arkg(qngr_tra))
> ++            sbe v va enatr(10)]
> +         jrofreivpr = jrofreivpr_sbe_crefba(frys.snpgbel.znxrCrefba())
> +         pbyyrpgvba = jrofreivpr.anzrq_trg(
> +             '/dhrfgvbaf', 'frnepuDhrfgvbaf', frnepu_grkg='sbb',
> +=== zbqvsvrq svyr 'yvo/yc/pbqr/oebjfre/pbasvther.mpzy'
> +--- yvo/yc/pbqr/oebjfre/pbasvther.mpzy	2015-03-13 14:15:24 +0000
> ++++ yvo/yc/pbqr/oebjfre/pbasvther.mpzy	2015-03-19 17:04:22 +0000
> +@@ -806,12 +806,27 @@
> +         cngu_rkcerffvba="fgevat:+ers/${cngu}"
> +         nggevohgr_gb_cnerag="ercbfvgbel"
> +         ebbgfvgr="pbqr"/>
> +-    <oebjfre:cntr
> ++    <oebjfre:cntrf
> +         sbe="yc.pbqr.vagresnprf.tvgers.VTvgErs"
> +         pynff="yc.pbqr.oebjfre.tvgers.TvgErsIvrj"
> +-        crezvffvba="ynhapucnq.Ivrj"
> +-        anzr="+vaqrk"
> +-        grzcyngr="../grzcyngrf/tvgers-vaqrk.cg"/>
> ++        crezvffvba="ynhapucnq.Ivrj">
> ++        <oebjfre:cntr
> ++            anzr="+vaqrk"
> ++            grzcyngr="../grzcyngrf/tvgers-vaqrk.cg"/>
> ++        <oebjfre:cntr
> ++            anzr="++ers-pbzzvgf"
> ++            grzcyngr="../grzcyngrf/tvgers-pbzzvgf.cg"/>
> ++    </oebjfre:cntrf>
> ++    <oebjfre:cntr
> ++        sbe="yc.pbqr.vagresnprf.tvgers.VTvgErs"
> ++        anzr="+znpebf"
> ++        crezvffvba="mbcr.Choyvp"
> ++        grzcyngr="../grzcyngrf/tvgers-znpebf.cg"/>
> ++    <oebjfre:cntr
> ++        sbe="yc.pbqr.vagresnprf.tvgers.VTvgErsOngpuAnivtngbe"
> ++        crezvffvba="mbcr.Choyvp"
> ++        anzr="+ers-yvfgvat"
> ++        grzcyngr="../grzcyngrf/tvgers-yvfgvat.cg"/>
> + 
> +     <oebjfre:zrahf
> +         pynffrf="CebqhpgOenapurfZrah"
> +
> +=== zbqvsvrq svyr 'yvo/yc/pbqr/oebjfre/tvgers.cl'
> +--- yvo/yc/pbqr/oebjfre/tvgers.cl	2015-03-13 14:15:24 +0000
> ++++ yvo/yc/pbqr/oebjfre/tvgers.cl	2015-03-19 17:04:22 +0000
> +@@ -17,3 +17,12 @@
> +     @cebcregl
> +     qrs ynory(frys):
> +         erghea frys.pbagrkg.qvfcynl_anzr
> ++
> ++    @cebcregl
> ++    qrs gvc_pbzzvg_vasb(frys):
> ++        erghea {
> ++            "fun1": frys.pbagrkg.pbzzvg_fun1,
> ++            "nhgube": frys.pbagrkg.nhgube,
> ++            "nhgube_qngr": frys.pbagrkg.nhgube_qngr,
> ++            "pbzzvg_zrffntr": frys.pbagrkg.pbzzvg_zrffntr,
> ++            }
> +
> +=== zbqvsvrq svyr 'yvo/yc/pbqr/oebjfre/tvgercbfvgbel.cl'
> +--- yvo/yc/pbqr/oebjfre/tvgercbfvgbel.cl	2015-03-13 14:15:24 +0000
> ++++ yvo/yc/pbqr/oebjfre/tvgercbfvgbel.cl	2015-03-24 13:10:52 +0000
> +@@ -18,6 +18,7 @@
> + 
> + sebz yc.ncc.oebjfre.vasbezngvbaglcr vzcbeg VasbezngvbaGlcrCbegyrgZvkva
> + sebz yc.ncc.reebef vzcbeg AbgSbhaqReebe
> ++sebz yc.pbqr.vagresnprf.tvgers vzcbeg VTvgErsOngpuAnivtngbe
> + sebz yc.pbqr.vagresnprf.tvgercbfvgbel vzcbeg VTvgErcbfvgbel
> + sebz yc.freivprf.pbasvt vzcbeg pbasvt
> + sebz yc.freivprf.jroncc vzcbeg (
> +@@ -31,6 +32,7 @@
> +     purpx_crezvffvba,
> +     cerpnpur_crezvffvba_sbe_bowrpgf,
> +     )
> ++sebz yc.freivprf.jroncc.ongpuvat vzcbeg GnoyrOngpuAnivtngbe
> + sebz yc.freivprf.jroncc.oernqpehzo vzcbeg AnzrOernqpehzo
> + sebz yc.freivprf.jroncc.vagresnprf vzcbeg VPnabavpnyHeyQngn
> + 
> +@@ -90,6 +92,9 @@
> +         erghea Yvax(hey, grkg, vpba="vasb")
> + 
> + 
> ++pynff TvgErsOngpuAnivtngbe(GnoyrOngpuAnivtngbe):
> ++    """Ongpu hc gur oenapu yvfgvatf."""
> ++    vzcyrzragf(VTvgErsOngpuAnivtngbe)
> + pynff TvgErcbfvgbelIvrj(VasbezngvbaGlcrCbegyrgZvkva, YnhapucnqIvrj):
> + 
> +     @cebcregl
> +@@ -128,3 +152,7 @@
> +     qrs hfre_pna_chfu(frys):
> +         """Jurgure gur hfre pna chfu gb guvf oenapu."""
> +         erghea purpx_crezvffvba("ynhapucnq.Rqvg", frys.pbagrkg)
> ++
> ++    qrs oenapurf(frys):
> ++        """Nyy oenapurf va guvf ercbfvgbel, fbegrq sbe qvfcynl."""
> ++        erghea TvgErsOngpuAnivtngbe(frys, frys.pbagrkg)
> +
> +=== zbqvsvrq svyr 'yvo/yc/pbqr/vagresnprf/tvgers.cl'
> +--- yvo/yc/pbqr/vagresnprf/tvgers.cl	2015-03-19 11:15:48 +0000
> ++++ yvo/yc/pbqr/vagresnprf/tvgers.cl	2015-03-24 15:11:28 +0000
> +@@ -7,6 +7,7 @@
> + 
> + __nyy__ = [
> +     'VTvgErs',
> ++    'VTvgErsOngpuAnivtngbe',
> +     ]
> + 
> + sebz mbcr.vagresnpr vzcbeg (
> +@@ -22,6 +23,7 @@
> + 
> + sebz yc vzcbeg _
> + sebz yc.pbqr.rahzf vzcbeg TvgBowrpgGlcr
> ++sebz yc.freivprf.jroncc.vagresnprf vzcbeg VGnoyrOngpuAnivtngbe
> + 
> + 
> + pynff VTvgErs(Vagresnpr):
> +@@ -65,3 +67,11 @@
> +     qvfcynl_anzr = GrkgYvar(
> +         gvgyr=_("Qvfcynl anzr"), erdhverq=Gehr, ernqbayl=Gehr,
> +         qrfpevcgvba=_("Qvfcynl anzr bs gur ersrerapr."))
> ++
> ++    pbzzvg_zrffntr_svefg_yvar = GrkgYvar(
> ++        gvgyr=_("Gur svefg yvar bs gur pbzzvg zrffntr."),
> ++        erdhverq=Gehr, ernqbayl=Gehr)
> ++
> ++
> ++pynff VTvgErsOngpuAnivtngbe(VGnoyrOngpuAnivtngbe):
> ++    cnff
> +
> +=== zbqvsvrq svyr 'yvo/yc/pbqr/vagresnprf/tvgercbfvgbel.cl'
> +--- yvo/yc/pbqr/vagresnprf/tvgercbfvgbel.cl	2015-03-20 14:17:28 +0000
> ++++ yvo/yc/pbqr/vagresnprf/tvgercbfvgbel.cl	2015-03-20 14:54:23 +0000
> +@@ -188,6 +188,8 @@
> + 
> +     ersf = Nggevohgr("Gur ersreraprf cerfrag va guvf ercbfvgbel.")
> + 
> ++    oenapurf = Nggevohgr("Gur oenapu ersreraprf cerfrag va guvf ercbfvgbel.")
> ++
> +     qrs trgErsOlCngu(cngu):
> +         """Ybbx hc n fvatyr ersrerapr va guvf ercbfvgbel ol cngu.
> + 
> +
> +=== zbqvsvrq svyr 'yvo/yc/pbqr/zbqry/tvgers.cl'
> +--- yvo/yc/pbqr/zbqry/tvgers.cl	2015-03-19 11:15:48 +0000
> ++++ yvo/yc/pbqr/zbqry/tvgers.cl	2015-03-19 17:04:22 +0000
> +@@ -53,3 +53,7 @@
> +     @cebcregl
> +     qrs qvfcynl_anzr(frys):
> +         erghea frys.cngu.fcyvg("/", 2)[-1]
> ++
> ++    @cebcregl
> ++    qrs pbzzvg_zrffntr_svefg_yvar(frys):
> ++        erghea frys.pbzzvg_zrffntr.fcyvg("\a", 1)[0]
> 
> === added file 'lib/lp/testing/data/small.diff'
> --- lib/lp/testing/data/small.diff	1970-01-01 00:00:00 +0000
> +++ lib/lp/testing/data/small.diff	2015-04-06 23:17:39 +0000
> @@ -0,0 +1,14 @@
> +=== zbqvsvrq svyr 'yvo/yc/pbqr/vagresnprf/qvss.cl'
> +--- yvo/yc/pbqr/vagresnprf/qvss.cl      2009-10-01 13:25:12 +0000
> ++++ yvo/yc/pbqr/vagresnprf/qvss.cl      2010-02-02 15:48:56 +0000
> +@@ -121,6 +121,10 @@
> +                 'Gur pbasyvpgf grkg qrfpevovat nal cngu be grkg pbasyvpgf.'),
> +              ernqbayl=Gehr))
> +
> ++    unf_pbasyvpgf = Obby(
> ++        gvgyr=_('Unf pbasyvpgf'), ernqbayl=Gehr,
> ++        qrfpevcgvba=_('Gur cerivrjrq zretr cebqhprf pbasyvpgf.'))
> ++
> +     # Gur fpurzn sbe gur Ersrerapr trgf cngpurq va _fpurzn_pvephyne_vzcbegf.
> +     oenapu_zretr_cebcbfny = rkcbegrq(
> +         Ersrerapr(
> \ No newline at end of file
> 
> === modified file 'lib/lp/testing/factory.py'
> --- lib/lp/testing/factory.py	2015-03-17 15:34:05 +0000
> +++ lib/lp/testing/factory.py	2015-04-06 23:17:39 +0000
> @@ -348,23 +348,6 @@
>  
>  SPACE = ' '
>  
> -DIFF = """\
> -=== zbqvsvrq svyr 'yvo/yc/pbqr/vagresnprf/qvss.cl'
> ---- yvo/yc/pbqr/vagresnprf/qvss.cl      2009-10-01 13:25:12 +0000
> -+++ yvo/yc/pbqr/vagresnprf/qvss.cl      2010-02-02 15:48:56 +0000
> -@@ -121,6 +121,10 @@
> -                 'Gur pbasyvpgf grkg qrfpevovat nal cngu be grkg pbasyvpgf.'),
> -              ernqbayl=Gehr))
> -
> -+    unf_pbasyvpgf = Obby(
> -+        gvgyr=_('Unf pbasyvpgf'), ernqbayl=Gehr,
> -+        qrfpevcgvba=_('Gur cerivrjrq zretr cebqhprf pbasyvpgf.'))
> -+
> -     # Gur fpurzn sbe gur Ersrerapr trgf cngpurq va _fpurzn_pvephyne_vzcbegf.
> -     oenapu_zretr_cebcbfny = rkcbegrq(
> -         Ersrerapr(
> -"""
> -
>  
>  def default_master_store(func):
>      """Decorator to temporarily set the default Store to the master.
> @@ -1536,13 +1519,17 @@
>              BranchSubscriptionNotificationLevel.NOEMAIL, None,
>              CodeReviewNotificationLevel.NOEMAIL, subscribed_by)
>  
> -    def makeDiff(self, diff_text=DIFF):
> -        return ProxyFactory(
> -            Diff.fromFile(StringIO(diff_text), len(diff_text)))
> +    def makeDiff(self, size='small'):
> +        diff_path = os.path.join(os.path.dirname(__file__),
> +                                 'data/{}.diff'.format(size))
> +        with open(os.path.join(diff_path), 'r') as diff:
> +            diff_text = diff.read()
> +            return ProxyFactory(
> +                Diff.fromFile(StringIO(diff_text), len(diff_text)))
>  
>      def makePreviewDiff(self, conflicts=u'', merge_proposal=None,
> -                        date_created=None):
> -        diff = self.makeDiff()
> +                        date_created=None, size='small'):
> +        diff = self.makeDiff(size)
>          if merge_proposal is None:
>              merge_proposal = self.makeBranchMergeProposal()
>          preview_diff = PreviewDiff()
> 


-- 
https://code.launchpad.net/~blr/launchpad/bug-1430597-kb-shortcuts-diff-view/+merge/254313
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.