launchpad-reviewers team mailing list archive

[Bayard 'kit' Randel <kit.randel@xxxxxxxxxxxxx>]

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-01 10:37:35 +0000
> @@ -604,6 +604,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-01 10:37:35 +0000
> @@ -9,6 +9,16 @@
>  
>  YUI.add('lp.code.branchmergeproposal.inlinecomments', function(Y) {
>  
> +if (typeof KeyEvent == "undefined") {
> +    var 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 === 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,7 +714,63 @@
>  
>  });
>  
> +namespace.scrollState = {'files': -1, 'hunks': -1};
> +namespace.diffHeaders = {'files' : Y.all('td.diff-file'),
> +                         'hunks' : Y.all('td.diff-chunk')};
> +
> +/*
> + * Move viewport top to hunk or file header.
> + */
> +namespace.updateScrollState = function(type, direction) {
> +    var numHeaders = namespace.diffHeaders[type].size();
> +    if (numHeaders <= 1) { return; }
> +    if (direction == 'forward') {
> +        if (namespace.scrollState[type] + 1 >= numHeaders) {
> +            namespace.scrollState[type] = -1;
> +        }
> +        namespace.scrollState[type] += 1;
> +    }
> +    else if (direction == 'backwards') {
> +        if (namespace.scrollState[type] - 1 == -1) {

The keybindings are global for the page, so I do think the right behaviour is probably still to scroll to the first diff/hunk (0) i.e. hitting J at the top of the page, sets scroll state to the first element (0) and scrolls the viewport to the top of the diff viewer. I think it would be potentially confusing to have it jump the first file/chunk if the preview diff is initially outside the viewport.

> +            namespace.scrollState[type] = numHeaders;
> +        }
> +        namespace.scrollState[type] -= 1;
> +    }
> +    var header = namespace.diffHeaders[type].item(namespace.scrollState[type]);
> +    // XXX blr 20150326: node must be re-initialised with Y.one or scrollIntoView silently
> +    // fails. yui3 bug?
> +    var row = Y.one('tr#' + header.ancestor().get('id'));
> +    row.scrollIntoView({block: 'start', behavior: 'smooth'});

Yes, imagine that is possible, but will need to build a map of files with their child hunks.

> +};
> +
> +/*
> + * 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 === KeyEvent.DOM_VK_J) {
> +            namespace.updateScrollState('files', 'forward');
> +        }
> +        if (e.button === KeyEvent.DOM_VK_K) {
> +            namespace.updateScrollState('files', 'backwards');
> +        }
> +        if (e.button === KeyEvent.DOM_VK_N) {
> +            namespace.updateScrollState('hunks', 'forward');
> +        }
> +        if (e.button === KeyEvent.DOM_VK_P) {
> +            namespace.updateScrollState('hunks', 'backwards');
> +        }
> +    }
> +};
> +
> +var diffNavKeys = [KeyEvent.DOM_VK_J, KeyEvent.DOM_VK_K,
> +                   KeyEvent.DOM_VK_N, KeyEvent.DOM_VK_P];
> +Y.one('body').on('key', namespace.handleNavKeys, 'down:' + diffNavKeys.join(','));
> +
>  namespace.DiffNav = DiffNav;
>  
> -}, '0.1', {requires: ['datatype-date', 'event', 'io', 'node', 'widget',
> +}, '0.1', {requires: ['datatype-date', 'event', 'io', 'node', 'widget', 
>                        'lp.client', 'lp.ui.editor', 'lp.app.date']});
> 
> === 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-01 10:37:35 +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-01 10:37:35 +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-01 10:37:35 +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-01 10:37:35 +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-01 10:37:35 +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.