Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2496823002:
Implement word wrapping and panning in multiline Braille. (Closed)
|
Created:
4 years, 1 month ago by ultimatedbz Modified:
4 years ago CC:
chromium-reviews, extensions-reviews_chromium.org, alemate+watch_chromium.org, oshima+watch_chromium.org, aboxhall+watch_chromium.org, nektar+watch_chromium.org, yuzo+watch_chromium.org, je_julie, arv+watch_chromium.org, dtseng+watch_chromium.org, chromium-apps-reviews_chromium.org, dmazzoni+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement word wrapping and panning in multiline Braille.
The current code doesn't support word wrapping when displaying multiline braille.
The panStrategy class also doesn't allow panning in multiline. This CL changes
the panStrategy completely. Before, it only returns starting and ending indices
of the current viewport. Now, it stores the fixed and wrapped versions of the
braille that is displayed. It also keeps the state of whether the pan strategy
is currently "fixed" or "wrapped". The panStrategy class also now supports
panning in multiline. Changes to the braille display manager have been made to
utilize the new panStrategy. In addition, changes have been made to the braille
captions background so that grouping works, even if the viewport is not at the
beginning of the buffer. Specifically, it uses two offsets that the panStrategy
class generates to find the correct indices for braille and text.
BUG=661199
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/2a59dc298b6f036467777ae3658fe8546f0a5978
Cr-Commit-Position: refs/heads/master@{#437318}
Patch Set 1 #Patch Set 2 : removed unnecessary files #
Total comments: 16
Patch Set 3 : Addressed code reviews. #
Total comments: 56
Patch Set 4 : Writing cursor and routing should work, need to do tests still #Patch Set 5 : Wrote tests, fixed bugs :) #
Total comments: 27
Patch Set 6 : Addressed comments #Patch Set 7 : Rebased onto Master #Patch Set 8 : changed from using local storage to chrome.storage.local #
Total comments: 8
Patch Set 9 : Addressed David's comments #Messages
Total messages: 53 (18 generated)
Description was changed from ========== Implement word wrapping and panning in multiline Braille. The current code doesn't support word wrapping when displaying multiline braille. The panStrategy class also doesn't allow panning in multiline. This CL changes the panStrategy completely. Before, it only returns starting and ending indices of the current viewport. Now, it stores the fixed and wrapped versions of the braille that is displayed. It also keeps the state of whether the pan strategy is currently "fixed" or "wrapped". The panStrategy class also now supports panning in multiline. Changes to the braille display manager have been made to utilize the new panStrategy. In addition, changes have been made to the braille captions background so that grouping works, even if the viewport is not at the beginning of the buffer. Specifically, it uses two offsets that the panStrategy class generates to find the correct indices for braille and text. BUG=661199 ========== to ========== Implement word wrapping and panning in multiline Braille. The current code doesn't support word wrapping when displaying multiline braille. The panStrategy class also doesn't allow panning in multiline. This CL changes the panStrategy completely. Before, it only returns starting and ending indices of the current viewport. Now, it stores the fixed and wrapped versions of the braille that is displayed. It also keeps the state of whether the pan strategy is currently "fixed" or "wrapped". The panStrategy class also now supports panning in multiline. Changes to the braille display manager have been made to utilize the new panStrategy. In addition, changes have been made to the braille captions background so that grouping works, even if the viewport is not at the beginning of the buffer. Specifically, it uses two offsets that the panStrategy class generates to find the correct indices for braille and text. BUG=661199 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
On 2016/11/11 02:37:20, ultimatedbz wrote: > Description was changed from > > ========== > Implement word wrapping and panning in multiline Braille. > > The current code doesn't support word wrapping when displaying multiline > braille. > The panStrategy class also doesn't allow panning in multiline. This CL changes > the panStrategy completely. Before, it only returns starting and ending indices > of the current viewport. Now, it stores the fixed and wrapped versions of the > braille that is displayed. It also keeps the state of whether the pan strategy > is currently "fixed" or "wrapped". The panStrategy class also now supports > panning in multiline. Changes to the braille display manager have been made to > utilize the new panStrategy. In addition, changes have been made to the braille > captions background so that grouping works, even if the viewport is not at the > beginning of the buffer. Specifically, it uses two offsets that the panStrategy > class generates to find the correct indices for braille and text. > > > BUG=661199 > ========== > > to > > ========== > Implement word wrapping and panning in multiline Braille. > > The current code doesn't support word wrapping when displaying multiline > braille. > The panStrategy class also doesn't allow panning in multiline. This CL changes > the panStrategy completely. Before, it only returns starting and ending indices > of the current viewport. Now, it stores the fixed and wrapped versions of the > braille that is displayed. It also keeps the state of whether the pan strategy > is currently "fixed" or "wrapped". The panStrategy class also now supports > panning in multiline. Changes to the braille display manager have been made to > utilize the new panStrategy. In addition, changes have been made to the braille > captions background so that grouping works, even if the viewport is not at the > beginning of the buffer. Specifically, it uses two offsets that the panStrategy > class generates to find the correct indices for braille and text. > > > BUG=661199 > CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation > ========== This is still in progress. I realize there are probably a lot of comments to be written and tests to change. That being said, the code written so far takes care of the initial edge cases that I thought of and came across. If anything that confuses you, please ping me or ask me in person!
Description was changed from ========== Implement word wrapping and panning in multiline Braille. The current code doesn't support word wrapping when displaying multiline braille. The panStrategy class also doesn't allow panning in multiline. This CL changes the panStrategy completely. Before, it only returns starting and ending indices of the current viewport. Now, it stores the fixed and wrapped versions of the braille that is displayed. It also keeps the state of whether the pan strategy is currently "fixed" or "wrapped". The panStrategy class also now supports panning in multiline. Changes to the braille display manager have been made to utilize the new panStrategy. In addition, changes have been made to the braille captions background so that grouping works, even if the viewport is not at the beginning of the buffer. Specifically, it uses two offsets that the panStrategy class generates to find the correct indices for braille and text. BUG=661199 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Implement word wrapping and panning in multiline Braille. The current code doesn't support word wrapping when displaying multiline braille. The panStrategy class also doesn't allow panning in multiline. This CL changes the panStrategy completely. Before, it only returns starting and ending indices of the current viewport. Now, it stores the fixed and wrapped versions of the braille that is displayed. It also keeps the state of whether the pan strategy is currently "fixed" or "wrapped". The panStrategy class also now supports panning in multiline. Changes to the braille display manager have been made to utilize the new panStrategy. In addition, changes have been made to the braille captions background so that grouping works, even if the viewport is not at the beginning of the buffer. Specifically, it uses two offsets that the panStrategy class generates to find the correct indices for braille and text. BUG=661199 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
ultimatedbz@google.com changed reviewers: + dmazzoni@chromium.org
ultimatedbz@google.com changed reviewers: + dtseng@chromium.org - dmazzoni@chromium.org
ultimatedbz@google.com changed reviewers: + dmazzoni@chromium.org
https://codereview.chromium.org/2496823002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js (right): https://codereview.chromium.org/2496823002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:163: // TODO might need to change when display is plugged in. This is going to break for real devices. Look at the logic dealing with whether the display is available. Understand how: - getVirtualDisplayState works above - after you understand that, reconsider how you get the virtual display params below https://codereview.chromium.org/2496823002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:288: // */ Please don't comment out code like this with a TODO especially because this breaks routing... Btw, TODO's are formmated like: TODO(ultimatedbz) ... https://codereview.chromium.org/2496823002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js (left): https://codereview.chromium.org/2496823002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:191: cvox.FixedPanStrategy = cvox.PanStrategy; Please don't remove these classes. A much better way for you to introduce your changes and ensure you don't break existing functionality is to add a subclass of PanStrategy e.g. MultiLinePanStrategy, that knows how to pan inside of a multi line braille display. Inside of BrailleDisplayManager, you can swap in your pan strategy whenever a mlutiline display or virtual display is detected. https://codereview.chromium.org/2496823002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js (right): https://codereview.chromium.org/2496823002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:47: this.wrappedLineCount_ = 0; All of the members here need JS doc. Use the other members to get a sense of how that works practically and/or read up on it. The annotations are here to help you ensure correctness of your code. To run the Closure compiler (which amongst other things, does type checking), do: git cl presubmit -u --force
> > Please don't remove these classes. > For context, note that his changes supercede the existing PanStrategy implementations - the new code supports all of the previous functionality in addition to multiline and in fact that logic is basically the same. I don't think subclassing is needed. Please take a closer look . -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2496823002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js (right): https://codereview.chromium.org/2496823002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:161: var wrappedBrailleArray = new Uint8Array(translatedContent.byteLength * 4); Since Uint8Array can't be resized, it might make more sense to work with a different data type inside the loop, then convert to a Uint8array at the end? https://codereview.chromium.org/2496823002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:213: this.fixedLineCount_ = this.fixedBuffer_.byteLength / this.columnCount_; How about Math.ceil(byteLength / columnCount)? https://codereview.chromium.org/2496823002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:216: this.wrappedLineCount_ = same https://codereview.chromium.org/2496823002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js (right): https://codereview.chromium.org/2496823002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:487: var companionID2 = cell.getAttribute('companionID2'); Rather than two attributes, how about a single attribute companionIDs with a list of ids, space-separated? Use split() and join() to make it easy.
https://codereview.chromium.org/2496823002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js (right): https://codereview.chromium.org/2496823002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:288: // */ On 2016/11/14 21:05:08, David Tseng wrote: > Please don't comment out code like this with a TODO especially because this > breaks routing... > > Btw, TODO's are formmated like: > TODO(ultimatedbz) ... Hey David! Noted, my intentions weren't for this to stay here when the code to be committed, but only so that I could be reminded to fill this out. This initial CL was just to get the conversation started and get some comments on code I've already written, while I continue to work on commenting, cleaning up the code, and finishing up these TODO's. Thanks! https://codereview.chromium.org/2496823002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js (left): https://codereview.chromium.org/2496823002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:191: cvox.FixedPanStrategy = cvox.PanStrategy; On 2016/11/14 21:05:08, David Tseng wrote: > Please don't remove these classes. > > A much better way for you to introduce your changes and ensure you don't break > existing functionality is to add a subclass of PanStrategy e.g. > MultiLinePanStrategy, that knows how to pan inside of a multi line braille > display. Inside of BrailleDisplayManager, you can swap in your pan strategy > whenever a mlutiline display or virtual display is detected. I was thinking that if we got multiline to work, it would work for single line braille displays too? However, with functionality like routing that might not be supported for multiline braille, maybe it's better to have separate classes. Instead of having completely different functionality between single line and multi line braille, I was thinking that we could keep some of the changes I made for multi line braille. For example, instead of having a wrapping and fixed panning strategy, I could just have a flag that specified the [wrapped/fixed]. Also, instead of just returning start and end indices, I could store the whole braille buffer and return the whole slice to display. Thoughts? https://codereview.chromium.org/2496823002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js (right): https://codereview.chromium.org/2496823002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:47: this.wrappedLineCount_ = 0; On 2016/11/14 21:05:08, David Tseng wrote: > All of the members here need JS doc. Use the other members to get a sense of how > that works practically and/or read up on it. > > The annotations are here to help you ensure correctness of your code. > > To run the Closure compiler (which amongst other things, does type checking), > do: > git cl presubmit -u --force Thanks! I actually wasn't sure how to type check. I'll definitely be using that command a lot!
On 2016/11/14 21:28:11, dmazzoni wrote: > > > > Please don't remove these classes. > > > > For context, note that his changes supercede the existing PanStrategy > implementations - the new code supports all of the previous functionality > in addition to multiline and in fact that logic is basically the same. > > I don't think subclassing is needed. Please take a closer look . > For one thing, it would make it much easier to test the changes here isolated from the behaviors we know work for the previous fixed and wrapping strategies. And, also, I didn't see any of the tests updated. Please do that as well and ensure they all pass and then I'll take a closer look. > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by dtseng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/15 04:21:27, David Tseng wrote: > On 2016/11/14 21:28:11, dmazzoni wrote: > > > > > > Please don't remove these classes. > > > > > > > For context, note that his changes supercede the existing PanStrategy > > implementations - the new code supports all of the previous functionality > > in addition to multiline and in fact that logic is basically the same. > > > > I don't think subclassing is needed. Please take a closer look . > > > For one thing, it would make it much easier to test the changes here isolated > from the behaviors we know work for the previous fixed and wrapping strategies. > > And, also, I didn't see any of the tests updated. Please do that as well and > ensure they all pass and then I'll take a closer look. > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. Sorry for the multiple emails, but you will also want to run this change on some of the try bots. I've sent this change to the dry run commit queue. I'm pretty sure this change isn't going to pass chromevox_tests, so look at those first (e.g. you removed WrappingPanStrategy, but it's being explicitly tested in the pan_strategy_test.unitjs)
On 2016/11/15 04:26:13, David Tseng wrote: > On 2016/11/15 04:21:27, David Tseng wrote: > > On 2016/11/14 21:28:11, dmazzoni wrote: > > > > > > > > Please don't remove these classes. > > > > > > > > > > For context, note that his changes supercede the existing PanStrategy > > > implementations - the new code supports all of the previous functionality > > > in addition to multiline and in fact that logic is basically the same. > > > > > > I don't think subclassing is needed. Please take a closer look . > > > > > For one thing, it would make it much easier to test the changes here isolated > > from the behaviors we know work for the previous fixed and wrapping > strategies. > > > > And, also, I didn't see any of the tests updated. Please do that as well and > > ensure they all pass and then I'll take a closer look. > > > > > -- > > > You received this message because you are subscribed to the Google Groups > > > "Chromium-reviews" group. > > > To unsubscribe from this group and stop receiving emails from it, send an > > email > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > Sorry for the multiple emails, but you will also want to run this change on some > of the try bots. I've sent this change to the dry run commit queue. > > I'm pretty sure this change isn't going to pass chromevox_tests, so look at > those first (e.g. you removed WrappingPanStrategy, but it's being explicitly > tested in the pan_strategy_test.unitjs) Yes, I'm sure this change is going to break a lot of tests haha. I'm definitely going to be working on the tests now, but for the time being, I wanted to put this commit up to get some comments on the general logic and classes that I set up, as I've changed and implemented quite a few things! Looking back, I definitely should have specified what I wanted to be reviewed, and what I still needed to implement, to avoid all this confusion. Also, @dmazzoni, it isn't entirely correct to say that the current code shares the same functionality as the old code because it doesn't support routing. Although we aren't sure if per-cell routing will be supported in multiline, I feel that I should still implement routing for multiline, so that it works for single line and multiline if it's supported in the future.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/11/15 04:49:38, ultimatedbz wrote: > On 2016/11/15 04:26:13, David Tseng wrote: > > On 2016/11/15 04:21:27, David Tseng wrote: > > > On 2016/11/14 21:28:11, dmazzoni wrote: > > > > > > > > > > Please don't remove these classes. > > > > > > > > > > > > > For context, note that his changes supercede the existing PanStrategy > > > > implementations - the new code supports all of the previous functionality > > > > in addition to multiline and in fact that logic is basically the same. > > > > > > > > I don't think subclassing is needed. Please take a closer look . > > > > > > > For one thing, it would make it much easier to test the changes here > isolated > > > from the behaviors we know work for the previous fixed and wrapping > > strategies. > > > > > > And, also, I didn't see any of the tests updated. Please do that as well and > > > ensure they all pass and then I'll take a closer look. > > > > > > > -- > > > > You received this message because you are subscribed to the Google Groups > > > > "Chromium-reviews" group. > > > > To unsubscribe from this group and stop receiving emails from it, send an > > > email > > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > Sorry for the multiple emails, but you will also want to run this change on > some > > of the try bots. I've sent this change to the dry run commit queue. > > > > I'm pretty sure this change isn't going to pass chromevox_tests, so look at > > those first (e.g. you removed WrappingPanStrategy, but it's being explicitly > > tested in the pan_strategy_test.unitjs) > > Yes, I'm sure this change is going to break a lot of tests haha. I'm definitely > going to be working on the tests now, but for the time being, I wanted to put > this commit up to get some comments on the general logic and classes that I set > up, as I've changed and implemented quite a few things! Looking back, I > definitely should have specified what I wanted to be reviewed, and what I still > needed to implement, to avoid all this confusion. > No problems. The overall design is fine. Generally, keeping less state is better in terms of design and if making invasive changes, you can use the existing tests to guide your design. In terms of overall feedback, the reason I suggested subclassing was because I could see a need to have multi-line styled panning e.g. pan by page, pan vertically, pan horizontally, etc. I'll take an in-depth look at patchset 3, but it looks like tests still haven't been updated. There are some basic unittests that are failing. > Also, @dmazzoni, it isn't entirely correct to say that the current code shares > the same functionality as the old code because it doesn't support routing. > Although we aren't sure if per-cell routing will be supported in multiline, I > feel that I should still implement routing for multiline, so that it works for > single line and multiline if it's supported in the future.
https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js (right): https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:167: this.panStrategy_.setDisplaySize(newRowCount, newColumnCount); Make the display size a pair i.e. @type {{rows: number, columns: number}} https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:193: var brailleBuf = this.panStrategy_.getCurrentBrailleSlice(); Slice seems wrong in terms of multiline; maybe call this getCurrentBrailleViewportContents https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:285: /* TODO The goal is to get this cl checked in, so please restore this block and support at least the single line case. https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js (left): https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:67: this.panToPosition_(this.viewPort_.start); You'll want ot keep this around I suspect. https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js (right): https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:51: * @type {Array} Array<number> https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:57: * Start and end are both inclusive. Fix these comments. https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:104: * @return {Array<number>} The offset of braille and text indices of the Make this an Object e.g. {brailleOffset: number, textOffset: nuimber} https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:114: * corresponding text character. @return {Array<number} https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:129: this.wrappedBuffer_ : this.fixedBuffer_; 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. https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:130: buf = buf.slice(this.viewPort.start * this.columnCount_, return buff.slice... https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:131: (this.viewPort.end + 1) * this.columnCount_); 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. https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:137: * the current braille slice. @return {Array<number>} https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:142: var index = (this.viewPort.end + 1) * this.columnCount_ - 1; This just looks wrong (mostly the viewport.end + 1 expression). https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:150: } nit: no curlies for single lined if bodies. https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:151: return this.textBuffer.substring( This deserves some unit tests to exercises the various corner cases. https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:205: // On Delete all 0's at beginning of new line. Rephrase https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:209: } nit: remove curlies https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:219: wrappedBrailleArray[j] = 0; 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. https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:251: Math.ceil(this.wrappedBuffer_.byteLength / this.columnCount_); Optional: You might consider even storing a 2d array as previously discussed. https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:261: this.wrappedLineCount_ : this.fixedLineCount_; 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. https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:308: /* Don't do this, even in a work in progress cl.
On 2016/11/15 15:24:08, David Tseng wrote: > On 2016/11/15 04:49:38, ultimatedbz wrote: > > On 2016/11/15 04:26:13, David Tseng wrote: > > > On 2016/11/15 04:21:27, David Tseng wrote: > > > > On 2016/11/14 21:28:11, dmazzoni wrote: > > > > > > > > > > > > Please don't remove these classes. > > > > > > > > > > > > > > > > For context, note that his changes supercede the existing PanStrategy > > > > > implementations - the new code supports all of the previous > functionality > > > > > in addition to multiline and in fact that logic is basically the same. > > > > > > > > > > I don't think subclassing is needed. Please take a closer look . > > > > > > > > > For one thing, it would make it much easier to test the changes here > > isolated > > > > from the behaviors we know work for the previous fixed and wrapping > > > strategies. > > > > > > > > And, also, I didn't see any of the tests updated. Please do that as well > and > > > > ensure they all pass and then I'll take a closer look. > > > > > > > > > -- > > > > > You received this message because you are subscribed to the Google > Groups > > > > > "Chromium-reviews" group. > > > > > To unsubscribe from this group and stop receiving emails from it, send > an > > > > email > > > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > Sorry for the multiple emails, but you will also want to run this change on > > some > > > of the try bots. I've sent this change to the dry run commit queue. > > > > > > I'm pretty sure this change isn't going to pass chromevox_tests, so look at > > > those first (e.g. you removed WrappingPanStrategy, but it's being explicitly > > > tested in the pan_strategy_test.unitjs) > > > > Yes, I'm sure this change is going to break a lot of tests haha. I'm > definitely > > going to be working on the tests now, but for the time being, I wanted to put > > this commit up to get some comments on the general logic and classes that I > set > > up, as I've changed and implemented quite a few things! Looking back, I > > definitely should have specified what I wanted to be reviewed, and what I > still > > needed to implement, to avoid all this confusion. > > > > No problems. The overall design is fine. Generally, keeping less state is better > in terms of design and if making invasive changes, you can use the existing > tests to guide your design. > > In terms of overall feedback, the reason I suggested subclassing was because I > could see a need to have multi-line styled panning e.g. pan by page, pan > vertically, pan horizontally, etc. > > I'll take an in-depth look at patchset 3, but it looks like tests still haven't > been updated. There are some basic unittests that are failing. > > > Also, @dmazzoni, it isn't entirely correct to say that the current code shares > > the same functionality as the old code because it doesn't support routing. > > Although we aren't sure if per-cell routing will be supported in multiline, I > > feel that I should still implement routing for multiline, so that it works for > > single line and multiline if it's supported in the future. Hey David, I was wondering why less state is usually better? I also think that what you brought up about there being a possible need to have different multi-line styled panning interesting as I've only assumed that we'd pan vertically going by the size of the display. I can also see the possibility of implementing panning line by line. With that in mind, I can see that if Panning Strategy has to keep track of all these states, maybe it's better to subclass.
https://codereview.chromium.org/2496823002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js (right): https://codereview.chromium.org/2496823002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:163: // TODO might need to change when display is plugged in. On 2016/11/14 21:05:08, David Tseng wrote: > This is going to break for real devices. Look at the logic dealing with whether > the display is available. Understand how: > - getVirtualDisplayState works above > - after you understand that, reconsider how you get the virtual display params > below > Right, I'm thinking that if a newState is available, I'll also set localStorage to the physical display's dimensions, so that the localStorage values will be consistent and won't break for real devices. https://codereview.chromium.org/2496823002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js (right): https://codereview.chromium.org/2496823002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:161: var wrappedBrailleArray = new Uint8Array(translatedContent.byteLength * 4); On 2016/11/14 21:37:37, dmazzoni wrote: > Since Uint8Array can't be resized, it might make more sense to work with a > different data type inside the loop, then convert to a Uint8array at the end? I was thinking if I had to expand the Uint8array, I'd make a new one that's twice the size, copy the current array's information over, and continue. I would expect that it would only happen once or twice at most. As for a different datatype that is mutable, a regular array seems like it would do the trick. According to stackoverflow, I'd also just have to call wrappedBrailleArray = [].slice.call(view) to get an array version of it, and wrappedBrailleUint8Array = new Uint8Array(wrappedBrailleArray) to change it back! https://codereview.chromium.org/2496823002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:213: this.fixedLineCount_ = this.fixedBuffer_.byteLength / this.columnCount_; On 2016/11/14 21:37:37, dmazzoni wrote: > How about Math.ceil(byteLength / columnCount)? Oh! I forgot about Math.ceil(), thanks! https://codereview.chromium.org/2496823002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:216: this.wrappedLineCount_ = On 2016/11/14 21:37:37, dmazzoni wrote: > same Done. https://codereview.chromium.org/2496823002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js (right): https://codereview.chromium.org/2496823002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:487: var companionID2 = cell.getAttribute('companionID2'); On 2016/11/14 21:37:37, dmazzoni wrote: > Rather than two attributes, how about a single attribute companionIDs with a > list of ids, space-separated? Use split() and join() to make it easy. That's a great idea! I also heard that attributes should have 'data-' in front of them to make them html compliant, so that's what I'll do.
https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js (right): https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:285: /* TODO On 2016/11/15 17:54:59, David Tseng wrote: > The goal is to get this cl checked in, so please restore this block and support > at least the single line case. I do intend to complete this block, but for the time being, I wanted to get something up for initial review. It's in a TODO block to signify that I'm still not done with this method. Do you have any suggestions as to how I should go about doing this in the future? Either explicitly saying I'm going to do this later on, or to not submit this cl until everything is complete? https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js (right): https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:142: var index = (this.viewPort.end + 1) * this.columnCount_ - 1; On 2016/11/15 17:54:59, David Tseng wrote: > This just looks wrong (mostly the viewport.end + 1 expression). 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;' https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:209: } On 2016/11/15 17:55:00, David Tseng wrote: > nit: remove curlies Is it okay to remove the curly braces, even though the while statement is two lines? https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:251: Math.ceil(this.wrappedBuffer_.byteLength / this.columnCount_); On 2016/11/15 17:54:59, David Tseng wrote: > Optional: You might consider even storing a 2d array as previously discussed. Acknowledged. https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:261: this.wrappedLineCount_ : this.fixedLineCount_; On 2016/11/15 17:54:59, David Tseng wrote: > 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. I agree. https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:308: /* On 2016/11/15 17:55:00, David Tseng wrote: > Don't do this, even in a work in progress cl. What should I do instead? The code would just break if I don't comment this out.
https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js (right): https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:285: /* TODO On 2016/11/15 19:31:57, ultimatedbz wrote: > On 2016/11/15 17:54:59, David Tseng wrote: > > The goal is to get this cl checked in, so please restore this block and > support > > at least the single line case. > > I do intend to complete this block, but for the time being, I wanted to get > something up for initial review. It's in a TODO block to signify that I'm still > not done with this method. Do you have any suggestions as to how I should go > about doing this in the future? Either explicitly saying I'm going to do this > later on, or to not submit this cl until everything is complete? I would not accept this cl without this working as it did before. You should never comment out code like this as much as it might make things easier. Routing is just another way of addressing a braille display's physical cells. If I have a 40-cell display and click on the 20th cell, Brltty will send an event which eventually gets here. The question is, how to address that position into the viewport. If I panned 40 cells forward, you would obviously offset the above by 40 giving us 60. In a multiline world, and your current implementation, how would you translate a hardware key press over a cell into a valid position within your Offset by start * columns?viewport and keep the single line case working as it did before? https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js (right): https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:142: var index = (this.viewPort.end + 1) * this.columnCount_ - 1; On 2016/11/15 19:31:57, ultimatedbz wrote: > On 2016/11/15 17:54:59, David Tseng wrote: > > This just looks wrong (mostly the viewport.end + 1 expression). > > 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;' pan_strategy_test.unitjs awaits; I am generally ok with somewhat unreadable code as long as it is well covered by tests. https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:150: } On 2016/11/15 17:54:59, David Tseng wrote: > nit: no curlies for single lined if bodies. Ignore this :). https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:209: } On 2016/11/15 19:31:57, ultimatedbz wrote: > On 2016/11/15 17:55:00, David Tseng wrote: > > nit: remove curlies > > Is it okay to remove the curly braces, even though the while statement is two > lines? Good question. It looks like this file keeps curly braces around no matter what, so ignore the nits regarding curleis. https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:308: /* On 2016/11/15 19:31:57, ultimatedbz wrote: > On 2016/11/15 17:55:00, David Tseng wrote: > > Don't do this, even in a work in progress cl. > > What should I do instead? The code would just break if I don't comment this out. 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?
https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js (right): https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:285: /* TODO On 2016/11/15 21:36:38, David Tseng wrote: > On 2016/11/15 19:31:57, ultimatedbz wrote: > > On 2016/11/15 17:54:59, David Tseng wrote: > > > The goal is to get this cl checked in, so please restore this block and > > support > > > at least the single line case. > > > > I do intend to complete this block, but for the time being, I wanted to get > > something up for initial review. It's in a TODO block to signify that I'm > still > > not done with this method. Do you have any suggestions as to how I should go > > about doing this in the future? Either explicitly saying I'm going to do this > > later on, or to not submit this cl until everything is complete? > > I would not accept this cl without this working as it did before. You should > never comment out code like this as much as it might make things easier. > > Routing is just another way of addressing a braille display's physical cells. If > I have a 40-cell display and click on the 20th cell, Brltty will send an event > which eventually gets here. The question is, how to address that position into > the viewport. > > If I panned 40 cells forward, you would obviously offset the above by 40 giving > us 60. > > In a multiline world, and your current implementation, how would you translate a > hardware key press over a cell into a valid position within your Offset by > start * columns?viewport and keep the single line case working as it did before? I think you are assuming that I want to commit the code like this, but what I'm saying is that I am going to get this commented code completed before trying to get this cl accepted. I'm not looking for you the accept everything as it is right now, rather i'd like you to ignore the TODO's and focus on reviewing the code that are no in TODO's. Sorry for the confusion. As for the last question, the current implementation definitely does not support per-cell routing, so I'll need to add an offset of some sort.
On 2016/11/15 21:58:09, ultimatedbz wrote: > https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... > File > chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js > (right): > > https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... > chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:285: > /* TODO > On 2016/11/15 21:36:38, David Tseng wrote: > > On 2016/11/15 19:31:57, ultimatedbz wrote: > > > On 2016/11/15 17:54:59, David Tseng wrote: > > > > The goal is to get this cl checked in, so please restore this block and > > > support > > > > at least the single line case. > > > > > > I do intend to complete this block, but for the time being, I wanted to get > > > something up for initial review. It's in a TODO block to signify that I'm > > still > > > not done with this method. Do you have any suggestions as to how I should go > > > about doing this in the future? Either explicitly saying I'm going to do > this > > > later on, or to not submit this cl until everything is complete? > > > > I would not accept this cl without this working as it did before. You should > > never comment out code like this as much as it might make things easier. > > > > Routing is just another way of addressing a braille display's physical cells. > If > > I have a 40-cell display and click on the 20th cell, Brltty will send an event > > which eventually gets here. The question is, how to address that position into > > the viewport. > > > > If I panned 40 cells forward, you would obviously offset the above by 40 > giving > > us 60. > > > > In a multiline world, and your current implementation, how would you translate > a > > hardware key press over a cell into a valid position within your Offset by > > start * columns?viewport and keep the single line case working as it did > before? > > I think you are assuming that I want to commit the code like this, but what I'm > saying is that I am going to get this commented code completed before trying to > get this cl accepted. I'm not looking for you the accept everything as it is > right now, rather i'd like you to ignore the TODO's and focus on reviewing the > code that are no in TODO's. Sorry for the confusion. No apologies necessary. Usually, a TODO in an uploaded change means to the reviewer something that the uploader is leaving for a subsequent change e.g. TODO(dtseng): implement this missing funcionality. By commenting out this block, you're breaking things in other ways :). What's been helpful for me (and I've not been super successful at this either) is to stash any debugging or temporary changes `git stash` or committing it on an experimental branch from my change. Before uploading the final change, I try and squash my local commits together via `git rebase -i HEAD~x` and then reviewing the squashed commit. > > As for the last question, the current implementation definitely does not support > per-cell routing, so I'll need to add an offset of some sort. That's fine. Again, use the tests to help you here. Write a few and see where that leads you in terms of teasing out the corner cases and testing some of your own assumptions.
https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js (right): https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:308: /* On 2016/11/15 21:36:38, David Tseng wrote: > On 2016/11/15 19:31:57, ultimatedbz wrote: > > On 2016/11/15 17:55:00, David Tseng wrote: > > > Don't do this, even in a work in progress cl. > > > > What should I do instead? The code would just break if I don't comment this > out. > > 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? 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.
On 2016/11/15 22:10:23, David Tseng wrote: > On 2016/11/15 21:58:09, ultimatedbz wrote: > > > https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... > > File > > chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js > > (right): > > > > > https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... > > > chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:285: > > /* TODO > > On 2016/11/15 21:36:38, David Tseng wrote: > > > On 2016/11/15 19:31:57, ultimatedbz wrote: > > > > On 2016/11/15 17:54:59, David Tseng wrote: > > > > > The goal is to get this cl checked in, so please restore this block and > > > > support > > > > > at least the single line case. > > > > > > > > I do intend to complete this block, but for the time being, I wanted to > get > > > > something up for initial review. It's in a TODO block to signify that I'm > > > still > > > > not done with this method. Do you have any suggestions as to how I should > go > > > > about doing this in the future? Either explicitly saying I'm going to do > > this > > > > later on, or to not submit this cl until everything is complete? > > > > > > I would not accept this cl without this working as it did before. You should > > > never comment out code like this as much as it might make things easier. > > > > > > Routing is just another way of addressing a braille display's physical > cells. > > If > > > I have a 40-cell display and click on the 20th cell, Brltty will send an > event > > > which eventually gets here. The question is, how to address that position > into > > > the viewport. > > > > > > If I panned 40 cells forward, you would obviously offset the above by 40 > > giving > > > us 60. > > > > > > In a multiline world, and your current implementation, how would you > translate > > a > > > hardware key press over a cell into a valid position within your Offset by > > > start * columns?viewport and keep the single line case working as it did > > before? > > > > I think you are assuming that I want to commit the code like this, but what > I'm > > saying is that I am going to get this commented code completed before trying > to > > get this cl accepted. I'm not looking for you the accept everything as it is > > right now, rather i'd like you to ignore the TODO's and focus on reviewing the > > code that are no in TODO's. Sorry for the confusion. > > No apologies necessary. > > Usually, a TODO in an uploaded change means to the reviewer something that the > uploader is leaving for a subsequent change e.g. TODO(dtseng): implement this > missing funcionality. > > By commenting out this block, you're breaking things in other ways :). What's > been helpful for me (and I've not been super successful at this either) is to > stash any debugging or temporary changes `git stash` or committing it on an > experimental branch from my change. Before uploading the final change, I try and > squash my local commits together via `git rebase -i HEAD~x` and then reviewing > the squashed commit. > > > > > > > As for the last question, the current implementation definitely does not > support > > per-cell routing, so I'll need to add an offset of some sort. > > That's fine. Again, use the tests to help you here. Write a few and see where > that leads you in terms of teasing out the corner cases and testing some of your > own assumptions. Hey David, Reading through these code reviews, it's very apparent that both of us started to miscommunicate. We definitely should have pinged each other sooner to sync up offline! Let's work this out tomorrow in person.
Just a few more comments. We can go through them one by one in person. https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js (right): https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:75: this.fixedLineCount_ = 0; 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). https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:137: * the current braille slice. On 2016/11/15 17:54:59, David Tseng wrote: > @return {Array<number>} Disregard. https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:190: this.textBuffer = textBuffer; This isn't annotated and set in the constructor. https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:191: this.fixedBuffer_ = translatedContent; 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. https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:308: /* On 2016/11/15 22:25:42, ultimatedbz wrote: > On 2016/11/15 21:36:38, David Tseng wrote: > > On 2016/11/15 19:31:57, ultimatedbz wrote: > > > On 2016/11/15 17:55:00, David Tseng wrote: > > > > Don't do this, even in a work in progress cl. > > > > > > What should I do instead? The code would just break if I don't comment this > > out. > > > > 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? > > 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? 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 > 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... 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. > > 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.
https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js (right): https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:167: this.panStrategy_.setDisplaySize(newRowCount, newColumnCount); On 2016/11/15 17:54:59, David Tseng wrote: > Make the display size a pair i.e. > @type {{rows: number, columns: number}} Done. The parameters are still the same, but the internal data structure is changed. https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:193: var brailleBuf = this.panStrategy_.getCurrentBrailleSlice(); On 2016/11/15 17:54:59, David Tseng wrote: > Slice seems wrong in terms of multiline; maybe call this > getCurrentBrailleViewportContents Done. https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:285: /* TODO On 2016/11/15 21:58:09, ultimatedbz wrote: > On 2016/11/15 21:36:38, David Tseng wrote: > > On 2016/11/15 19:31:57, ultimatedbz wrote: > > > On 2016/11/15 17:54:59, David Tseng wrote: > > > > The goal is to get this cl checked in, so please restore this block and > > > support > > > > at least the single line case. > > > > > > I do intend to complete this block, but for the time being, I wanted to get > > > something up for initial review. It's in a TODO block to signify that I'm > > still > > > not done with this method. Do you have any suggestions as to how I should go > > > about doing this in the future? Either explicitly saying I'm going to do > this > > > later on, or to not submit this cl until everything is complete? > > > > I would not accept this cl without this working as it did before. You should > > never comment out code like this as much as it might make things easier. > > > > Routing is just another way of addressing a braille display's physical cells. > If > > I have a 40-cell display and click on the 20th cell, Brltty will send an event > > which eventually gets here. The question is, how to address that position into > > the viewport. > > > > If I panned 40 cells forward, you would obviously offset the above by 40 > giving > > us 60. > > > > In a multiline world, and your current implementation, how would you translate > a > > hardware key press over a cell into a valid position within your Offset by > > start * columns?viewport and keep the single line case working as it did > before? > > I think you are assuming that I want to commit the code like this, but what I'm > saying is that I am going to get this commented code completed before trying to > get this cl accepted. I'm not looking for you the accept everything as it is > right now, rather i'd like you to ignore the TODO's and focus on reviewing the > code that are no in TODO's. Sorry for the confusion. > > As for the last question, the current implementation definitely does not support > per-cell routing, so I'll need to add an offset of some sort. As per our offline conversation, I will leave the TODO but uncomment out this part of the code! https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:285: /* TODO On 2016/11/15 21:36:38, David Tseng wrote: > On 2016/11/15 19:31:57, ultimatedbz wrote: > > On 2016/11/15 17:54:59, David Tseng wrote: > > > The goal is to get this cl checked in, so please restore this block and > > support > > > at least the single line case. > > > > I do intend to complete this block, but for the time being, I wanted to get > > something up for initial review. It's in a TODO block to signify that I'm > still > > not done with this method. Do you have any suggestions as to how I should go > > about doing this in the future? Either explicitly saying I'm going to do this > > later on, or to not submit this cl until everything is complete? > > I would not accept this cl without this working as it did before. You should > never comment out code like this as much as it might make things easier. > > Routing is just another way of addressing a braille display's physical cells. If > I have a 40-cell display and click on the 20th cell, Brltty will send an event > which eventually gets here. The question is, how to address that position into > the viewport. > > If I panned 40 cells forward, you would obviously offset the above by 40 giving > us 60. > > In a multiline world, and your current implementation, how would you translate a > hardware key press over a cell into a valid position within your Offset by > start * columns?viewport and keep the single line case working as it did before? I just implemented a solution to your problem. I'm going to do event.displayPosition = this.brailleToTextPosition_( event.displayPosition + this.panStrategy_.viewPort.firstRow * this.panStrategy_.displaySize.columns); I can explain in person! https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js (left): https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:67: this.panToPosition_(this.viewPort_.start); On 2016/11/15 17:54:59, David Tseng wrote: > You'll want ot keep this around I suspect. Acknowledged. https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js (right): https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:51: * @type {Array} On 2016/11/15 17:55:00, David Tseng wrote: > Array<number> Done. https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:57: * Start and end are both inclusive. On 2016/11/15 17:55:00, David Tseng wrote: > Fix these comments. Done. https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:75: this.fixedLineCount_ = 0; On 2016/11/16 16:56:01, David Tseng wrote: > 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). Done. https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:104: * @return {Array<number>} The offset of braille and text indices of the On 2016/11/15 17:55:00, David Tseng wrote: > Make this an Object e.g. {brailleOffset: number, textOffset: nuimber} Done. https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:114: * corresponding text character. On 2016/11/15 17:55:00, David Tseng wrote: > @return {Array<number} I'm not sure what you're suggesting? Thanks! https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:129: this.wrappedBuffer_ : this.fixedBuffer_; On 2016/11/15 17:54:59, David Tseng wrote: > 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. 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. https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:130: buf = buf.slice(this.viewPort.start * this.columnCount_, On 2016/11/15 17:54:59, David Tseng wrote: > return buff.slice... Done. https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:131: (this.viewPort.end + 1) * this.columnCount_); On 2016/11/15 17:55:00, David Tseng wrote: > 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. How about firstRow and lastRow? https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:190: this.textBuffer = textBuffer; On 2016/11/16 16:56:01, David Tseng wrote: > This isn't annotated and set in the constructor. Done. https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:191: this.fixedBuffer_ = translatedContent; On 2016/11/16 16:56:01, David Tseng wrote: > 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. 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. https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:205: // On Delete all 0's at beginning of new line. On 2016/11/15 17:55:00, David Tseng wrote: > Rephrase Done. https://codereview.chromium.org/2496823002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy.js:308: /* On 2016/11/16 16:56:01, David Tseng wrote: > On 2016/11/15 22:25:42, ultimatedbz wrote: > > On 2016/11/15 21:36:38, David Tseng wrote: > > > On 2016/11/15 19:31:57, ultimatedbz wrote: > > > > On 2016/11/15 17:55:00, David Tseng wrote: > > > > > Don't do this, even in a work in progress cl. > > > > > > > > What should I do instead? The code would just break if I don't comment > this > > > out. > > > > > > 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? > > > > 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? > > 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 > > 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... > > 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. > > > > > 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. > Yep, this makes a lot more sense, and I'm glad we resolved this in person!
Looking forward to the tests. I'll take another pass through when those are ready.
Patchset #5 (id:80001) has been deleted
On 2016/11/21 23:57:47, David Tseng wrote: > Looking forward to the tests. I'll take another pass through when those are > ready. Tests are ready. Please take a look when you have time :)
Almost there; here are some comments to start. https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js (left): https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:374: console.error('WARNING: Braille position < 0: ' + braillePosition); Doesn't seem like this should be removed. https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js (right): https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:177: * @private */ nit: move */ to a new line https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:183: var textBuf = this.panStrategy_.getCurrentTextSlice(); Maybe make this getCurrentTextViewPortContents https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:239: cells, brailleToText, targetPosition); Optional: to help the reviewer and any future readers, pass a json-object to this function e.g. setContent({text: text, braille: braille, ...}) https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager_test.unitjs (right): https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager_test.unitjs:77: console.log(a[2]); nit: remove debug logging. https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/chromevox/braille/pan_strategy_test.unitjs (right): https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy_test.unitjs:28: assertEquals(JSON.stringify(arrayA), JSON.stringify(arrayB)); Add these to testing/assert_additions.js Also, I would compare each key/value, to ease future debugging. https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy_test.unitjs:58: var brailleToText = [0, 0, 0, 0, 0, 0,]; Unused, remove. https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy_test.unitjs:59: panner.setContent('a', translatedContent, [], 0); If 'a' is unused, pass it as 'unused' to not set up any expectations. Or, pass translatedContent https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy_test.unitjs:65: assertEqualsJSON({firstRow: 0, lastRow: 0}, panner.viewPort); Assert column size is 10 to keep the spirit of the previous assertion. https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy_test.unitjs:97: nit: remove
https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/chromevox/braille/pan_strategy_test.unitjs (right): https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy_test.unitjs:126: assertEqualsJSON({firstRow: 0, lastRow: 0}, panner.viewPort); I would also test the individual slices and assert they are as you expect (since you no longer capture the absolute start/end of the buffers like it was before). e.g. assertEquals('01234...', panStrategy.getBrailleViewPortContents()) or assertEquals(translatedContent.slice(start, end), panStrategy.getBrailleViewPortContents()) ... start = 27, end = 30 etc. https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy_test.unitjs:221: this.assertArraysEquals(expectedMappingValue, panner.brailleToText); I would also add unit tests to exercise the new public methods you added to PanStrategy e.g. getBrailleViewPortContents, getCurrentTextSlice.
https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js (left): https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:374: console.error('WARNING: Braille position < 0: ' + braillePosition); On 2016/11/30 22:11:54, David Tseng wrote: > Doesn't seem like this should be removed. Thanks for catching this! https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js (right): https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:177: * @private */ On 2016/11/30 22:11:54, David Tseng wrote: > nit: move */ to a new line Done. https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:183: var textBuf = this.panStrategy_.getCurrentTextSlice(); On 2016/11/30 22:11:54, David Tseng wrote: > Maybe make this getCurrentTextViewPortContents Done. https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:239: cells, brailleToText, targetPosition); On 2016/11/30 22:11:54, David Tseng wrote: > Optional: to help the reviewer and any future readers, pass a json-object to > this function e.g. > setContent({text: text, braille: braille, ...}) Acknowledged. https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager_test.unitjs (right): https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager_test.unitjs:77: console.log(a[2]); On 2016/11/30 22:11:54, David Tseng wrote: > nit: remove debug logging. Done. https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/chromevox/braille/pan_strategy_test.unitjs (right): https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy_test.unitjs:58: var brailleToText = [0, 0, 0, 0, 0, 0,]; On 2016/11/30 22:11:55, David Tseng wrote: > Unused, remove. Done. https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy_test.unitjs:59: panner.setContent('a', translatedContent, [], 0); On 2016/11/30 22:11:54, David Tseng wrote: > If 'a' is unused, pass it as 'unused' to not set up any expectations. Or, pass > translatedContent Done. https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy_test.unitjs:65: assertEqualsJSON({firstRow: 0, lastRow: 0}, panner.viewPort); On 2016/11/30 22:11:54, David Tseng wrote: > Assert column size is 10 to keep the spirit of the previous assertion. Done. https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy_test.unitjs:97: On 2016/11/30 22:11:55, David Tseng wrote: > nit: remove Done. https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy_test.unitjs:126: assertEqualsJSON({firstRow: 0, lastRow: 0}, panner.viewPort); On 2016/11/30 22:21:52, David Tseng wrote: > I would also test the individual slices and assert they are as you expect (since > you no longer capture the absolute start/end of the buffers like it was before). > e.g. assertEquals('01234...', panStrategy.getBrailleViewPortContents()) or > assertEquals(translatedContent.slice(start, end), > panStrategy.getBrailleViewPortContents()) > ... > start = 27, end = 30 > etc. The current function 'createArrayBuffer()' takes in a string and turns spaces into 0, and everything else into 1. (ex. '01234 6789' to [1,1,1,1,1,0,1,1,1,1]. I can still check the equality of the two, but it won't be as accurate. I think I will change the createArrayBuffer() function to create an array buffer with the original values. https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy_test.unitjs:221: this.assertArraysEquals(expectedMappingValue, panner.brailleToText); On 2016/11/30 22:21:52, David Tseng wrote: > I would also add unit tests to exercise the new public methods you added to > PanStrategy e.g. getBrailleViewPortContents, getCurrentTextSlice. I believe that by testing for the current slices above, I'd already be testing for getBrailleViewportContents(). (Will be included in next patch set) Do you think we should still have tests solely for this function? I agree that I should write tests for getCurrentTextSlice() though!
The CQ bit was checked by dtseng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ping - David, could you respond to Jeffrey's questions? Jeffrey, you mentioned that a unit test for getCurrentTextSlice would be a good idea, could you do that?
https://codereview.chromium.org/2496823002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager_test.unitjs (right): https://codereview.chromium.org/2496823002/diff/160001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager_test.unitjs:287: translated, text, mapping, offsets); nit: replace tabs with spaces https://codereview.chromium.org/2496823002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/chromeos/chromevox/braille/pan_strategy_test.unitjs (right): https://codereview.chromium.org/2496823002/diff/160001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy_test.unitjs:139: panner.getCurrentBrailleViewportContents()); same, more tabs https://codereview.chromium.org/2496823002/diff/160001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy_test.unitjs:151: // Test Multi-line Panning. Not a big deal, but I'd start a new test here rather than making it part of WrappedPanning. In the future if someone's changing something and a test fails it will be helpful to know if single-line or multi-line broke or both, just by seeing what tests failed. https://codereview.chromium.org/2496823002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/chromeos/chromevox/chromevox/background/braille_captions_background.js (right): https://codereview.chromium.org/2496823002/diff/160001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/chromevox/background/braille_captions_background.js:155: * @param {Function} callback The callback to pass the display state into. Nit: you can specify the type of the callback function, something like: @param {function(!cvox.BrailleDisplayState)}
https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/chromevox/braille/pan_strategy_test.unitjs (right): https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy_test.unitjs:126: assertEqualsJSON({firstRow: 0, lastRow: 0}, panner.viewPort); On 2016/12/01 05:12:15, ultimatedbz wrote: > On 2016/11/30 22:21:52, David Tseng wrote: > > I would also test the individual slices and assert they are as you expect > (since > > you no longer capture the absolute start/end of the buffers like it was > before). > > e.g. assertEquals('01234...', panStrategy.getBrailleViewPortContents()) or > > assertEquals(translatedContent.slice(start, end), > > panStrategy.getBrailleViewPortContents()) > > ... > > start = 27, end = 30 > > etc. > > The current function 'createArrayBuffer()' takes in a string and turns spaces > into 0, and everything else into 1. (ex. '01234 6789' to [1,1,1,1,1,0,1,1,1,1]. > I can still check the equality of the two, but it won't be as accurate. I think > I will change the createArrayBuffer() function to create an array buffer with > the original values. Ok. The only concern was that we're just testing viewPort and not the absolute offsets into the buffer. Another probably more concise way to express things is something like: assertBrailleContent({ firstRow: 2, lastRow: 4, content: ['123456712345678...', '123 123 123 ...' ]}, panner); That way, you can very quickly assert the multi line layout you expect. Optional though. https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy_test.unitjs:221: this.assertArraysEquals(expectedMappingValue, panner.brailleToText); On 2016/12/01 05:12:15, ultimatedbz wrote: > On 2016/11/30 22:21:52, David Tseng wrote: > > I would also add unit tests to exercise the new public methods you added to > > PanStrategy e.g. getBrailleViewPortContents, getCurrentTextSlice. > > I believe that by testing for the current slices above, I'd already be testing > for getBrailleViewportContents(). (Will be included in next patch set) Do you > think we should still have tests solely for this function? I agree that I should > write tests for getCurrentTextSlice() though! Sgtm For organizational purposes, one test per function sounds good.
On 2016/12/07 06:42:10, dmazzoni wrote: > Ping - David, could you respond to Jeffrey's questions? > > Jeffrey, you mentioned that a unit test for getCurrentTextSlice > would be a good idea, could you do that? Hey Dominic, Yes, they've actually already been included in this patchset. David recommended that we change the name of the function, so that's probably why it was harder to find. It's in the pan strategy tests under 'getCurrentTextViewportContents'. Thanks!
https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/chromevox/braille/pan_strategy_test.unitjs (right): https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy_test.unitjs:126: assertEqualsJSON({firstRow: 0, lastRow: 0}, panner.viewPort); On 2016/12/07 16:46:36, David Tseng wrote: > On 2016/12/01 05:12:15, ultimatedbz wrote: > > On 2016/11/30 22:21:52, David Tseng wrote: > > > I would also test the individual slices and assert they are as you expect > > (since > > > you no longer capture the absolute start/end of the buffers like it was > > before). > > > e.g. assertEquals('01234...', panStrategy.getBrailleViewPortContents()) or > > > assertEquals(translatedContent.slice(start, end), > > > panStrategy.getBrailleViewPortContents()) > > > ... > > > start = 27, end = 30 > > > etc. > > > > The current function 'createArrayBuffer()' takes in a string and turns spaces > > into 0, and everything else into 1. (ex. '01234 6789' to > [1,1,1,1,1,0,1,1,1,1]. > > I can still check the equality of the two, but it won't be as accurate. I > think > > I will change the createArrayBuffer() function to create an array buffer with > > the original values. > > Ok. > > The only concern was that we're just testing viewPort and not the absolute > offsets into the buffer. > > Another probably more concise way to express things is something like: > assertBrailleContent({ > firstRow: 2, lastRow: 4, > content: > ['123456712345678...', > '123 123 123 ...' > ]}, panner); > > That way, you can very quickly assert the multi line layout you expect. > > Optional though. Acknowledged. https://codereview.chromium.org/2496823002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager_test.unitjs (right): https://codereview.chromium.org/2496823002/diff/160001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager_test.unitjs:287: translated, text, mapping, offsets); On 2016/12/07 06:48:56, dmazzoni wrote: > nit: replace tabs with spaces Done. https://codereview.chromium.org/2496823002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/chromeos/chromevox/braille/pan_strategy_test.unitjs (right): https://codereview.chromium.org/2496823002/diff/160001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy_test.unitjs:139: panner.getCurrentBrailleViewportContents()); On 2016/12/07 06:48:56, dmazzoni wrote: > same, more tabs Done. https://codereview.chromium.org/2496823002/diff/160001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy_test.unitjs:151: // Test Multi-line Panning. On 2016/12/07 06:48:56, dmazzoni wrote: > Not a big deal, but I'd start a new test here rather than making it part of > WrappedPanning. > In the future if someone's changing something and a test fails it will be > helpful to know if > single-line or multi-line broke or both, just by seeing what tests failed. > Done. https://codereview.chromium.org/2496823002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/chromeos/chromevox/chromevox/background/braille_captions_background.js (right): https://codereview.chromium.org/2496823002/diff/160001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/chromevox/background/braille_captions_background.js:155: * @param {Function} callback The callback to pass the display state into. On 2016/12/07 06:48:56, dmazzoni wrote: > Nit: you can specify the type of the callback function, something like: > > @param {function(!cvox.BrailleDisplayState)} Done.
https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/chromevox/braille/pan_strategy_test.unitjs (right): https://codereview.chromium.org/2496823002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/braille/pan_strategy_test.unitjs:28: assertEquals(JSON.stringify(arrayA), JSON.stringify(arrayB)); On 2016/11/30 22:11:54, David Tseng wrote: > Add these to testing/assert_additions.js > > Also, I would compare each key/value, to ease future debugging. Done.
lgtm
The CQ bit was checked by ultimatedbz@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1481220656828160,
"parent_rev": "808f176e5fb45cb333fb07750908bbda5aaba9a0", "commit_rev":
"1e66117dec44b6e7dd155aabd5f85c9c1702b64e"}
Message was sent while issue was closed.
Description was changed from ========== Implement word wrapping and panning in multiline Braille. The current code doesn't support word wrapping when displaying multiline braille. The panStrategy class also doesn't allow panning in multiline. This CL changes the panStrategy completely. Before, it only returns starting and ending indices of the current viewport. Now, it stores the fixed and wrapped versions of the braille that is displayed. It also keeps the state of whether the pan strategy is currently "fixed" or "wrapped". The panStrategy class also now supports panning in multiline. Changes to the braille display manager have been made to utilize the new panStrategy. In addition, changes have been made to the braille captions background so that grouping works, even if the viewport is not at the beginning of the buffer. Specifically, it uses two offsets that the panStrategy class generates to find the correct indices for braille and text. BUG=661199 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Implement word wrapping and panning in multiline Braille. The current code doesn't support word wrapping when displaying multiline braille. The panStrategy class also doesn't allow panning in multiline. This CL changes the panStrategy completely. Before, it only returns starting and ending indices of the current viewport. Now, it stores the fixed and wrapped versions of the braille that is displayed. It also keeps the state of whether the pan strategy is currently "fixed" or "wrapped". The panStrategy class also now supports panning in multiline. Changes to the braille display manager have been made to utilize the new panStrategy. In addition, changes have been made to the braille captions background so that grouping works, even if the viewport is not at the beginning of the buffer. Specifically, it uses two offsets that the panStrategy class generates to find the correct indices for braille and text. BUG=661199 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Implement word wrapping and panning in multiline Braille. The current code doesn't support word wrapping when displaying multiline braille. The panStrategy class also doesn't allow panning in multiline. This CL changes the panStrategy completely. Before, it only returns starting and ending indices of the current viewport. Now, it stores the fixed and wrapped versions of the braille that is displayed. It also keeps the state of whether the pan strategy is currently "fixed" or "wrapped". The panStrategy class also now supports panning in multiline. Changes to the braille display manager have been made to utilize the new panStrategy. In addition, changes have been made to the braille captions background so that grouping works, even if the viewport is not at the beginning of the buffer. Specifically, it uses two offsets that the panStrategy class generates to find the correct indices for braille and text. BUG=661199 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Implement word wrapping and panning in multiline Braille. The current code doesn't support word wrapping when displaying multiline braille. The panStrategy class also doesn't allow panning in multiline. This CL changes the panStrategy completely. Before, it only returns starting and ending indices of the current viewport. Now, it stores the fixed and wrapped versions of the braille that is displayed. It also keeps the state of whether the pan strategy is currently "fixed" or "wrapped". The panStrategy class also now supports panning in multiline. Changes to the braille display manager have been made to utilize the new panStrategy. In addition, changes have been made to the braille captions background so that grouping works, even if the viewport is not at the beginning of the buffer. Specifically, it uses two offsets that the panStrategy class generates to find the correct indices for braille and text. BUG=661199 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/2a59dc298b6f036467777ae3658fe8546f0a5978 Cr-Commit-Position: refs/heads/master@{#437318} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/2a59dc298b6f036467777ae3658fe8546f0a5978 Cr-Commit-Position: refs/heads/master@{#437318} |
