Add shortcut (arrow up/down) to focus on next/previous line in side-by-side view.

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

[gvrpython, 2012-02-20 19:08:16]

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)
On Feb 20, 2012 10:39 AM, <albrecht.andi@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/<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>
>
>
>

[Andi Albrecht, 2012-02-20 19:21:09]

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

>
> --Guido van Rossum (sent from Android phone)
>
> On Feb 20, 2012 10:39 AM, <albrecht.andi@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>
>>
>>
>

[Andi Albrecht, 2012-02-21 04:52:46]

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

[Andi Albrecht, 2012-02-22 18:40:48]

Is the version with arrow keys better?

--
Andi

On Tue, Feb 21, 2012 at 5:52 AM,  <albrecht.andi@googlemail.com> wrote:
> 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/

[GvR, 2012-02-24 04:19:24]

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. :-(

[Andi Albrecht, 2012-02-24 05:03:21]

On Fri, Feb 24, 2012 at 5:19 AM,  <gvanrossum@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/

[gvrpython, 2012-02-24 15:46:41]

On Thu, Feb 23, 2012 at 9:03 PM, Andi Albrecht
<albrecht.andi@googlemail.com> wrote:
> On Fri, Feb 24, 2012 at 5:19 AM,  <gvanrossum@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*

- 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, 2012-02-25 17:25:17]

Thanks for the thorough testing!

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

[gvrpython, 2012-02-26 03:10:55]

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

On Sat, Feb 25, 2012 at 9:25 AM, Andi Albrecht
<albrecht.andi@googlemail.com> wrote:
> Thanks for the thorough testing!
>
> On Fri, Feb 24, 2012 at 4:46 PM, Guido van Rossum <guido@python.org> wrote:
>> On Thu, Feb 23, 2012 at 9:03 PM, Andi Albrecht
>> <albrecht.andi@googlemail.com> wrote:
>>> On Fri, Feb 24, 2012 at 5:19 AM,  <gvanrossum@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)

[Andi Albrecht, 2012-02-26 07:32:26]

On Sun, Feb 26, 2012 at 4:10 AM, Guido van Rossum <guido@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).

>
> On Sat, Feb 25, 2012 at 9:25 AM, Andi Albrecht
> <albrecht.andi@googlemail.com> wrote:
>> Thanks for the thorough testing!
>>
>> On Fri, Feb 24, 2012 at 4:46 PM, Guido van Rossum <guido@python.org> wrote:
>>> On Thu, Feb 23, 2012 at 9:03 PM, Andi Albrecht
>>> <albrecht.andi@googlemail.com> wrote:
>>>> On Fri, Feb 24, 2012 at 5:19 AM,  <gvanrossum@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.
>

[gvrpython, 2012-02-26 15:38:59]

On Sat, Feb 25, 2012 at 11:32 PM, Andi Albrecht
<albrecht.andi@googlemail.com> wrote:
> On Sun, Feb 26, 2012 at 4:10 AM, Guido van Rossum <guido@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.

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

[M-A, 2012-03-02 18:20:19]

FTR, I don't feel qualified to review javascript/html. I don't have an opinion
on the change, I'd only prefer to have lines wrapped at 80 cols.

[Andi Albrecht, 2012-03-17 07:53:21]

How to proceed with this issue?

Adding codereview-discuss in the hope that there's someone on the list
with JavaScript knowledge to have a look at the patch.

--
Andi

On Sat, Mar 3, 2012 at 10:06 PM,  <albrecht.andi@googlemail.com> wrote:
> http://codereview.appspot.com/5685057/

[gvrpython, 2012-03-17 14:58:44]

So just check it in!

On Sat, Mar 17, 2012 at 12:53 AM, Andi Albrecht
<albrecht.andi@googlemail.com> wrote:
> How to proceed with this issue?
>
> Adding codereview-discuss in the hope that there's someone on the list
> with JavaScript knowledge to have a look at the patch.
>
> --
> Andi
>
> On Sat, Mar 3, 2012 at 10:06 PM,  <albrecht.andi@googlemail.com> wrote:
>> http://codereview.appspot.com/5685057/



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

[techtonik, 2012-03-17 16:48:46]

I am not a user for this feature, neither do I have time to give a proper
review, so I'm +1 on committing this as is, because it doesn't seem to conflict
with anything else, and let users send further comments.

http://codereview.appspot.com/5685057/diff/15001/templates/base.html
File templates/base.html (right):

http://codereview.appspot.com/5685057/diff/15001/templates/base.html#newcode107
templates/base.html:107: <td class="shortcut"><span class="letter">a</span> /
<span class="letter">d</span> <b>:</b></td><td>next / previous line</td>
Did you forget to change this to Up/Down?

[Andi Albrecht, 2012-03-18 07:12:21]

Thanks for the review!

This version is now live.

http://codereview.appspot.com/5685057/diff/15001/templates/base.html
File templates/base.html (right):

http://codereview.appspot.com/5685057/diff/15001/templates/base.html#newcode107
templates/base.html:107: <td class="shortcut"><span class="letter">a</span> /
<span class="letter">d</span> <b>:</b></td><td>next / previous line</td>
On 2012/03/17 16:48:46, techtonik wrote:
> Did you forget to change this to Up/Down?

hmpf, yes... In the first iteration this was a/d... Thanks!