From 58c315476954b0246bcf555e2069602a804ceb66 Mon Sep 17 00:00:00 2001 From: Luiza Pagliari Date: Mon, 9 Jul 2018 17:44:38 -0300 Subject: [PATCH] [fix] Ignore default line attribs when detecting edges of changeset (#3420) When comparing original content with the changes made by the user, we need to ignore some line attribs that are added by content collector, otherwise we would consider the change started on the first char of the line -- the '*' that is added when line has line attribs. In order to be able to handle both #3354 and #3118, we need to take into account both the styles attribs (to fix #3354) and the line attribs defined by any of the plugins (to fix #3118), but we can ignore those extra line attribs that are added by Etherpad and do not add any functionality (`'lmkr', 'insertorder', 'start'`). --- src/static/js/AttributeManager.js | 7 +- src/static/js/ace2_inner.js | 13 +-- .../frontend/specs/authorship_of_editions.js | 105 ++++++++++++++++++ 3 files changed, 117 insertions(+), 8 deletions(-) create mode 100644 tests/frontend/specs/authorship_of_editions.js diff --git a/src/static/js/AttributeManager.js b/src/static/js/AttributeManager.js index 53b233e0..17b21662 100644 --- a/src/static/js/AttributeManager.js +++ b/src/static/js/AttributeManager.js @@ -4,6 +4,10 @@ var _ = require('./underscore'); var lineMarkerAttribute = 'lmkr'; +// Some of these attributes are kept for compatibility purposes. +// Not sure if we need all of them +var DEFAULT_LINE_ATTRIBUTES = ['author', 'lmkr', 'insertorder', 'start']; + // If one of these attributes are set to the first character of a // line it is considered as a line attribute marker i.e. attributes // set on this marker are applied to the whole line. @@ -35,6 +39,7 @@ var AttributeManager = function(rep, applyChangesetCallback) // it will be considered as a line marker }; +AttributeManager.DEFAULT_LINE_ATTRIBUTES = DEFAULT_LINE_ATTRIBUTES; AttributeManager.lineAttributes = lineAttributes; AttributeManager.prototype = _(AttributeManager.prototype).extend({ @@ -375,7 +380,7 @@ AttributeManager.prototype = _(AttributeManager.prototype).extend({ ChangesetUtils.buildKeepToStartOfRange(this.rep, builder, [lineNum, 0]); var countAttribsWithMarker = _.chain(attribs).filter(function(a){return !!a[1];}) - .map(function(a){return a[0];}).difference(['author', 'lmkr', 'insertorder', 'start']).size().value(); + .map(function(a){return a[0];}).difference(DEFAULT_LINE_ATTRIBUTES).size().value(); //if we have marker and any of attributes don't need to have marker. we need delete it if(hasMarker && !countAttribsWithMarker){ diff --git a/src/static/js/ace2_inner.js b/src/static/js/ace2_inner.js index d7ec4326..8b0e2c3e 100644 --- a/src/static/js/ace2_inner.js +++ b/src/static/js/ace2_inner.js @@ -1778,19 +1778,15 @@ function Ace2Inner(){ strikethrough: true, list: true }; - var OTHER_INCORPED_ATTRIBS = { - insertorder: true, - author: true - }; function isStyleAttribute(aname) { return !!STYLE_ATTRIBS[aname]; } - function isOtherIncorpedAttribute(aname) + function isDefaultLineAttribute(aname) { - return !!OTHER_INCORPED_ATTRIBS[aname]; + return AttributeManager.DEFAULT_LINE_ATTRIBUTES.indexOf(aname) !== -1; } function insertDomLines(nodeToAddAfter, infoStructs, isTimeUp) @@ -2757,9 +2753,12 @@ function Ace2Inner(){ function analyzeChange(oldText, newText, oldAttribs, newAttribs, optSelStartHint, optSelEndHint) { + // we need to take into account both the styles attributes & attributes defined by + // the plugins, so basically we can ignore only the default line attribs used by + // Etherpad function incorpedAttribFilter(anum) { - return !isOtherIncorpedAttribute(rep.apool.getAttribKey(anum)); + return !isDefaultLineAttribute(rep.apool.getAttribKey(anum)); } function attribRuns(attribs) diff --git a/tests/frontend/specs/authorship_of_editions.js b/tests/frontend/specs/authorship_of_editions.js new file mode 100644 index 00000000..3bea2c46 --- /dev/null +++ b/tests/frontend/specs/authorship_of_editions.js @@ -0,0 +1,105 @@ +describe('author of pad edition', function() { + var REGULAR_LINE = 0; + var LINE_WITH_ORDERED_LIST = 1; + var LINE_WITH_UNORDERED_LIST = 2; + + // author 1 creates a new pad with some content (regular lines and lists) + before(function(done) { + var padId = helper.newPad(function() { + // make sure pad has at least 3 lines + var $firstLine = helper.padInner$('div').first(); + var threeLines = ['regular line', 'line with ordered list', 'line with unordered list'].join('
'); + $firstLine.html(threeLines); + + // wait for lines to be processed by Etherpad + helper.waitFor(function() { + var $lineWithUnorderedList = getLine(LINE_WITH_UNORDERED_LIST); + return $lineWithUnorderedList.text() === 'line with unordered list'; + }).done(function() { + // create the unordered list + var $lineWithUnorderedList = getLine(LINE_WITH_UNORDERED_LIST); + $lineWithUnorderedList.sendkeys('{selectall}'); + + var $insertUnorderedListButton = helper.padChrome$('.buttonicon-insertunorderedlist'); + $insertUnorderedListButton.click(); + + helper.waitFor(function() { + var $lineWithUnorderedList = getLine(LINE_WITH_UNORDERED_LIST); + return $lineWithUnorderedList.find('ul li').length === 1; + }).done(function() { + // create the ordered list + var $lineWithOrderedList = getLine(LINE_WITH_ORDERED_LIST); + $lineWithOrderedList.sendkeys('{selectall}'); + + var $insertOrderedListButton = helper.padChrome$('.buttonicon-insertorderedlist'); + $insertOrderedListButton.click(); + + helper.waitFor(function() { + var $lineWithOrderedList = getLine(LINE_WITH_ORDERED_LIST); + return $lineWithOrderedList.find('ol li').length === 1; + }).done(function() { + // Reload pad, to make changes as a second user. Need a timeout here to make sure + // all changes were saved before reloading + setTimeout(function() { + helper.newPad(done, padId); + }, 1000); + }); + }); + }); + }); + this.timeout(60000); + }); + + // author 2 makes some changes on the pad + it('marks only the new content as changes of the second user on a regular line', function(done) { + changeLineAndCheckOnlyThatChangeIsFromThisAuthor(REGULAR_LINE, 'x', done); + }); + + it('marks only the new content as changes of the second user on a line with ordered list', function(done) { + changeLineAndCheckOnlyThatChangeIsFromThisAuthor(LINE_WITH_ORDERED_LIST, 'y', done); + }); + + it('marks only the new content as changes of the second user on a line with unordered list', function(done) { + changeLineAndCheckOnlyThatChangeIsFromThisAuthor(LINE_WITH_UNORDERED_LIST, 'z', done); + }); + + /* ********************** Helper functions ************************ */ + var getLine = function(lineNumber) { + return helper.padInner$('div').eq(lineNumber); + } + + var getAuthorFromClassList = function(classes) { + return classes.find(function(cls) { + return cls.startsWith('author'); + }); + } + + var changeLineAndCheckOnlyThatChangeIsFromThisAuthor = function(lineNumber, textChange, done) { + // get original author class + var classes = getLine(lineNumber).find('span').first().attr('class').split(' '); + var originalAuthor = getAuthorFromClassList(classes); + + // make change on target line + var $regularLine = getLine(lineNumber); + helper.selectLines($regularLine, $regularLine, 2, 2); // place caret after 2nd char of line + $regularLine.sendkeys(textChange); + + // wait for change to be processed by Etherpad + var otherAuthorsOfLine; + helper.waitFor(function() { + var authorsOfLine = getLine(lineNumber).find('span').map(function() { + return getAuthorFromClassList($(this).attr('class').split(' ')); + }).get(); + otherAuthorsOfLine = authorsOfLine.filter(function(author) { + return author !== originalAuthor; + }); + var lineHasChangeOfThisAuthor = otherAuthorsOfLine.length > 0; + return lineHasChangeOfThisAuthor; + }).done(function() { + var thisAuthor = otherAuthorsOfLine[0]; + var $changeOfThisAuthor = getLine(lineNumber).find('span.' + thisAuthor); + expect($changeOfThisAuthor.text()).to.be(textChange); + done(); + }); + } +});