launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #18243
Re: [Merge] lp:~blr/launchpad/bug-1430597-kb-shortcuts-diff-view into lp:launchpad
Review: Needs Fixing
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) {
This looks like it will misbehave if your first keypress is K or P, because the scroll states are initialised to -1, so this condition is initially false. Perhaps the initialisers should be 0 instead, to fix this and to arrange that an initial press of J or N takes you to the second file/hunk?
Also, perhaps both these branches would be both briefer and clearer if rephrased using mod, something like this (with a little care to avoid having to think about semantics around negative numbers):
var offset;
if (direction == 'forward') {
offset = 1;
}
else {
offset = numHeaders - 1;
}
namespace.scrollState[type] = (namespace.scrollState[type] + offset) % numHeaders;
> + 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'});
Is there any way to update the position of the other scroll state, so that for example you can hit J a few times and then hit N to navigate by hunk within the file you've reached rather than surprisingly jumping back up? In fact, it would be even better if the scroll states also updated when scrolling the page by other mechanisms, although I appreciate that may be harder.
> +};
> +
> +/*
> + * 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')) {
The indentation is confusing here; an extra two spaces at the start would help.
> + 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:
No space before the open paren here.
> + 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.
References
