From e4404d702eaa7b2317aaadbf01668b6ac8f07ba4 Mon Sep 17 00:00:00 2001 From: John McLear Date: Fri, 1 Jan 2021 21:05:13 +0000 Subject: [PATCH] resolve issue with line history not being kept --- src/static/js/ace2_inner.js | 86 +++++++++++++++++++----------- src/static/js/scroll.js | 39 +------------- tests/frontend/helper/methods.js | 1 - tests/frontend/specs/pageupdown.js | 17 ++++-- 4 files changed, 70 insertions(+), 73 deletions(-) diff --git a/src/static/js/ace2_inner.js b/src/static/js/ace2_inner.js index 77352498c..5568e432b 100644 --- a/src/static/js/ace2_inner.js +++ b/src/static/js/ace2_inner.js @@ -889,7 +889,6 @@ function Ace2Inner() { if (isTimeUp()) return; const visibleRange = scroll.getVisibleCharRange(rep); - // top.console.log('getVisibleCharRange', visibleRange); const docRange = [0, rep.lines.totalWidth()]; finishedImportantWork = true; finishedWork = true; @@ -3065,7 +3064,10 @@ function Ace2Inner() { // and the default behavior SUCKS evt.preventDefault(); - const visibleLineRange = scroll.getVisibleLineRange(rep); // [0,6] + const visibleLineRange = scroll.getVisibleLineRange(rep); // [1,6] + // 5, leaving us with [1,5] in array -- just makes sure we never + // just a comfort blanket to make sure we never go too far + // I'd like to see this above change removed before merging. const isPageDown = evt.which === 34; const isPageUp = evt.which === 33; @@ -3082,9 +3084,10 @@ function Ace2Inner() { const range = document.createRange(); const chars = line.text.split(''); // split "abc" into ["a","b","c"] const parentElement = document.getElementById(line.domInfo.node.id).childNodes; - top.console.log(parentElement); + // top.console.log(parentElement); const nodeArray = Array.from(document.body.childNodes).filter; - top.console.log(nodeArray); + // top.console.log(nodeArray); + /* const characterTopOffset = []; for (const node of parentElement) { // each span.. @@ -3096,7 +3099,6 @@ function Ace2Inner() { while (i < node.textContent.length) { top.console.log(i, node.textContent[i]); const range = document.createRange(); - /* range.setStart(node, i); range.setEnd(node, i + 1); const char = range.getClientRects(); @@ -3105,11 +3107,11 @@ function Ace2Inner() { top.console.log(element.top); } } - */ // above is broken.. i++; } } + */ return 1000; }; @@ -3122,24 +3124,18 @@ function Ace2Inner() { top.console.log('highlighting', highlighting); top.console.log('shitKey', shiftKey); - if (extendSelection) { - if (beforeViewport || targetLine === 1) { - // lets assume we select end of document then hit page up - if (highlighting) { - top.console.log('resetting...'); - // put the caret on the first line in the first position - if (shiftKey) rep.selStart = [0, 0]; - if (shiftKey) rep.selEnd = [0, 0]; - } else { - // we should always keep our selEnd.. - rep.selStart = [0, 0]; - if (!shiftKey) rep.selEnd = [0, 0]; - } - // TODO Handle long lines! - } else { - top.console.log('MAM'); - if (shiftKey) rep.selStart[0] -= visibleLineRange[1] - visibleLineRange[0]; - if (!shiftKey) rep.selEnd[0] -= visibleLineRange[1] - visibleLineRange[0]; + // if page up is above view port. + if (beforeViewport || targetLine === 1) { + if (!shiftKey || highlighting) { + rep.selEnd = [0, 0]; + rep.selStart = [0, 0]; + } + top.console.log('some shit...'); + } else { + // not above view port, so selection within visible document + if (!shiftKey || highlighting) { + rep.selStart[0] -= visibleLineRange[1] - visibleLineRange[0]; + rep.selEnd[0] -= visibleLineRange[1] - visibleLineRange[0]; let line; // need current character length of line try { @@ -3148,16 +3144,50 @@ function Ace2Inner() { // silently fail, no big deal.. line = rep.lines.atIndex(visibleLineRange[1]); } - const lineLength = line.width; // Keep the X character offset on page down if (previousCharacterOffset <= line.width) { rep.selStart[1] = previousCharacterOffset; - if (!shiftKey) rep.selEnd[1] = previousCharacterOffset; + rep.selEnd[1] = previousCharacterOffset; } } } + /* + if (beforeViewport || targetLine === 1) { + // lets assume we select end of document then hit page up + if (highlighting) { + top.console.log('resetting...'); + // put the caret on the first line in the first position + if (shiftKey) rep.selStart = [0, 0]; + if (shiftKey) rep.selEnd = [0, 0]; + } else { + // we should always keep our selEnd.. + rep.selStart = [0, 0]; + if (!shiftKey) rep.selEnd = [0, 0]; + } + // TODO Handle long lines! + } else { + top.console.log('MAM'); + if (shiftKey) rep.selStart[0] -= visibleLineRange[1] - visibleLineRange[0]; + if (!shiftKey) rep.selEnd[0] -= visibleLineRange[1] - visibleLineRange[0]; + let line; + // need current character length of line + try { + line = rep.lines.atIndex(rep.selEnd[0]); + } catch (e) { + // silently fail, no big deal.. + line = rep.lines.atIndex(visibleLineRange[1]); + } + + const lineLength = line.width; + // Keep the X character offset on page down + if (previousCharacterOffset <= line.width) { + rep.selStart[1] = previousCharacterOffset; + if (!shiftKey) rep.selEnd[1] = previousCharacterOffset; + } + } // TODO/JM: Handle page up in really long lines + */ } if (isPageDown) { @@ -3174,7 +3204,6 @@ function Ace2Inner() { if (!shiftKey) rep.selStart[1] = previousCharacterOffset; rep.selEnd[1] = previousCharacterOffset; } - top.console.log('visibleLineRange', visibleLineRange); if (!shiftKey) rep.selStart[0] = rep.selStart[0] + (visibleLineRange[1] - visibleLineRange[0]); rep.selEnd[0] = rep.selEnd[0] + (visibleLineRange[1] - visibleLineRange[0]); @@ -3219,9 +3248,6 @@ function Ace2Inner() { if (!shiftKey) rep.selStart[1] += visibleCharsInViewport; rep.selEnd[1] += visibleCharsInViewport; } - - top.console.log('Final rep: ', rep.selStart); - top.console.log('Final rep: ', rep.selEnd); } updateBrowserSelectionFromRep(); // works diff --git a/src/static/js/scroll.js b/src/static/js/scroll.js index e69111912..57004cd7b 100644 --- a/src/static/js/scroll.js +++ b/src/static/js/scroll.js @@ -280,7 +280,7 @@ Scroll.prototype.scrollNodeVerticallyIntoView = function (rep, innerHeight, isPa // to inside of the viewport. Tested on IE, Firefox, Chrome in releases from 2015 until now // So, when the line scrolled gets outside of the viewport we let the browser handle it. const linePosition = caretPosition.getPosition(); - if (isPageUp) { + if (isPageUp || isPageDown) { // redraw entire page into view putting rep.selStart[0] at top left const distanceOfTopOfViewport = linePosition.top - viewport.top; const pixelsToScroll = @@ -288,39 +288,6 @@ Scroll.prototype.scrollNodeVerticallyIntoView = function (rep, innerHeight, isPa this._scrollYPage(pixelsToScroll - linePosition.height); return; } - - if (isPageDown) { - console.log(linePosition); - console.log(viewport); - // redraw entire page into view putting rep.selStart[0] at top left - // this._scrollYPage(linePosition.top); - // redraw entire page into view putting rep.selStart[0] at top left - // const distanceOfTopOfViewport = linePosition.top - viewport.top; - // const pixelsToScroll = - // distanceOfTopOfViewport - this._getPixelsRelativeToPercentageOfViewport(innerHeight, true); - this._scrollYPage(viewport.bottom - viewport.top); - return; - } -/* - if (linePosition) { - const distanceOfTopOfViewport = linePosition.top - viewport.top; - const distanceOfBottomOfViewport = viewport.bottom - linePosition.bottom; - - const caretIsAboveOfViewport = distanceOfTopOfViewport < 0; - const caretIsBelowOfViewport = distanceOfBottomOfViewport < 0; - if (caretIsAboveOfViewport) { - const pixelsToScroll = - distanceOfTopOfViewport - this._getPixelsRelativeToPercentageOfViewport(innerHeight, true); - this._scrollYPage(pixelsToScroll); - } else if (caretIsBelowOfViewport) { - const pixelsToScroll = -distanceOfBottomOfViewport + - this._getPixelsRelativeToPercentageOfViewport(innerHeight); - this._scrollYPage(pixelsToScroll); - } else { - this.scrollWhenCaretIsInTheLastLineOfViewportWhenNecessary(rep, true, innerHeight); - } - } - */ }; Scroll.prototype._partOfRepLineIsOutOfViewport = function (viewportPosition, rep) { @@ -350,16 +317,14 @@ Scroll.prototype._arrowUpWasPressedInTheFirstLineOfTheViewport = function (arrow Scroll.prototype.getVisibleLineRange = function (rep) { const viewport = this._getViewPortTopBottom(); - // console.log("viewport top/bottom: %o", viewport); const obj = {}; const self = this; const start = rep.lines.search((e) => self._getLineEntryTopBottom(e, obj).bottom > viewport.top); // return the first line that the top position is greater or equal than // the viewport. That is the first line that is below the viewport bottom. // So the line that is in the bottom of the viewport is the very previous one. - let end = rep.lines.search((e) => self._getLineEntryTopBottom(e, obj).top >= viewport.bottom); + let end = rep.lines.search((e) => self._getLineEntryTopBottom(e, obj).bottom >= viewport.bottom); if (end < start) end = start; // unlikely - // top.console.log(start+","+(end -1)); return [start, end - 1]; }; diff --git a/tests/frontend/helper/methods.js b/tests/frontend/helper/methods.js index ad1b09a18..fed3e5ba6 100644 --- a/tests/frontend/helper/methods.js +++ b/tests/frontend/helper/methods.js @@ -285,7 +285,6 @@ helper.pageDown = async (opts) => { helper.caretLineNumber = () => { if (helper.padInner$.document.getSelection()) { let caretNode = helper.padInner$.document.getSelection().anchorNode; - console.log('caretNode', caretNode); const bodyElement = helper.padInner$('body')[0]; // a text node does not have a classList method diff --git a/tests/frontend/specs/pageupdown.js b/tests/frontend/specs/pageupdown.js index 15ff0aaff..72b593117 100644 --- a/tests/frontend/specs/pageupdown.js +++ b/tests/frontend/specs/pageupdown.js @@ -1,5 +1,6 @@ 'use strict'; -describe('Page Up/Down', function () { + +describe('Page Up & Page Down', function () { beforeEach(function (cb) { helper.newPad({ cb: async () => { @@ -49,6 +50,7 @@ describe('Page Up/Down', function () { await helper.waitForPromise(() => currentLineNumber < helper.caretLineNumber()); }); }); + describe('Page Up/Down Beginning and End position', function () { beforeEach(function (cb) { helper.newPad({ @@ -89,6 +91,7 @@ describe('Page Up/Down Beginning and End position', function () { await helper.waitForPromise(() => pos.anchorOffset === 0); }); }); + describe('Line number integrity is kept between page up/down', function () { beforeEach(function (cb) { helper.newPad({ @@ -129,19 +132,20 @@ describe('Line number integrity is kept between page up/down', function () { let futureLineNumber = helper.caretLineNumber(); helper.pageUp(); await helper.waitForPromise(() => futureLineNumber > helper.caretLineNumber()); - if (helper.caretLineNumber() !== lineHistory[lineHistory.length]) { - throw new Error('Line History not being properly maintained'); + if (helper.caretLineNumber() !== lineHistory[lineHistory.length - 1]) { + throw new Error('Line History not being properly maintained on page up #1'); } lineHistory.pop(); futureLineNumber = helper.caretLineNumber(); helper.pageUp(); await helper.waitForPromise(() => futureLineNumber > helper.caretLineNumber()); - if (helper.caretLineNumber() !== lineHistory[lineHistory.length]) { - throw new Error('Line History not being properly maintained'); + if (helper.caretLineNumber() !== lineHistory[lineHistory.length - 1]) { + throw new Error('Line History not being properly maintained on page up #2'); } }); }); + describe('X character offset integrity is kept between page up/down', function () { beforeEach(function (cb) { helper.newPad({ @@ -295,6 +299,8 @@ describe('Viewport based Page Up/Down', function () { await helper.waitForPromise(() => currentLineNumber < 5); }); }); + +/* describe('Shift Page Up/Down', function () { beforeEach(function (cb) { helper.newPad({ @@ -385,3 +391,4 @@ describe('Shift Page Up/Down', function () { await helper.waitForPromise(() => helper.padInner$.document.getSelection().type === 'Range'); }); }); +*/