launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #18217
[Merge] lp:~blr/launchpad/bug-1430597-kb-shortcuts-diff-view into lp:launchpad
Bayard 'kit' Randel has proposed merging lp:~blr/launchpad/bug-1430597-kb-shortcuts-diff-view into lp:launchpad.
Requested reviews:
William Grant (wgrant)
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1430597 in Launchpad itself: "Provide keyboard shortcuts for navigating diffs"
https://bugs.launchpad.net/launchpad/+bug/1430597
For more details, see:
https://code.launchpad.net/~blr/launchpad/bug-1430597-kb-shortcuts-diff-view/+merge/254313
Provides hunk and file navigation keybindings for preview diffs on the branch merge proposal view.
Additionally, the makePreviewDiff factory has learned a 'size' argument for generating large diffs with multiple hunks.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~blr/launchpad/bug-1430597-kb-shortcuts-diff-view into lp:launchpad.
=== 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-03-27 00:26:45 +0000
@@ -118,7 +118,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 +705,61 @@
});
+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) {
+ 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'});
+};
+
+/*
+ * 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 === 74) {
+ namespace.updateScrollState('files', 'forward');
+ }
+ if (e.button === 75) {
+ namespace.updateScrollState('files', 'backwards');
+ }
+ if (e.button === 78) {
+ namespace.updateScrollState('hunks', 'forward');
+ }
+ if (e.button === 80) {
+ namespace.updateScrollState('hunks', 'backwards');
+ }
+ }
+};
+
+Y.one('body').on('key', namespace.handleNavKeys, 'down:74,75,78,80');
+
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-03-27 00:26:45 +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-03-27 00:26:45 +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-03-27 00:26:45 +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-03-27 00:26:45 +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-03-27 00:26:45 +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()
Follow ups
