Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Replace ace with monaco editor (Closes #571) #1800

Merged
merged 18 commits into from
Sep 17, 2020
Merged

Replace ace with monaco editor (Closes #571) #1800

merged 18 commits into from
Sep 17, 2020

Conversation

umesh-timalsina
Copy link
Contributor

@umesh-timalsina umesh-timalsina commented Jul 22, 2020 •
edited
Loading

Checklist:

  • Apply Basic Changes to panels

    • LogViewer
    • OperationCodeEditor
    • OperationDepEditor
    • TabbedTextEditor
    • SerializeEditor

  • Add Support for different themes, via contextmenu

  • Add support for keybindings (Currently, Only VIM is supported, Emacs can be added on a later PR/issue)

brollb
brollb reviewed Jul 23, 2020
View reviewed changes
src/visualizers/widgets/TextEditor/TextEditorWidget.js Outdated Show resolved Hide resolved
src/visualizers/widgets/TextEditor/TextEditorWidget.js Outdated
this.logger = logger.fork('Widget');
MonacoLanguages = JSON.parse(MonacoLanguages);
const DEFAULT_THEMES = ['vs-dark', 'vs', 'hc-black'];
const DEFAULT_COLORS = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this can be removed.

Copy link
Contributor Author

@umesh-timalsina umesh-timalsina Jul 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to the lack of submenu support and already exsiting context menu. My plan was to add a separate button. This which opens the existing contextmenu. These colors were used in #1746 for the button in the context menu.

src/visualizers/widgets/TextEditor/TextEditorWidget.js Outdated Show resolved Hide resolved
src/visualizers/widgets/TextEditor/TextEditorWidget.js Outdated
TextEditorWidget = function (logger, container, config={}) {
this.logger = logger.fork('Widget');
MonacoLanguages = JSON.parse(MonacoLanguages);
const DEFAULT_THEMES = ['vs-dark', 'vs', 'hc-black'];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be nice to have a couple more themes available. Maybe 5 or 6?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/brijeshb42/monaco-themes. Similar to #1800 (comment). Which would be the best course?

@umesh-timalsina
Copy link
Contributor Author

It is to be noted that the current LogViewerWidget doesn't print ANSI colors correctly.

Python terminal(Bash Shell)
image

Output in deepforge:
image

image

@brollb
Copy link
Contributor

brollb commented Aug 12, 2020

Can you share the raw text before it is rendered?

@umesh-timalsina
Copy link
Contributor Author

The raw text is the following:

\033[31;1;4mHello\033[0m

@brollb
Copy link
Contributor

brollb commented Aug 13, 2020

Ah, I see. The first one is using underlines and other SGR parameters; it currently only supports colors and expected there to be only two options. This should be pretty easy fix but could be fixed in it's own PR.

@umesh-timalsina umesh-timalsina mentioned this pull request Aug 17, 2020
Open
@brollb brollb added this to the v2.5.0 milestone Aug 18, 2020
@umesh-timalsina umesh-timalsina marked this pull request as ready for review September 9, 2020 21:23
@umesh-timalsina
Copy link
Contributor Author

umesh-timalsina commented Sep 9, 2020 •
edited
Loading

I am marking this active. There is one issue left to sortout: Somehow, 2 editor workers are being assingned when viewing stdout in LogViewerWidget.

Also, following issues will stem and should be addressed after this is merged:

  1. Integrate python language server and code completion support. Sources and providers are already available at https://github.com/umesh-timalsina/monaco-webgme-viz
  2. Integrate Emacs?
  3. Add a ANSI text rendering via Monarch or any other decorators? I am not sure if it is even possible via Monarch.
  4. Provide diffing for collaborative editing via diff editors? (Low Priority)

@brollb
Copy link
Contributor

brollb commented Sep 16, 2020

I am still seeing the following warning :/
DeepinScreenshot_select-area_20200916155240

brollb
brollb requested changes Sep 16, 2020
View reviewed changes
Copy link
Contributor

@brollb brollb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! If you can remove the console.log, it should be good to go!

src/visualizers/widgets/TextEditor/TextEditorWidget.js Outdated Show resolved Hide resolved
@umesh-timalsina
Copy link
Contributor Author

This PR only is part one of the two needed to close #571. This Introduces Monaco Text Editor, which has support for a language server protocol.

@umesh-timalsina umesh-timalsina mentioned this pull request Sep 16, 2020
Open
@umesh-timalsina umesh-timalsina changed the title Monaco Text Editor Replace ace with monaco editor (Closes #571) Sep 16, 2020
@brollb brollb merged commit a700a9e into master Sep 17, 2020
@brollb brollb deleted the 571-monaco-text-editor branch September 17, 2020 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brollb brollb brollb requested changes

Assignees
No one assigned
Labels
enhancement major
Projects
None yet
Milestone
v2.5.0
Development

Successfully merging this pull request may close these issues.
2 participants
@umesh-timalsina @brollb