From c85e9ba3b1d1effd77625de999b34b1c972129bf Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Tue, 20 Feb 2018 13:13:24 +0000 Subject: [PATCH 1/7] Instead of setting value of CM, swap Docs This allows for tracking changes on individual docs (i.e. files), instead of just changes to the editor. This is similar to how Ace works with sessions --- .../public/coffee/ide/editor/directives/cmEditor.coffee | 5 +++-- services/web/public/es/rich-text.js | 8 +++++++- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/services/web/public/coffee/ide/editor/directives/cmEditor.coffee b/services/web/public/coffee/ide/editor/directives/cmEditor.coffee index a0e475688b..1241e54415 100644 --- a/services/web/public/coffee/ide/editor/directives/cmEditor.coffee +++ b/services/web/public/coffee/ide/editor/directives/cmEditor.coffee @@ -17,8 +17,9 @@ define [ attachToCM(sharejsDoc) attachToCM = (sharejsDoc) -> - cm.setValue(sharejsDoc.getSnapshot()) - sharejsDoc.attachToCM(cm) + setTimeout () -> + Frontend.richText.openDoc(cm, sharejsDoc.getSnapshot()) + sharejsDoc.attachToCM(cm) detachFromCM = (sharejsDoc) -> sharejsDoc.detachFromCM() diff --git a/services/web/public/es/rich-text.js b/services/web/public/es/rich-text.js index b07155829d..14399c1663 100644 --- a/services/web/public/es/rich-text.js +++ b/services/web/public/es/rich-text.js @@ -1,5 +1,11 @@ -import CodeMirror from 'codemirror' +import CodeMirror, { Doc } from 'codemirror' export function init(rootEl) { return CodeMirror(rootEl) } + +export function openDoc(cm, content) { + const newDoc = Doc(content) + cm.swapDoc(newDoc) + return newDoc +} From e1187f3d8a814688122bf8f7149e0d359c101307 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Tue, 20 Feb 2018 13:16:37 +0000 Subject: [PATCH 2/7] Listen for changes to the CodeMirror Doc, instead of all changes to the editor This prevents an issue where switching docs (i.e. files) would cause the newly opened doc from being inserted into the old doc. This approach is similar to Ace's sessions. --- .../coffee/ide/editor/sharejs/vendor/client/cm.coffee | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/services/web/public/coffee/ide/editor/sharejs/vendor/client/cm.coffee b/services/web/public/coffee/ide/editor/sharejs/vendor/client/cm.coffee index 7b32ed541d..d2cf8f2dce 100644 --- a/services/web/public/coffee/ide/editor/sharejs/vendor/client/cm.coffee +++ b/services/web/public/coffee/ide/editor/sharejs/vendor/client/cm.coffee @@ -36,6 +36,8 @@ window.sharejs.extendDoc 'attach_cm', (editor, keepEditorContents) -> throw new Error 'Only text documents can be attached to CodeMirror2' sharedoc = @ + editorDoc = editor.getDoc() + check = -> window.setTimeout -> editorText = editor.getValue() @@ -64,11 +66,10 @@ window.sharejs.extendDoc 'attach_cm', (editor, keepEditorContents) -> # Listen for edits in CodeMirror. editorListener = (ed, change) -> return if suppress - applyCMToShareJS editor, change, sharedoc - + applyCMToShareJS editorDoc, change, sharedoc check() - editor.on 'change', editorListener + editorDoc.on 'change', editorListener @on 'insert', (pos, text) -> suppress = true @@ -87,7 +88,7 @@ window.sharejs.extendDoc 'attach_cm', (editor, keepEditorContents) -> @detach_cm = -> # TODO: can we remove the insert and delete event callbacks? - editor.off 'onChange', editorListener + editorDoc.off 'change', editorListener delete @detach_cm return From c5735a31df5479bb00143083fe9ac21fbcc0510a Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Tue, 20 Feb 2018 13:24:00 +0000 Subject: [PATCH 3/7] Remove unused listener --- .../coffee/ide/editor/sharejs/vendor/client/ace.coffee | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/services/web/public/coffee/ide/editor/sharejs/vendor/client/ace.coffee b/services/web/public/coffee/ide/editor/sharejs/vendor/client/ace.coffee index 177af1317e..462aa456f0 100644 --- a/services/web/public/coffee/ide/editor/sharejs/vendor/client/ace.coffee +++ b/services/web/public/coffee/ide/editor/sharejs/vendor/client/ace.coffee @@ -87,15 +87,6 @@ window.sharejs.extendDoc 'attach_ace', (editor, keepEditorContents, maxDocLength editorDoc.on 'change', editorListener - # Listen for remote ops on the sharejs document - docListener = (op) -> - suppress = true - applyToDoc editorDoc, op - suppress = false - - check() - - # Horribly inefficient. offsetToPos = (offset) -> # Again, very inefficient. @@ -154,7 +145,6 @@ window.sharejs.extendDoc 'attach_ace', (editor, keepEditorContents, maxDocLength check() doc.detach_ace = -> - doc.removeListener 'remoteop', docListener editorDoc.removeListener 'change', editorListener delete doc.detach_ace From 598837e17c8212173b5a3cfd80287637fc359c6c Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Tue, 20 Feb 2018 15:03:43 +0000 Subject: [PATCH 4/7] Use applyAsync to prevent issue when switching docs --- .../web/public/coffee/ide/editor/directives/cmEditor.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/web/public/coffee/ide/editor/directives/cmEditor.coffee b/services/web/public/coffee/ide/editor/directives/cmEditor.coffee index 1241e54415..37a7724d85 100644 --- a/services/web/public/coffee/ide/editor/directives/cmEditor.coffee +++ b/services/web/public/coffee/ide/editor/directives/cmEditor.coffee @@ -17,7 +17,7 @@ define [ attachToCM(sharejsDoc) attachToCM = (sharejsDoc) -> - setTimeout () -> + scope.$applyAsync () -> Frontend.richText.openDoc(cm, sharejsDoc.getSnapshot()) sharejsDoc.attachToCM(cm) From 2be37795bd3fbbc2537027e3862dc200fbe87c30 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Tue, 20 Feb 2018 15:26:45 +0000 Subject: [PATCH 5/7] TODO for cleaning up sharejs callbacks --- .../public/coffee/ide/editor/sharejs/vendor/client/ace.coffee | 1 + 1 file changed, 1 insertion(+) diff --git a/services/web/public/coffee/ide/editor/sharejs/vendor/client/ace.coffee b/services/web/public/coffee/ide/editor/sharejs/vendor/client/ace.coffee index 462aa456f0..997116fc02 100644 --- a/services/web/public/coffee/ide/editor/sharejs/vendor/client/ace.coffee +++ b/services/web/public/coffee/ide/editor/sharejs/vendor/client/ace.coffee @@ -145,6 +145,7 @@ window.sharejs.extendDoc 'attach_ace', (editor, keepEditorContents, maxDocLength check() doc.detach_ace = -> + # TODO: can we remove the insert and delete event callbacks? editorDoc.removeListener 'change', editorListener delete doc.detach_ace From 791c126df6850150c42313a880001ffe6a64ea09 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Thu, 22 Feb 2018 12:09:51 +0000 Subject: [PATCH 6/7] Fix test to match implementation --- .../coffee/ide/editor/directives/cmEditorTests.coffee | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/services/web/test/unit_frontend/coffee/ide/editor/directives/cmEditorTests.coffee b/services/web/test/unit_frontend/coffee/ide/editor/directives/cmEditorTests.coffee index 3f74268ad9..ada42238af 100644 --- a/services/web/test/unit_frontend/coffee/ide/editor/directives/cmEditorTests.coffee +++ b/services/web/test/unit_frontend/coffee/ide/editor/directives/cmEditorTests.coffee @@ -4,9 +4,11 @@ define ['ide/editor/directives/cmEditor'], () -> beforeEach () -> @richTextInit = sinon.stub() + @richTextOpenDoc = sinon.stub() window.Frontend = { richText: { - init: @richTextInit + init: @richTextInit, + openDoc: @richTextOpenDoc } } @@ -17,8 +19,6 @@ define ['ide/editor/directives/cmEditor'], () -> it 'attaches to CM', () -> inject ($compile, $rootScope) -> - setValue = sinon.stub() - @richTextInit.returns({ setValue: setValue }) getSnapshot = sinon.stub() detachFromCM = sinon.stub() attachToCM = sinon.stub() @@ -30,10 +30,11 @@ define ['ide/editor/directives/cmEditor'], () -> $compile('
')($rootScope) $rootScope.$digest() + $rootScope.$digest() - expect(getSnapshot).to.have.been.called - expect(setValue).to.have.been.called expect(detachFromCM).to.have.been.called + expect(getSnapshot).to.have.been.called + expect(@richTextOpenDoc).to.have.been.called expect(attachToCM).to.have.been.called it 'detaches from CM when destroyed', () -> From 29410b7aabc7b54ceeed16348f2d13a410f8c841 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Thu, 22 Feb 2018 13:28:16 +0000 Subject: [PATCH 7/7] Fix applyAsync from not evaluating expression in tests See https://github.com/angular/angular.js/issues/10788#issuecomment-70376834 which explains that applyAsync is scheduled to evaluate in the next tick, but this is managed by $browser. Therefore we can manually flush the trigger --- .../coffee/ide/editor/directives/cmEditorTests.coffee | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/services/web/test/unit_frontend/coffee/ide/editor/directives/cmEditorTests.coffee b/services/web/test/unit_frontend/coffee/ide/editor/directives/cmEditorTests.coffee index ada42238af..1200d6105c 100644 --- a/services/web/test/unit_frontend/coffee/ide/editor/directives/cmEditorTests.coffee +++ b/services/web/test/unit_frontend/coffee/ide/editor/directives/cmEditorTests.coffee @@ -18,7 +18,7 @@ define ['ide/editor/directives/cmEditor'], () -> expect(@richTextInit).to.have.been.called it 'attaches to CM', () -> - inject ($compile, $rootScope) -> + inject ($compile, $rootScope, $browser) -> getSnapshot = sinon.stub() detachFromCM = sinon.stub() attachToCM = sinon.stub() @@ -30,7 +30,9 @@ define ['ide/editor/directives/cmEditor'], () -> $compile('
')($rootScope) $rootScope.$digest() - $rootScope.$digest() + # Trigger $applyAsync to evaluate the expression, normally done in the + # next tick + $browser.defer.flush() expect(detachFromCM).to.have.been.called expect(getSnapshot).to.have.been.called