Add a/d shortcut to focus on next/previous line in side-by-side view. (issue 5685057)

[albrec...@googlemail.com]

Reviewers: GvR, M-A,

Description:
I often use "n" to jump to a hook ("hunk" in diff terms) but then
Rietveld lacks a shortcut to move the indicator line-by-line. So I
have to fall back to use the mouse to double-click a line to add
comments.

This change adds a/d shortcuts to move the indicator to the next/prev
line in side-by-side view. The difficulty here is that I had to add
"temporary" hooks on the JavaScript side that become "real" hooks when
a comment is added. This is to not break the n/p movement between
hooks.

This version is live here: http://rvtests.appspot.com
(Don't forget to reload to get the latest script.js and to login to
leave comments by pressing enter.)

Please review this at http://codereview.appspot.com/5685057/

Affected files:
M static/script.js
M templates/base.html

Index: static/script.js
===================================================================
--- a/static/script.js
+++ b/static/script.js
@@ -1528,6 +1528,12 @@
this.indicator = document.getElementById("hook-sel");

/**
+ * The element the indicator points to
+ * @type Element
+ */
+ this.indicated_element = null;
+
+ /**
* Caches whether we are in an IE browser
* @type Boolean
*/
@@ -1579,7 +1585,9 @@
*/
M_HookState.prototype.updateHooks = function() {
var curHook = null;
- if (this.hookPos >= 0 && this.hookPos < this.visibleHookCache.length) {
+ if (this.indicated_element != null) {
+ curHook = this.indicated_element;
+ } else if (this.hookPos >= 0 && this.hookPos <
this.visibleHookCache.length) {
curHook = this.visibleHookCache[this.hookPos];
}
this.computeHooks_();
@@ -1619,6 +1627,7 @@
this.indicator.style.left = "0px";
this.indicator.style.width = totWidth + "px";
this.indicator.style.display = "";
+ this.indicated_element = tr;
};

/**
@@ -1633,6 +1642,7 @@

// Hide the current selection image
this.indicator.style.display = "none";
+ this.indicated_element = null;

// Add a border to all td's in the selected row
if (this.hookPos < -1) {
@@ -1674,6 +1684,38 @@
}
};

+
+/**
+ * Update the indicator and hook position by moving to the next/prev line.
+ * If the target line doesn't have a hook marker, the marker is added.
+ * @param {Integer} direction Scroll direction: -1 for up, 1 for down.
+ */
+M_HookState.prototype.gotoLine = function(direction) {
+ var thecode = document.getElementById("thecode").rows;
+ // find current hook and store visible code lines
+ var currHook = this.indicated_element;
+ var hookIdx = -1;
+ var codeRows = new Array();
+ for (var i=0; i < thecode.length; i++) {
+ if (thecode[i].id.substr(0, 4) == "pair") {
+ codeRows.push(thecode[i]);
+ if (currHook && thecode[i].id == currHook.id) {
+ hookIdx = codeRows.length - 1;
+ }
+ }
+ }
+ if (direction > 0) {
+ hookIdx = Math.min(hookIdx + 1, codeRows.length - 1);
+ } else {
+ hookIdx = Math.max(hookIdx - 1, 0);
+ }
+ var tr = codeRows[hookIdx];
+ if (tr) {
+ this.updateIndicator_(tr);
+ }
+}
+
+
/**
* Set this.hookPos to the next desired hook.
* @param {Boolean} findComment Whether to look only for comment hooks
@@ -1819,6 +1861,11 @@
* the draft if there is one already. Prefer the right side of the table.
*/
M_HookState.prototype.respond = function() {
+ if (this.indicated_element && !
this.indicated_element.getAttribute("name") != "hook") {
+ // Turn indicated element into a "real" hook so we can add comments.
+ this.indicated_element.setAttribute("name", "hook")
+ }
+ this.updateHooks();
var hooks = this.visibleHookCache;
if (this.hookPos >= 0 && this.hookPos < hooks.length &&
M_isElementVisible(this.win, hooks[this.hookPos].cells[0])) {
@@ -2144,6 +2191,10 @@
} else if (key == 'Shift-P') {
// previous comment
if (hookState) hookState.gotoPrevHook(true);
+ } else if (key == 'A') {
+ if (hookState) hookState.gotoLine(1);
+ } else if (key == 'D') {
+ if (hookState) hookState.gotoLine(-1);
} else if (key == 'J') {
// next file
M_jumpToHrefOrChangelist('nextFile')
Index: templates/base.html
===================================================================
--- a/templates/base.html
+++ b/templates/base.html
@@ -104,6 +104,9 @@
<td class="shortcut"><span class="letter">N</span> / <span
class="letter">P</span> <b>:</b></td><td>next / previous comment</td>
</tr>
<tr>
+ <td class="shortcut"><span class="letter">a</span> / <span
class="letter">d</span> <b>:</b></td><td>next / previous line</td>
+ </tr>
+ <tr>
<td class="shortcut"><span class="letter">&lt;Enter&gt;</span>
<b>:</b></td><td>respond to / edit current comment</td>
</tr>
</table>

[Guido van Rossum]

Hmmm... In Mondrian we used up/down arrows for this. It would move the blue line and scroll 1 line. And IIRC o to open a comment (or c?).

--Guido van Rossum (sent from Android phone)

[Andi Albrecht]

Initially I thought about using the arrow keys too, but then I didn't
want to change the default behavior in browsers (move page up and
down).

Now that I read your comment, it sounds much like that my concerns
don't count in real life :)
It should be possible to modify the change to use arrow keys.

On Mon, Feb 20, 2012 at 8:08 PM, Guido van Rossum <gu...@python.org> wrote:
> Hmmm... In Mondrian we used up/down arrows for this. It would move the blue
> line and scroll 1 line. And IIRC o to open a comment (or c?).

In Rietveld we use the Enter key to comment, c to collcapse all
comments o isn't used on side-by-side view.

--
Andi

[albrec...@googlemail.com]

http://codereview.appspot.com/5685057/

[albrec...@googlemail.com]

On 2012/02/21 04:50:06, Andi Albrecht wrote:

hm, it seems that upload.py's "-m" flag doesn't work as expected when
uploading a new changeset to an issue... :(

The message should have been: Please have another look. This version
(with arrow keys instead of a/d) is now live at
http://rvtests.appspot.com

http://codereview.appspot.com/5685057/

[Andi Albrecht]

Is the version with arrow keys better?

--
Andi

[gvanr...@gmail.com]

Looking at the test site I see two problems:

1. When you hit DOWN enough times the blue line goes off the page. In
Mondrian we solved this by simultaneously scrolling the window up by the
same amount (so the blue line stays put relative to the window but moves
down relative to the text).

2. Mixing DOWN and 'n' is confusing -- if you use DOWN to go past the
next chunk, 'n' moves the blue line *up*, apparently it doesn't keep
track of where you are on the page.

Yes, JavaScript sucks. :-(

http://codereview.appspot.com/5685057/

[Andi Albrecht]

On Fri, Feb 24, 2012 at 5:19 AM, <gvanr...@gmail.com> wrote:
> Looking at the test site I see two problems:
>
> 1. When you hit DOWN enough times the blue line goes off the page.  In
> Mondrian we solved this by simultaneously scrolling the window up by the
> same amount (so the blue line stays put relative to the window but moves
> down relative to the text).

Done. Position of the viewport is now updated when up/down key is pressed.

>
> 2. Mixing DOWN and 'n' is confusing -- if you use DOWN to go past the
> next chunk, 'n' moves the blue line *up*, apparently it doesn't keep
> track of where you are on the page.

Thanks! I've missed that.

An updated version with both changes is now live on my testing
instance. The changeset is updated accordingly.

>
> Yes, JavaScript sucks. :-(

agreed... you can test for hours and there's still strange behavior or
even worse: a questionable internal state of variables :)

--
Andi

>
> http://codereview.appspot.com/5685057/

[Guido van Rossum]

Much better. I found some odd anomalies (off-by-one errors?):

- using DOWN I cannot reach the lowest point where I can get with 'n'
or 'N' (between the last line of the file and the table footer saying
OLD ... NEW)

- after going all the way down with 'N', UP sends me to the top of the
file except one line up

- using UP I cannot reach the highest point (*above* the OLD ... NEW
table header)

- from that highest point, UP goes one line *down*

- it's a little jarring that hitting DOWN from the highest point
scrolls the viewport so much

--
--Guido van Rossum (python.org/~guido)

[Andi Albrecht]

Thanks for the thorough testing!

All of the above came from not handling the edge cases correctly.

>
> - it's a little jarring that hitting DOWN from the highest point
> scrolls the viewport so much

I've addressed the edge cases and limited the scrolling to when it's
really needed in my latest changeset. My testing instance runs that
latest version.

--
Andi

[Guido van Rossum]

Looks great, except the viewport now no longer adjusts when you move
the blue line. I'm not sure if I care though. But I really hope
someone else can review the JS code for style and substance...

> --
> You received this message because you are subscribed to the Google Groups "codereview-list" group.
> To post to this group, send an email to coderev...@googlegroups.com.
> To unsubscribe from this group, send email to codereview-li...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/codereview-list?hl=en-GB.

[Andi Albrecht]

On Sun, Feb 26, 2012 at 4:10 AM, Guido van Rossum <gu...@python.org> wrote:
> Looks great, except the viewport now no longer adjusts when you move
> the blue line. I'm not sure if I care though. But I really hope
> someone else can review the JS code for style and substance...

When doesn't the viewport adjust correctly? In the last version I've
changed that when moving the blue line with up/down the viewport only
adjusts when the blue line moves out of the viewport (in contrast to
the previous version where the viewport was adjusted with every
up/down move).

[Guido van Rossum]

On Sat, Feb 25, 2012 at 11:32 PM, Andi Albrecht
<albrec...@googlemail.com> wrote:
> On Sun, Feb 26, 2012 at 4:10 AM, Guido van Rossum <gu...@python.org> wrote:
>> Looks great, except the viewport now no longer adjusts when you move
>> the blue line. I'm not sure if I care though. But I really hope
>> someone else can review the JS code for style and substance...
>
> When doesn't the viewport adjust correctly? In the last version I've
> changed that when moving the blue line with up/down the viewport only
> adjusts when the blue line moves out of the viewport (in contrast to
> the previous version where the viewport was adjusted with every
> up/down move).

Either way is fine with me. I notice Mondrian has the behavior of your
previous version in this respect.