-
Notifications
You must be signed in to change notification settings - Fork 7
Skia support, multi-editors, monaco updates #28
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
base: uno
Are you sure you want to change the base?
Conversation
Updated Uno.WinUI package version in Directory.Packages.props to 6.4.0-dev.212. Enhanced MonacoEditorComponent.csproj to support net10.0 framework alongside net9.0, including updates to target frameworks, conditional property groups, and item groups. Modified App.xaml.cs to navigate to MainPage by default. Added BlazorWebAssemblyJiterpreter property in MonacoEditorTestApp.csproj and generalized file inclusion pattern for browserwasm.
Updated the `Uno.WinUI` package to `6.4.0-dev.235` and the `Uno.Sdk` versions in `global.json` to `6.4.0-dev.50` and `6.4.0-dev.256`. Changed the project SDK in `MonacoEditorTestApp.csproj` from `Uno.Sdk.Private` to `Uno.Sdk` and upgraded the target framework to `net10.0-browserwasm`. Commented out unused embedded resources in `MonacoEditorComponent.csproj` and added the `<WasmFingerprintAssets>` property with a value of `false` to optimize asset handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request updates the Monaco Editor dependency to a newer version and modernizes the project's build configuration. The changes introduce centralized package management, enable nullable reference types, and update build tooling to improve maintainability and type safety across the codebase.
Key changes:
- Centralized build configuration with Directory.Build.props/targets and Directory.Packages.props
- Monaco Editor and related dependencies upgraded to newer versions
- Solution filter added to streamline project loading
- Event handling refactored to support nullable reference types
Reviewed Changes
Copilot reviewed 188 out of 306 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| Directory.Build.props | Adds central project properties including nullable reference types and warning configurations |
| Directory.Build.targets | Adds shared build targets for artifact publishing |
| Directory.Packages.props | Introduces central package version management |
| MonacoEditorComponent.slnf | Adds solution filter for selective project loading |
| GenerateMonacoTypings/package.json | Updates monaco-editor, typescript, and typedoc dependencies |
| .vsts-ci.yml | Updates CI to use solution filter and adds binary logging |
| CodeEditor.Events.cs | Refactors events to use nullable event handlers and improves initialization logic |
| monaco-editor/min/vs/* | Updates Monaco Editor library files to newer version |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Modernized versioning by replacing `gitversion.yml` with `version.json` using the `Nerdbank.GitVersioning` schema. Added a global package reference for `Nerdbank.GitVersioning` (v3.8.118) in `Directory.Packages.props`. Updated `MonacoEditorComponent.csproj`: - Changed `Title` to "Monaco Editor Component WASM". - Enhanced NuGet metadata with `Authors`, `Copyright`, `PackageLicenseExpression`, and `PackageTags`. - Simplified packaging by removing `NuspecProperties` and `NuspecFile`. - Excluded unnecessary files from the NuGet package using `Pack="false"`. Improved project structure and clarified the scope of the Monaco Editor component.
- Added `<ContinuousIntegrationBuild>` property for CI builds. - Updated copyright year to dynamically use the current year. - Adjusted content packaging rules to include necessary files. - Introduced framework-specific content packaging logic. - Improved handling of primary target framework extraction. - Allowed `.pdb` files in package build output. - Removed deprecated `<GenerateLibraryLayout>` property. - General cleanup of content inclusion and resource declarations.
…n sources. Switch CI to Ubuntu, update build props, and nuget config Updated the CI pipeline to use `ubuntu-latest` instead of `windows-latest` for the `Windows` job. Added the `<UseArtifactsOutput>` property in `Directory.Build.props` to enable artifact output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 208 out of 425 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 208 out of 425 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| root = true | ||
|
|
||
| [*] | ||
| indent_style = space |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spaces... it hurts me :-) But I understand it's a fork...
| <PackageVersion Include="Uno.Core.Extensions.Logging" Version="4.1.1" /> | ||
| <PackageVersion Include="Uno.Core.Extensions.Logging.Singleton" Version="4.1.1" /> | ||
| <PackageVersion Include="Microsoft.TypeScript.MSBuild" Version="5.9.3" /> | ||
| <GlobalPackageReference Include="Nerdbank.GitVersioning" Version="3.8.118"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't know we can do that. That's nice.
| <Configurations> | ||
| <Platform Name="Any CPU" /> | ||
| <Platform Name="ARM" /> | ||
| <Platform Name="ARM64" /> | ||
| <Platform Name="x64" /> | ||
| <Platform Name="x86" /> | ||
| </Configurations> | ||
| <Folder Name="/Solution Items/"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure Any CPU is enough.
| <Configurations> | |
| <Platform Name="Any CPU" /> | |
| <Platform Name="ARM" /> | |
| <Platform Name="ARM64" /> | |
| <Platform Name="x64" /> | |
| <Platform Name="x86" /> | |
| </Configurations> | |
| <Folder Name="/Solution Items/"> | |
| <Folder Name="/Solution Items/"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this file still relevant?
global.json
Outdated
| "Uno.Sdk": "6.4.0-dev.50", | ||
| "Uno.Sdk.Private": "6.4.0-dev.256" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually we don't use both in a same solution or it would cause strange problems with the DevServer.
I suggestion keeping only Uno.Sdk.Private here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: Uno.Sdk 6.4.0-dev.50 uses Uno version 6.4.0-dev.235, not 6.4.0-dev.256
| <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets> | ||
| </PackageReference> | ||
|
|
||
| <PackageReference Include="Newtonsoft.Json" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to ditch this dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No namespace here ??
| }); | ||
|
|
||
| // Set theme | ||
| console.debug("Getting parent theme value"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debugging code...
Updated `.vsts-ci.yml` to adjust artifact publishing paths: - Changed publish path to `package/release` for better structure. - Removed redundant log publishing path. Modified `Directory.Build.targets` to include a `SourceRoot` item: - Dynamically includes `$(ArtifactsPath)` if the variable is set. - Supports improved build and packaging workflows.
Updated the `<SourceRoot>` element in `Directory.Build.targets` to append a trailing slash (`/`) to the `$(ArtifactsPath)`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 208 out of 425 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
carldebilly
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 209 out of 426 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
MonacoEditorComponent/monaco-editor/min/vs/editor/editor.main.nls.de.js:1
- This entire file has been deleted which appears to be a German localization file. Ensure that German language support is intentionally being removed and that this won't break existing functionality for German-speaking users.
MonacoEditorComponent/monaco-editor/min/vs/css-CaeNmE3S.js:1 - The entire file content structure has changed from a multi-line AMD module definition to a single-line minified format. The version reference changed from 0.34.0 to being removed entirely. Verify that this represents a legitimate Monaco Editor upgrade and that the new format is compatible with the application's module loading system.
define("vs/css-CaeNmE3S",["exports"],(function(e){"use strict";const t={wordPattern:/(#?-?\d*\.\d\w*%?)|((::|[@#.!:])?[\w-?]+%?)|::|[@#.!:]/g,comments:{blockComment:["/*","*/"]},brackets:[["{","}"],["[","]"],["(",")"]],autoClosingPairs:[{open:"{",close:"}",notIn:["string","comment"]},{open:"[",close:"]",notIn:["string","comment"]},{open:"(",close:")",notIn:["string","comment"]},{open:'"',close:'"',notIn:["string","comment"]},{open:"'",close:"'",notIn:["string","comment"]}],surroundingPairs:[{open:"{",close:"}"},{open:"[",close:"]"},{open:"(",close:")"},{open:'"',close:'"'},{open:"'",close:"'"}],folding:{markers:{start:new RegExp("^\\s*\\/\\*\\s*#region\\b\\s*(.*?)\\s*\\*\\/"),end:new RegExp("^\\s*\\/\\*\\s*#endregion\\b.*\\*\\/")}}},n={defaultToken:"",tokenPostfix:".css",ws:`[
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…n timing and syntax highlighting (#29)
|
|
Introduced a `FileExtension` property in the `CodeEditor` class to automatically determine the code language based on file extensions. This is backed by a new `DependencyProperty` and integrates with a JavaScript function `languageIdFromFileName` for language mapping using Monaco Editor. - Added `LanguageIdFromFileName` method in `NativeMethods` for JS interop. - Implemented `languageIdFromFileName` in `asyncCallbackHelpers.ts`. - Updated `globalThis` to include the new function. - Refactored `using` directives for consistency and removed redundancies. - Added XML documentation for the `FileExtension` property. - Improved maintainability and readability across related files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 209 out of 426 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Added the `AllowForWeb` attribute to enable JavaScript access to the `DebugLogger` class in UWP apps. Suppressed the CA1822 warning for the `Log` method to avoid marking it as static, ensuring compatibility with the existing design. Reformatted the `#if DEBUG` block for improved readability and consistency.
Replaced `FileExtension` property in `CodeEditor` with a new approach that determines code language based on file extensions. Removed the `LanguageIdFromFileName` method and introduced `LanguageIdFromExtension` for improved efficiency and clarity. - Removed `FileExtension` property and its `DependencyProperty` logic. - Updated `globalThis.languageIdFromFileName` to `languageIdFromExtension` in JavaScript. - Modified `LanguagesHelper` to include `GetCodeLanguageFromExtension` method. - Changed `LanguagesHelper` class to `sealed partial` for extensibility. - Adjusted imports and removed unused namespaces for cleanup. - Updated comments and attributes to reflect the new design. These changes simplify the codebase, reduce redundancy, and align with best practices for language detection.
The `JSImport` attribute for the `LanguageIdFromExtension` method in the `NativeMethods` class was updated to reference the `languageIdFromExtension` function from `globalThis` instead of `languageIdFromFileName`. This change reflects a shift in functionality to determine the language ID based on file extensions rather than file names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 211 out of 428 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This pull request introduces several significant improvements and updates across the codebase, focusing on modernizing project configuration, improving nullability and event handling, updating dependencies and build tooling, and enhancing the code editor's initialization logic. The changes aim to make the project more robust, maintainable, and compatible with newer tooling.
Project structure and configuration modernization:
Directory.Build.props,Directory.Build.targets, andDirectory.Packages.propsfiles to enable central management of build properties and package versions, including enabling implicit usings, nullable reference types, treating warnings as errors, and suppressing specific warnings (NU1507,NETSDK1201,PRI257,CS1998). [1] [2] [3]MonacoEditorComponent.sln) and introduced a solution filter (MonacoEditorComponent.slnf) to improve project organization and streamline loading in Visual Studio. [1] [2] [3]Dependency and tooling updates:
monaco-editor,typedoc, andtypescriptdependencies to more recent versions inGenerateMonacoTypings/package.json. Also updated the TypedocConverter tool to v2.6 and adjusted the PowerShell script to use new options for type conversion. [1] [2] [3]Build process enhancements:
.vsts-ci.yml) to use the new solution filter file, added binary logging to MSBuild, and ensured artifacts are always published regardless of build success. [1] [2]Code editor improvements and nullability:
CodeEditor.Events.csto use nullable event handlers and renamed events for clarity (e.g.,EditorLoading,EditorLoaded). Improved null safety throughout the file, updated initialization logic, and enhanced focus management and theme handling to be more robust and compatible with nullable reference types. [1] [2] [3] [4] [5]CommandHandlerdelegate inCodeEditor.Methods.csto accept nullable parameters, aligning with the project's move to nullable reference types.These changes collectively modernize the project, improve safety and maintainability, and ensure compatibility with current .NET and JavaScript tooling.