Monaco-editor support #299

pranjaltale16 · 2018-05-24T05:22:23Z

Add support for monaco-editor.
I'm not sure, how do I've to update tests.
Also, I'm not sure about the detach thing. I tried to look for it, but unable to find anything for that.
Could someone help me with these 2 things?

…aco-adapter.js

coveralls · 2018-05-24T05:26:27Z

Coverage decreased (-0.2%) to 72.543% when pulling 9b4880e on pranjaltale16:monaco into b678875 on firebase:master.

mikelehen · 2018-05-25T22:30:30Z

Thanks! Would it be possible to add an examples/monaco.html file that demos this working? That'll also be helpful for anybody that tries to use this down the road.

pranjaltale16 · 2018-05-26T06:25:07Z

Done. I followed this to integrate the editor.
Since it is loaded with AMD modules so I've to keep the scripts locally.

pranjaltale16 · 2018-05-26T06:29:34Z

The other alternative could be to have monaco-editor in package.json. But in this case, one has to install node modules first. Otherwise, it won't work.

pranjaltale16 · 2018-05-30T19:42:06Z

Hey! What would be the better way to have the editor in examples?

mikelehen · 2018-05-30T20:47:37Z

Could you use this CDN version? https://cdnjs.com/libraries/monaco-editor

Else, having people install node modules first is probably fine.

pranjaltale16 · 2018-06-01T08:59:38Z

Hey! I updated that. Have a look.

pranjaltale16 · 2018-06-04T10:15:02Z

Updated the detach function following this: https://stackoverflow.com/questions/50405510/remove-listner-from-monaco-editor/50413982#50413982

mikelehen

Woo! I was able to test this out and it seems to work quite well. Nice work!

I've left a few comments (though I didn't review the actual adapter too deeply). Please take a look and then we can likely get this merged. Thanks for putting this together!

mikelehen · 2018-06-04T13:58:27Z

package.json

    "jsdom": ">3",
-    "firebase": "^3 || ^4"
+    "monaco-editor": "^0.13.1"


can you make this a devDependency please? It's necessary to run the examples but we don't want to automatically depend on monaco-editor when you install firepad (since you may be using it with ace / codemirror)

mikelehen · 2018-06-04T14:01:12Z

test/index.html

@@ -15,6 +15,9 @@
    <!-- CodeMirror -->
    <script src="../bower_components/codemirror/lib/codemirror.js"></script>

+    <!-- Monaco -->
+    <script src="../bower_components/monaco-editor-official/monaco.d.ts"></script>


Hrm... I don't see monaco-editor-official added to ~/bower.json and when I open this file, there's a 404. But the tests still pass because I don't think they depend on this. So maybe just remove this script include?

mikelehen · 2018-06-04T16:01:09Z

examples/monaco.html

+        var firepadRef = getExampleRef();
+
+        //// Create Monaco and firepad.
+        require.config({ paths: {'vs': './../node_modules/monaco-editor/min/vs'}});


Hrm... You're pulling in loader.js from the CDN but then loading from node_modules here... Ideally we'd use one or the other (and the CDN would be best so people can just copy/paste the example and have it work).

I'm not that sure about the CDN thing, so added loader import from local only.

mikelehen · 2018-06-04T16:02:02Z

examples/monaco.html

+
+    <!-- Firepad -->
+    <link rel="stylesheet" href="https://cdn.firebase.com/libs/firepad/1.4.0/firepad.css" />
+    <script src="https://cdn.firebase.com/libs/firepad/1.4.0/firepad.min.js"></script>


Obviously this doesn't work yet. I can change it to 1.5.0 once it's released... but given that as-written, this depends on local files (node_modules/monaco-editor) you could also just change this to ../dist/firepad.min.js so that the example works as-is.

mikelehen · 2018-06-04T16:03:21Z

lib/firepad.js

