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.

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

Integrate monaco editor #1441

Merged
merged 58 commits into from
Jan 21, 2024
Merged

Integrate monaco editor #1441

merged 58 commits into from
Jan 21, 2024

Conversation

coolbreeze413
Copy link
Collaborator

@coolbreeze413 coolbreeze413 commented Jan 8, 2024

Motivate of the pull request

  • To address an existing issue. If so, please provide a link to the issue:
  • Breaking new feature. If so, please describe details in the description part.

Describe the technical details

The Editor instance now uses the Monaco Editor (https://github.com/microsoft/monaco-editor) embedded into a HTML page, and loaded using QWebEngineView, in place of the QScintilla based editor.

What does this pull request change?

  1. Integrate monaco-editor npm package download into the build process
  2. Add HTML page to embed monaco-editor instance
  3. MonacoEditor (monaco-editor based) class replaces Editor class (QScintilla based)
    This class implements QWebEngineView for the Editor instance, and uses QWebChannel to marshall signals and data between Qt C++ and monaco-editor JS code in the embedded HTML.
  4. Add MONACO_EDITOR=1 as default for the Makefile, which controls switch between Monaco Editor based build vs the current QScintilla based build: make MONACO_EDITOR=1
  5. Workflows updated to include QWebEngineWidgets and QWebChannel dependencies required for Monaco Editor
  6. QWebEngineWidgets is not supported for MSYS2 MinGW64(Chromium build not supported), and hence Monaco Editor cannot be used here, and the CI is updated to use MONACO_EDITOR=0 for this platform build
  7. Fixed a few issues with 'reload file when file changes externally' in TextEditorForm class (also applicable to QScintilla based editor build)

Which part of the code base require a change

  • Library: TextEditor
  • Plug-in:
  • Engine: Changes to optimize 'flashing' issue on displaying the monaco editor QWebEngineView first time
  • Documentation
  • Regression tests
  • Continous Integration (CI) scripts

Impact of the pull request

  • Require a change on Quality of Results (QoR)
  • Break back-compatibility. If so, please list who may be influenced.
    Use MONACO_EDITOR=0 with make to fallback to current QScintilla based editor, any build on MSYS2 MinGW64 for Windows will not work with monaco editor due to platform limitations

Known Issues/Limitations

  1. Monaco Editor will not work on MSYS2 MinGW64 Windows build
  2. Delay seen when opening editor instance first time
    Optimize the delay in waiting for the JS side of Monaco Editor to be initialized before returning from this ctor (seen on first editor instance open)
  3. Delay seen in monaco editor resizing
    Optimize the delay seen in Automatic Resize of Monaco Editor w.r.t resize of the FOEDAG window which seems to be due to the GPU rendering of the JS code
  4. Mouse Cursor not changing from 'Arrow' to 'Text' cursor sometimes
    On clicking outside the editor, cursor changes to 'Arrow' and on refocus on the editor, cursor changes to 'Text', however on doing this multiple times, sometimes, the 'Arrow' does not change to 'Text' even though editor is focused correctly (we can even select the text with the 'Arrow' cursor)

@coolbreeze413
Copy link
Collaborator Author

Requires installation of Qt6 WebEngine on the system (sudo apt install qt6-webengine-dev)

@ravic-rs
Copy link
Contributor

ravic-rs commented Jan 8, 2024

Requires installation of Qt6 WebEngine on the system (sudo apt install qt6-webengine-dev)

Have you tried this for windows mingw compiler and under Ubuntu 2022 ? We seem to be having issues with it as @nadeemyaseen-rs and @KochynVolodymyr found out.

ravic-rs and others added 2 commits January 9, 2024 07:28
* Update install_ubuntu_dependencies_build.sh to install qt6-webengine-dev

* Update main.yml to get webengine widgets as well

* Update main.yml

* Update install_ubuntu_dependencies_build.sh

* Update main.yml

* Update main.yml to pull qt with webengine on centos 7

* Update install_centos_dependencies_build.sh

* Incremented patch version

* qwebengine issues (#1444)

---------

Co-authored-by: nadeemyaseen-rs <nadeemyaseen-rs@users.noreply.github.com>
Co-authored-by: KochynVolodymyr <95262932+KochynVolodymyr@users.noreply.github.com>
Co-authored-by: ravic-rs <108307503+ravic-rs@users.noreply.github.com>
Copy link

codecov bot commented Jan 10, 2024

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (4e3ebea) 12.79% compared to head (1a705ff) 12.78%.

Files Patch % Lines
src/TextEditor/text_editor_form.cpp 0.00% 11 Missing ⚠️
src/MainWindow/main_window.cpp 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1441      +/-   ##
==========================================
- Coverage   12.79%   12.78%   -0.01%     
==========================================
  Files         278      278              
  Lines       31390    31398       +8     
  Branches    17930    17932       +2     
==========================================
  Hits         4015     4015              
- Misses      26519    26527       +8     
  Partials      856      856              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@coolbreeze413 coolbreeze413 marked this pull request as ready for review January 17, 2024 15:01
@KochynVolodymyr
Copy link
Contributor

Known issues:

  1. Click on links doesn't open url in default browser
  2. Mouse cursor doesn't change from arrow to text cursor in some cases.

@ravikiranchollangi
Copy link
Contributor

@coolbreeze413 a conflict.

@coolbreeze413
Copy link
Collaborator Author

@ravikiranchollangi @KochynVolodymyr
I will fix the conflicts and push shortly.
Meanwhile, I have updated the description and details of the PR, please review.

Copy link
Contributor

@KochynVolodymyr KochynVolodymyr 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 to me

@ravic-rs
Copy link
Contributor

@alain-rs keeping this on hold until @nadeemyaseen-rs adds qtwebengine in foedag_rs and raptor CI.

@alaindargelas alaindargelas requested review from alaindargelas and ravikiranchollangi and removed request for ravic-rs and w0lek January 19, 2024 19:31
@coolbreeze413
Copy link
Collaborator Author

coolbreeze413 commented Jan 21, 2024

@ravikiranchollangi @KochynVolodymyr @alaindargelas
I have pushed fix for 'open links in editor' with default desktop opener as well.

@ravikiranchollangi
Copy link
Contributor

@ravikiranchollangi @KochynVolodymyr @alaindargelas I have pushed fix for 'open links in editor' with default desktop opener as well.

Thank you!

@ravikiranchollangi ravikiranchollangi merged commit e28b90c into main Jan 21, 2024
33 of 35 checks passed
@ravikiranchollangi ravikiranchollangi deleted the monaco-editor-integration branch January 21, 2024 19:25
@alain-rs
Copy link
Contributor

alain-rs commented Jan 21, 2024 via email

@KochynVolodymyr KochynVolodymyr restored the monaco-editor-integration branch January 29, 2024 13:56
@KochynVolodymyr KochynVolodymyr deleted the monaco-editor-integration branch January 29, 2024 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants