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

Update monaco-editor-core to 1.96.3 #14737

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

tsmaeder
Copy link
Contributor

What it does

Fixes #14726

Contributed on behalf of STMicroelectronics

How to test

This needs a general test of functionality in particular of all editor functionality (including in secondary windows) and all quick-input related functions.

Follow-ups

Breaking changes

  • This PR introduces breaking changes and requires careful review. If yes, the breaking changes section in the changelog has been updated.

Attribution

Review checklist

Reminder for reviewers

Fixes eclipse-theia#14726

Contributed on behalf of STMicroelectronics

Signed-off-by: Thomas Mäder <[email protected]>
@tsmaeder tsmaeder requested review from msujew and sdirix January 17, 2025 14:25
@tsmaeder tsmaeder marked this pull request as draft January 17, 2025 14:26
@tsmaeder
Copy link
Contributor Author

Converting to "draft" status while waiting for https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/18700

@tsmaeder
Copy link
Contributor Author

@tsmaeder tsmaeder marked this pull request as ready for review January 27, 2025 08:53
@tsmaeder
Copy link
Contributor Author

tsmaeder commented Jan 27, 2025

@msujew @sdirix would be great to have this one in the community release candidate.

@sdirix
Copy link
Member

sdirix commented Jan 28, 2025

@tsmaeder I can take a look today in the evening

Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

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

I performed the following tests:

  • QuickPick
    • Open
    • Select
    • Escape
  • Text Editor
    • Open without extensions (i.e. no syntax highlighting etc.)
    • With extensions
    • Extract to second window
    • Switching theme
    • Debugging
  • Text Editor Typescript
    • Syntax Highlighting
    • Error/Warning markers
    • Refactor Actions
    • Hover
    • Autocomplete
    • Inline suggestions (AI)
    • Drag&Drop of code
  • Preferences Editor
    • Editing preferences
    • JSON view
    • Both at same time
  • Output channel
  • Diff editor
    • Open a diff
    • Accepting/Reverting a change
  • Debug
    • Setting, Editing and Hitting break points

I tested both in Browser and Electron

The only behavior finding which I found when testing the apps:

Revert popup

In the diff editor, the reverting a change popup looks broken:
Screenshot from 2025-01-28 16-36-24

In the current master it looks fine:

image

Copy link
Member

Choose a reason for hiding this comment

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

These changes should be reverted, right?

Comment on lines 6 to 11
const webpack = require('webpack');
const configs = require('./gen-webpack.config.js');
const nodeConfig = require('./gen-webpack.node.config.js');

const { MonacoWebpackPlugin } = require('@theia/native-webpack-plugin/lib/monaco-webpack-plugins.js');

Copy link
Member

Choose a reason for hiding this comment

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

Should be reverted?

Comment on lines +45 to +48
const iconClasses = this.labelProvider.getIcon(languageUri).split(' ');
if (iconClasses.length > 0) {
iconClasses.push('file-icon');
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this behavior change on purpose? An empty string will now produce file-icon while before it did not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that is correct: an empty string will split to an emtpy array, which has length 0, righ?

Copy link
Member

Choose a reason for hiding this comment

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

The split() method of String values takes a pattern and divides this string into an ordered list of substrings by searching for the pattern, puts these substrings into an array, and returns the array. MDN

The empty string "" therefore only returns a single substring entry, which is the empty string again [""].
That's the same logic as splitting "a", which will also result in ["a"]

For good measure:
image

Comment on lines +321 to +325
const icon = this.labelProvider.getIcon(uri).split(' ');
if (icon.length > 0) {
icon.push('file-icon');
}
return icon;
Copy link
Member

Choose a reason for hiding this comment

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

Is this behavior change on purpose? An empty string will now produce file-icon while before it did not.

Copy link
Member

Choose a reason for hiding this comment

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

Should be reverted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Waiting on reviewers
Development

Successfully merging this pull request may close these issues.

Update monaco-editor-core to 1.96.3
2 participants