-    if (!CodeMirror && !ace) {
-      throw new Error('Couldn\'t find CodeMirror or ACE.  Did you forget to include codemirror.js or ace.js?');
+    if (!CodeMirror && !ace && !global.monaco) {
+      throw new Error('Couldn\'t find CodeMirror, ACE or Monaco.  Did you forget to include codemirror.js/ace.js/monaco.d.ts?');


Hrm. I thought a .d.ts file only has type definitions and so it won't help to include it, right (it's not going to create global variables and whatnot). Can you fix this to be more correct, or just remove it?

I tried to remove this, but it is giving some error.

To be clear, I just meant remove "/monaco.d.ts" from the error message...

mikelehen · 2018-06-04T16:03:39Z

lib/firepad.js

+      this.monaco_ = this.editor_ = place;
+      curValue = this.monaco_.getValue();
+      if (curValue !== '') {
+        throw new Error("Can't initialize Firepad with an Monaco instance that already contains text.");


"an" => "a"

mikelehen · 2018-06-04T16:09:57Z

lib/monaco-adapter.js

+  };
+
+  MonacoAdapter.prototype.detach = function() {
+    this.monaco.onDidChangeModelContent(this.onChange).dispose();


I haven't dug into Monaco's implementation, but I kinda' suspect that this is attaching a new listener and then immediately removing it, and likely isn't removing the listener you attached in the constructor.

Yeah! Even I'm not that sure about this. A better addition would be to have
this.onChange.dispose() this will call the dispose of for the actual attached onChange.

pranjaltale16 · 2018-06-05T03:14:37Z

Hey! I updated the changes. Have a look. :D

mikelehen

Sorry... One more question about the detach() code. :-)

mikelehen · 2018-06-05T22:58:50Z

lib/monaco-adapter.js

+    this.onChange.dispose();
+    this.onBlur.dispose();
+    this.onFocus.dispose();
+    this.onCursorActivity.dispose();


Hrm... Sorry to keep bugging you about this code, but have you tried calling firepad.dispose() to verify this actually runs okay? Unless calling this.monaco.onDidChangeModelContent(this.onChange) actually mutates your onChange function and adds a dispose property, I'd guess that this will not work correctly.

I would guess that what you need to do is something like:

var changeHandler = this.monaco.onDidChangeModelContent(this.onChange); var didBlurHandler = this.monaco.onDidBlurEditor(this.onBlur); ... this.detach = function() { changeHandler.dispose(); blurHandler.dispose(); ... }

No worries, Thank you for helping me out with this. I'm clueless about the detach function.

pranjaltale16 · 2018-06-06T04:31:10Z

Is this okay? Do let me know if I've to make any other change. Actually, I'm not aware of the functioning of detach and how to handle attached listeners from monaco, so that's why this part is little confusing for me.
Again, Thanks for helping me.

mikelehen · 2018-06-06T17:57:51Z

Thanks! It wasn't quite right, but I went ahead and merged it and then submitted a fix. :-)

mikelehen · 2018-06-06T18:07:54Z

I've pushed a new v1.5.0 npm module of firepad with the monaco support. Thanks for tackling this!

pranjaltale16 · 2018-06-06T19:06:17Z

Thank you so much for helping me out with this. :)

* New MonacoAdapter class added and plumbed into lib/firepad.js, etc. * examples/monaco.html added to demonstrate it.

pranjaltale16 added 17 commits May 14, 2018 11:28

Update firepad.js to support monaco-adapter, create base class in mon…

549c627

…aco-adapter.js

Update indexFromPos in monaoc-adapter

d36139e

Connected firepad with monaco

eac71f5

Update indexFromPos function in monaco-adapter

13f60f2

Update setCursor in monaco-adapter

601ef5a

Update monaco-adapter

0030fb6

Update opterationFromMonaco in monaco-adapter

d73cf07

Update monaco-adapter. applyOperationToMonaco working

bb60a2a

add cursor functionality in setOtherCursor

450fedb

Update setOtherCursor to support coloured cursors

6064c67

update applyOperation function

8b886c2

Update monaco-adapter and ace-adapter

96358ca

Update monaco-adapter

ef7c20a

Update firepad

a157c44

Update applyOperation in monaco-editor, still edge case is remaining

e9e25ce

fix edge case issue in applyOperation

912eb02

fix cursor issue in setOtherCursor of monaco-adapter

b1ad38e

pranjaltale16 changed the title ~~Monaco~~ Monaco-editor support May 24, 2018

Add monaco editor in examples

9cba2b8

Update monaco.html in examples

c05ec2a

pranjaltale16 added 2 commits June 1, 2018 16:21

fix editor instance in monaco-adapter applyOperations

5a9b77e

Update detach in monaco-adapter

f5692cb

mikelehen reviewed Jun 4, 2018

View reviewed changes

mikelehen assigned pranjaltale16 Jun 4, 2018

pranjaltale16 added 4 commits June 4, 2018 22:56

Update suggested fix

0765d11

Merge branch 'master' into monaco

805b85b

Update package.json

5e1ffa0

Update package.json

0690042

mikelehen requested changes Jun 5, 2018

View reviewed changes

Update detach function in monaco-adapter

9b4880e

mikelehen merged commit 251df18 into FirebaseExtended:master Jun 6, 2018

mikelehen mentioned this pull request Jul 3, 2018

Add Quill Support #285

Closed

mfont-gh pushed a commit to mfont-gh/firepad that referenced this pull request Jun 3, 2019

Monaco-editor support (FirebaseExtended#299)

a9f1c72

* New MonacoAdapter class added and plumbed into lib/firepad.js, etc. * examples/monaco.html added to demonstrate it.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Monaco-editor support #299

Monaco-editor support #299

pranjaltale16 commented May 24, 2018 •

edited

Loading

coveralls commented May 24, 2018 •

edited

Loading

mikelehen commented May 25, 2018

pranjaltale16 commented May 26, 2018 •

edited

Loading

pranjaltale16 commented May 26, 2018

pranjaltale16 commented May 30, 2018

mikelehen commented May 30, 2018

pranjaltale16 commented Jun 1, 2018

pranjaltale16 commented Jun 4, 2018

mikelehen left a comment

mikelehen Jun 4, 2018

mikelehen Jun 4, 2018

mikelehen Jun 4, 2018

pranjaltale16 Jun 4, 2018

mikelehen Jun 4, 2018

mikelehen Jun 4, 2018

pranjaltale16 Jun 4, 2018

mikelehen Jun 4, 2018

mikelehen Jun 4, 2018

mikelehen Jun 4, 2018

pranjaltale16 Jun 4, 2018

pranjaltale16 commented Jun 5, 2018

mikelehen left a comment

mikelehen Jun 5, 2018

pranjaltale16 Jun 6, 2018

pranjaltale16 commented Jun 6, 2018

mikelehen commented Jun 6, 2018

mikelehen commented Jun 6, 2018

pranjaltale16 commented Jun 6, 2018

Monaco-editor support #299

Monaco-editor support #299

Conversation

pranjaltale16 commented May 24, 2018 • edited Loading

coveralls commented May 24, 2018 • edited Loading

mikelehen commented May 25, 2018

pranjaltale16 commented May 26, 2018 • edited Loading

pranjaltale16 commented May 26, 2018

pranjaltale16 commented May 30, 2018

mikelehen commented May 30, 2018

pranjaltale16 commented Jun 1, 2018

pranjaltale16 commented Jun 4, 2018

mikelehen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranjaltale16 commented Jun 5, 2018

mikelehen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranjaltale16 commented Jun 6, 2018

mikelehen commented Jun 6, 2018

mikelehen commented Jun 6, 2018

pranjaltale16 commented Jun 6, 2018

pranjaltale16 commented May 24, 2018 •

edited

Loading

coveralls commented May 24, 2018 •

edited

Loading

pranjaltale16 commented May 26, 2018 •

edited

Loading