Implement word wrapping and panning in multiline Braille.

chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 /** @fileoverview Logic for panning a braille display within a line of braille
  * content that might not fit on a single display.
  */
 
 goog.provide('cvox.PanStrategy');
 
 /**
  * @constructor
  *
  * A stateful class that keeps track of the current 'viewport' of a braille
  * display in a line of content.
  */
 cvox.PanStrategy = function() {
   /**
    * @type {number}
    * @private
    */
-  this.displaySize_ = 0;
+  this.rowCount_ = 0;
   /**
    * @type {number}
    * @private
    */
-  this.contentLength_ = 0;
+  this.columnCount_ = 0;
   /**
-   * Points before which it is desirable to break content if it doesn't fit
-   * on the display.
-   * @type {!Array<number>}
-   * @private
-   */
-  this.breakPoints_ = [];
-  /**
+   * Start and end are both inclusive.
    * @type {!cvox.PanStrategy.Range}
    * @private
    */
   this.viewPort_ = {start: 0, end: 0};
+
+  /**
+   * Start and end are both inclusive.
+   * @type {ArrayBuffer}
+   * @private
+   */
+  this.wrappedBuffer_ = new ArrayBuffer(0);
+
+  /**
+   * Start and end are both inclusive.
+   * @type {ArrayBuffer}
+   * @private
+   */
+  this.fixedBuffer_ = new ArrayBuffer(0);
+
+  /**
+   * Start and end are both inclusive.
+   * @type {Array}

[David Tseng, 2016/11/15 17:55:00]

Array<number>

[ultimatedbz, 2016/11/19 03:11:59]

Done.

+   * @private
+   */
+  this.wrappedBrailleToText_ = [];
+
+  /**
+   * Start and end are both inclusive.

[David Tseng, 2016/11/15 17:55:00]

Fix these comments.

[ultimatedbz, 2016/11/19 03:11:59]

Done.

+   * @type {Array}
+   * @private
+   */
+  this.fixedBrailleToText_ = [];
+
+  /**
+   * Start and end are both inclusive.
+   * @type {boolean}
+   * @private
+   */
+  this.panStrategyWrapped_ = false;
+
+  /**
+   * Start and end are both inclusive.
+   * @type {number}
+   * @private
+   */
+  this.fixedLineCount_ = 0;

[David Tseng, 2016/11/16 16:56:01]

You don't need this (and the below) members. Just compute them on the fly based
on the arrays above. For ease of calling, you could make them read only getters
(similarly to the way you have offsetsForSlices below).

[ultimatedbz, 2016/11/19 03:11:58]

Done.

+
+  /**
+   * Start and end are both inclusive.
+   * @type {number}
+   * @private
+   */
+  this.wrappedLineCount_ = 0;
 };
 
 /**
  * A range used to represent the viewport with inclusive start and xclusive
  * end position.
  * @typedef {{start: number, end: number}}
  */
 cvox.PanStrategy.Range;
 
 cvox.PanStrategy.prototype = {
   /**
    * Gets the current viewport which is never larger than the current
    * display size and whose end points are always within the limits of
    * the current content.
    * @type {!cvox.PanStrategy.Range}
    */
   get viewPort() {
     return this.viewPort_;
   },
 
   /**
+   * @return {Array<number>} The offset of braille and text indices of the

[David Tseng, 2016/11/15 17:55:00]

Make this an Object e.g. {brailleOffset: number, textOffset: nuimber}

[ultimatedbz, 2016/11/19 03:11:58]

Done.

+   *    current slice.
+   */
+  get offsetsForSlices() {
+    return [this.viewPort_.start * this.columnCount_,
+    this.brailleToText[this.viewPort_.start * this.columnCount_]];
+  },
+
+  /**
+   * @return {Array<number>} The map of Braille cells to the first index of the
+   *    corresponding text character.

[David Tseng, 2016/11/15 17:55:00]

@return {Array<number}

[ultimatedbz, 2016/11/19 03:11:59]

I'm not sure what you're suggesting? Thanks!

+   */
+  get brailleToText() {
+    if (this.panStrategyWrapped_)
+      return this.wrappedBrailleToText_;
+    else
+      return this.fixedBrailleToText_;
+  },
+
+  /**
+   * @return {ArrayBuffer} Buffer of the slice of braille cells within the
+   *    bounds of the viewport.
+   */
+  getCurrentBrailleSlice: function() {
+    var buf = this.panStrategyWrapped_ ?
+        this.wrappedBuffer_ : this.fixedBuffer_;

[David Tseng, 2016/11/15 17:54:59]

Part of why it was nice to have different classes for the two strategies is the
need to write code like this and the need to keep around two members per
strategy type. Wrapping vs fixed is controlled by an option in the ChromeVox
settings and is unlikely to change frequently.

[ultimatedbz, 2016/11/19 03:11:59]

As per our offline conversation, we'll be keeping the code as is for this
iteration, but in the future when we add more panning strategies, we would have
different classes to maintain readability. 

+    buf = buf.slice(this.viewPort.start * this.columnCount_,

[David Tseng, 2016/11/15 17:54:59]

return buff.slice...

[ultimatedbz, 2016/11/19 03:11:59]

Done.

+        (this.viewPort.end + 1) * this.columnCount_);

[David Tseng, 2016/11/15 17:55:00]

Why not just make the viewport index into the buffers? e.g. viewport.end = pos *
columnCount. I'm actually not realy understanding what viewport.star tand
viewport.end are supposed to mean at first; perhaps rename to rowStart, rowEnd.

[ultimatedbz, 2016/11/19 03:11:59]

How about firstRow and lastRow?

+    return buf;
+  },
+
+  /**
+   * @return {string} String of the slice of text letters corresponding with
+   *    the current braille slice.

[David Tseng, 2016/11/15 17:54:59]

@return {Array<number>}

[David Tseng, 2016/11/16 16:56:01]

Disregard.

+   */
+  getCurrentTextSlice: function() {
+    var brailleToText = this.brailleToText;
+    // Index of last braille character in slice.
+    var index = (this.viewPort.end + 1) * this.columnCount_ - 1;

[David Tseng, 2016/11/15 17:54:59]

This just looks wrong (mostly the viewport.end + 1 expression).

[ultimatedbz, 2016/11/15 19:31:57]

Right, so I know it looks a bit janky, but this.viewPort.end is inclusive. So,
if our viewPort only has one row, this.viewPort.start would be 0, and
this.viewPort.end would also be 0. I should note that viewPort.start and
viewPort.end are line indices, not character indices.

Here, index is the index of the last braille character in the slice.
this.viewPort.end + 1 gets us the next line number. (this.viewPort.end +1) *
this.columnCount_ gets us the first character of the line after end. and that, -
1, gets us the last character in the slice.

I must say though, that even though it looks wrong, the code surprisingly works!

Now that I look at this again though, I think I can make this simpler by writing
'var index = this.viewPort.start + this.rowCount_ * this.columnCount_ - 1;'

[David Tseng, 2016/11/15 21:36:38]

pan_strategy_test.unitjs awaits; I am generally ok with somewhat unreadable code
as long as it is well covered by tests.

+    // Index of first text character that the last braille character points to.
+    var end = brailleToText[index];
+    // Increment index until brailleToText[index] points to a different char.
+    // This is the cutoff point, as substring cuts up to, but not including,
+    // brailleToText[index].
+    while (index < brailleToText.length && brailleToText[index] == end) {
+      index++;
+    }

[David Tseng, 2016/11/15 17:54:59]

nit: no curlies for single lined if bodies.

[David Tseng, 2016/11/15 21:36:38]

Ignore this :).

+    return this.textBuffer.substring(

[David Tseng, 2016/11/15 17:54:59]

This deserves some unit tests to exercises the various corner cases.

+        brailleToText[this.viewPort.start * this.columnCount_],
+        brailleToText[index]);
+  },
+
+  /**
+   * Sets the current pan strategy and resets the viewport.
+   */
+  setPanStrategy: function(wordWrap) {
+    this.panStrategyWrapped_ = wordWrap;
+    this.viewPort_.start = 0;
+    this.viewPort_.end = this.rowCount_ - 1;
+  },
+
+  /**
    * Sets the display size.  This call may update the viewport.
-   * @param {number} size the new display size, or {@code 0} if no display is
+   * @param {number} rowCount the new row size, or {@code 0} if no display is
    *     present.
+   * @param {number} columnCount the new column size, or {@code 0}
+   *    if no display is present.
    */
-  setDisplaySize: function(size) {
-    this.displaySize_ = size;
-    this.panToPosition_(this.viewPort_.start);

[David Tseng, 2016/11/15 17:54:59]

You'll want ot keep this around I suspect.

[ultimatedbz, 2016/11/19 03:11:59]

Acknowledged.

+  setDisplaySize: function(rowCount, columnCount) {
+    this.rowCount_ = rowCount;
+    this.columnCount_ = columnCount;
   },
 
   /**
-   * Sets the current content that panning should happen within.  This call may
-   * change the viewport.
-   * @param {!ArrayBuffer} translatedContent The new content.
-   * @param {number} targetPosition Target position.  The viewport is changed
-   *     to overlap this position.
+   * Sets the internal data structures that hold the fixed and wrapped buffers
+   * and maps.
+   * @param {string} textBuffer Text of the shown braille.
+   * @param {!ArrayBuffer} translatedContent The new braille content.
+   * @param {Array<number>} fixedBrailleToText Map of Braille cells to the first
+   *     index of corresponding text letter.
    */
-  setContent: function(translatedContent, targetPosition) {
-    this.breakPoints_ = this.calculateBreakPoints_(translatedContent);
-    this.contentLength_ = translatedContent.byteLength;
-    this.panToPosition_(targetPosition);
+  setContent: function(textBuffer, translatedContent, fixedBrailleToText) {
+    this.viewPort_.start = 0;
+    this.viewPort_.end = this.rowCount_ - 1;
+    this.fixedBrailleToText_ = fixedBrailleToText;
+    this.wrappedBrailleToText_ = [];
+    this.textBuffer = textBuffer;

[David Tseng, 2016/11/16 16:56:01]

This isn't annotated and set in the constructor.

[ultimatedbz, 2016/11/19 03:11:59]

Done.

+    this.fixedBuffer_ = translatedContent;

[David Tseng, 2016/11/16 16:56:01]

Note that I think there are some tricky ownership issues with this.
BrailleDisplayManager owns (in a very loose sense) translatedContent. It does,
likely among other things, write a cursor into translateContent. After patching
in your change, that currently no longer works.

In the previous design, PanStrategy had only a read-only view into the various
buffers.

[ultimatedbz, 2016/11/19 03:11:58]

I've now given the panStrategy full ownership of the translatedContent. As you
can see in the next patch, braille display manager will no longer have
translatedContent or displayedContent.

+
+    // Convert the cells to Unicode braille pattern characters.
+    var view = new Uint8Array(translatedContent);
+    var wrappedBrailleArray = [];
+
+    var lastBreak = 0;
+    var cellsPadded = 0;
+    var index;
+    for (index = 0; index < translatedContent.byteLength + cellsPadded;
+        index++) {
+      // Is index at the beginning of a new line?
+      if (index != 0 && index % this.columnCount_ == 0) {
+        if (view[index - cellsPadded] == 0) {
+          // On Delete all 0's at beginning of new line.

[David Tseng, 2016/11/15 17:55:00]

Rephrase

[ultimatedbz, 2016/11/19 03:11:58]

Done.

+          while (index - cellsPadded < view.length &&
+              view[index - cellsPadded] == 0) {
+            cellsPadded--;
+          }

[David Tseng, 2016/11/15 17:55:00]

nit: remove curlies

[ultimatedbz, 2016/11/15 19:31:57]

Is it okay to remove the curly braces, even though the while statement is two
lines?

[David Tseng, 2016/11/15 21:36:38]

Good question. It looks like this file keeps curly braces around no matter what,
so ignore the nits regarding curleis.

+          index--;
+        } else if (view[index - cellsPadded - 1] != 0 &&
+            lastBreak % this.columnCount_ != 0) {
+          // If first cell is not empty, we need to move the whole word down to
+          // this line and padd to previous line with 0's, from |lastBreak| to
+          // index. The braille to text map is also updated.
+          // If lastBreak is at the beginning of a line, that means the current
+          // word is bigger than |this.columnCount_| so we shouldn't wrap.
+          for (var j = lastBreak + 1; j < index; j++) {
+            wrappedBrailleArray[j] = 0;

[David Tseng, 2016/11/15 17:55:00]

A few things:
- this absolutely needs tests
- for  clarity, it's ok to make multiple passes through the view
I might suggest a first pass where you compute a list of line breaks. On a
second pass, compute the wrapped braille buffer and braille to text.

+            this.wrappedBrailleToText_[j] = this.wrappedBrailleToText_[j - 1];
+            cellsPadded++;
+          }
+          lastBreak = index;
+          index--;
+        } else {
+          // |lastBreak| is at the beginning of a line, so current word is
+          // bigger than |this.columnCount_| so we shouldn't wrap.
+          wrappedBrailleArray.push(view[index - cellsPadded]);
+          this.wrappedBrailleToText_.push(
+              fixedBrailleToText[index - cellsPadded]);
+        }
+      } else {
+        if (view[index - cellsPadded] == 0) {
+          lastBreak = index;
+        }
+        wrappedBrailleArray.push(view[index - cellsPadded]);
+        this.wrappedBrailleToText_.push(
+            fixedBrailleToText[index - cellsPadded]);
+      }
+    }
+
+    // Convert the wrapped Braille Uint8 Array back to ArrayBuffer.
+    var wrappedBrailleUint8Array = new Uint8Array(wrappedBrailleArray);
+    this.wrappedBuffer_ = new ArrayBuffer(wrappedBrailleUint8Array.length);
+    new Uint8Array(this.wrappedBuffer_).set(wrappedBrailleUint8Array);
+
+    // Calculate how many lines are in the fixed and wrapped buffers.
+    this.fixedLineCount_ =
+        Math.ceil(this.fixedBuffer_.byteLength / this.columnCount_);
+    this.wrappedLineCount_ =
+        Math.ceil(this.wrappedBuffer_.byteLength / this.columnCount_);

[David Tseng, 2016/11/15 17:54:59]

Optional: You might consider even storing a 2d array as previously discussed.

[ultimatedbz, 2016/11/15 19:31:57]

Acknowledged.

   },
 
   /**
    * If possible, changes the viewport to a part of the line that follows
    * the current viewport.
    * @return {boolean} {@code true} if the viewport was changed.
    */
   next: function() {
+    var contentLength = this.panStrategyWrapped_ ?
+        this.wrappedLineCount_ : this.fixedLineCount_;

[David Tseng, 2016/11/15 17:54:59]

I'm even more convinced now that you should separate this out into  separate
classes especially considering the member resolution here and the more work we
need to do to support multiline wrapping.

[ultimatedbz, 2016/11/15 19:31:57]

I agree.

     var newStart = this.viewPort_.end;
     var newEnd;
-    if (newStart + this.displaySize_ < this.contentLength_) {
-      newEnd = this.extendRight_(newStart);
+    if (newStart + this.rowCount_ - 1 < contentLength) {
+      newEnd = newStart + this.rowCount_ - 1;
     } else {
-      newEnd = this.contentLength_;
+      newEnd = contentLength;
     }
     if (newEnd > newStart) {
       this.viewPort_ = {start: newStart, end: newEnd};
       return true;
     }
     return false;
   },
 
   /**
    * If possible, changes the viewport to a part of the line that precedes
    * the current viewport.
    * @return {boolean} {@code true} if the viewport was changed.
    */
   previous: function() {
+    var contentLength = this.panStrategyWrapped_ ?
+        this.wrappedLineCount_ : this.fixedLineCount_;
     if (this.viewPort_.start > 0) {
       var newStart, newEnd;
-      if (this.viewPort_.start <= this.displaySize_) {
+      if (this.viewPort_.start < this.rowCount_) {
         newStart = 0;
-        newEnd = this.extendRight_(newStart);
+        newEnd = Math.min(this.rowCount_, contentLength);
       } else {
         newEnd = this.viewPort_.start;
-        var limit = newEnd - this.displaySize_;
-        newStart = limit;
-        var pos = 0;
-        while (pos < this.breakPoints_.length &&
-            this.breakPoints_[pos] < limit) {
-          pos++;
-        }
-        if (pos < this.breakPoints_.length &&
-            this.breakPoints_[pos] < newEnd) {
-          newStart = this.breakPoints_[pos];
-        }
+        newStart = newEnd - this.rowCount_ + 1;
       }
       if (newStart < newEnd) {
         this.viewPort_ = {start: newStart, end: newEnd};
         return true;
       }
     }
     return false;
   },
 
   /**
-   * Finds the end position for a new viewport start position, considering
-   * current breakpoints as well as display size and content length.
-   * @param {number} from Start of the region to extend.
-   * @return {number}
-   * @private
-   */
-  extendRight_: function(from) {
-    var limit = Math.min(from + this.displaySize_, this.contentLength_);
-    var pos = 0;
-    var result = limit;
-    while (pos < this.breakPoints_.length && this.breakPoints_[pos] <= from) {
-      pos++;
-    }
-    while (pos < this.breakPoints_.length && this.breakPoints_[pos] <= limit) {
-      result = this.breakPoints_[pos];
-      pos++;
-    }
-    return result;
-  },
-
-  /**
-   * Overridden by subclasses to provide breakpoints given translated
-   * braille cell content.
-   * @param {!ArrayBuffer} content New display content.
-   * @return {!Array<number>} The points before which it is desirable to break
-   *     content if needed or the empty array if no points are more desirable
-   *     than any position.
-   * @private
-   */
-  calculateBreakPoints_: function(content) {return [];},
-
-  /**
+   * TODO need to implement for routing
    * Moves the viewport so that it overlaps a target position without taking
    * the current viewport position into consideration.
    * @param {number} position Target position.
    */
   panToPosition_: function(position) {
+    /*

[David Tseng, 2016/11/15 17:55:00]

Don't do this, even in a work in progress cl.

[ultimatedbz, 2016/11/15 19:31:57]

What should I do instead? The code would just break if I don't comment this out.

[David Tseng, 2016/11/15 21:36:38]

Well, could you see how this method works and see how it should behave in the
multi line case?

If we read the method, it looks like the author manually pans next until the
viewport contains the position. In your case, the |position| is a row, right?

[ultimatedbz, 2016/11/15 22:25:42]

Sorry, I should have clarified my question. My question was actually: "If I'm
doing a work in progress cl, but I'm not done with this piece of code, what
should I do if I don't comment the code out?" You are telling me not to comment
it out, but I think that what you're suggesting is that I implement fully
working code before submitting this cl. I'm curious to ask what are the proper
ways of submitting an in progress cl? It seems like as the code is right now, I
shouldn't even put this code up for review yet...

And yes, in my case, |position| will probably be a row number, although if we
are to increase granularity and have per-cell routing, position may need to be
an actual per-cell index.

[David Tseng, 2016/11/16 16:56:01]

As mentioned before, you're breaking things either way, so don't comment it out
in the first place and yes, if you feel like commenting out large blocks of
functionality, it usually doesn't make the reviewer very comfortable with the
change as a whole.

" You are telling me not to comment
What I'm suggesting is for you to uncomment the code and to consider subclassing
to limit the ways in which you're trying to hook into the current design since
you are unclear on how to support parts of it.

I'm here also in person, if you wanted to chat in detail.

[ultimatedbz, 2016/11/19 03:11:58]

Yep, this makes a lot more sense, and I'm glad we resolved this in person!

     if (this.displaySize_ > 0) {
       this.viewPort_ = {start: 0, end: 0};
       while (this.next() && this.viewPort_.end <= position) {
         // Nothing to do.
       }
     } else {
       this.viewPort_ = {start: position, end: position};
     }
+    */
   },
 };
-
-/**
- * A pan strategy that fits as much content on the display as possible, that
- * is, it doesn't do any wrapping.
- * @constructor
- * @extends {cvox.PanStrategy}
- */
-cvox.FixedPanStrategy = cvox.PanStrategy;
-/**
- * A pan strategy that tries to wrap 'words' when breaking content.
- * A 'word' in this context is just a chunk of non-blank braille cells
- * delimited by blank cells.
- * @constructor
- * @extends {cvox.PanStrategy}
- */
-cvox.WrappingPanStrategy = function() {
-  cvox.PanStrategy.call(this);
-};
-
-cvox.WrappingPanStrategy.prototype = {
-  __proto__: cvox.PanStrategy.prototype,
-
-  /** @override */
-  calculateBreakPoints_: function(content) {
-    var view = new Uint8Array(content);
-    var newContentLength = view.length;
-    var result = [];
-    var lastCellWasBlank = false;
-    for (var pos = 0; pos < view.length; ++pos) {
-      if (lastCellWasBlank && view[pos] != 0) {
-        result.push(pos);
-      }
-      lastCellWasBlank = (view[pos] == 0);
-    }
-    return result;
-  },
-};