> 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/<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 (sent from Android phone)

> On Feb 20, 2012 10:39 AM, <albrecht.a...@googlemail.com> wrote:

>> 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>

> 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/

> 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/

>> 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 :)

>>> 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 :)

> 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)

> On Fri, Feb 24, 2012 at 4:46 PM, Guido van Rossum <gu...@python.org> wrote:
>> On Thu, Feb 23, 2012 at 9:03 PM, Andi Albrecht
>> <albrecht.a...@googlemail.com> wrote:
>>> On Fri, Feb 24, 2012 at 5:19 AM,  <gvanros...@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 :)

>> 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*

> 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 (python.org/~guido)

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

> On Sat, Feb 25, 2012 at 9:25 AM, Andi Albrecht
> <albrecht.a...@googlemail.com> wrote:
>> Thanks for the thorough testing!

>> On Fri, Feb 24, 2012 at 4:46 PM, Guido van Rossum <gu...@python.org> wrote:
>>> On Thu, Feb 23, 2012 at 9:03 PM, Andi Albrecht
>>> <albrecht.a...@googlemail.com> wrote:
>>>> On Fri, Feb 24, 2012 at 5:19 AM,  <gvanros...@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 :)

>>> 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*

>> 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 (python.org/~guido)

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

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

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

> 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).