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

[RegistryPreview] Adopt Monaco Editor #35122

Merged
merged 8 commits into from
Oct 24, 2024

Conversation

davidegiacometti
Copy link
Collaborator

@davidegiacometti davidegiacometti commented Sep 29, 2024

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

  • Moved Monaco source code in a common location
  • Replaced the TextBox with a WebView2 control that loads Monaco Editor
  • Throttle UI refresh to at least 250ms when user type in the editor
  • Fixed a crash when multiple ContentDialog are opened
    Easy reproducible on stable opening a new Registry Preview, typing fast on the TextBox and press Enter quickly

TODO

  • Decouple from FilePreviewCommon project
  • Share Monaco Editor source code between Peek, RegistryPreview and PreviewHandler projects
  • Simulate an upgrade of PowerToys.RegistryPreviewUILib NuGet package in DevHome and validate that Monaco is working as expected
  • Investigate high contrast theme - vs and vs-dark theme are handled in HC mode

image

image

Validation Steps Performed

  • Performed a clean build of the user installer and validated that Monaco is still working in Peek and PreviewHandler
  • Performed a clean build of the machine installer and validated that Monaco is still working in Peek and PreviewHandler
  • Tested RegistryPreview
    • Explorer > .reg file > Open with > Registry Preview
    • Open File
    • Reload
    • Save
    • Save as
    • Write to Registry
    • Monaco Copy/Paste
    • Load test: verified that the Editor is still responsive with a 5K lines file

@davidegiacometti davidegiacometti marked this pull request as draft September 29, 2024 11:42
@htcfreek
Copy link
Collaborator

Does an on off switch (settings or RegistryPreviewUi) makes sense? 🤔

@davidegiacometti
Copy link
Collaborator Author

@htcfreek I think we can abstract the editor under an IEditor interface with two implementation: Monaco Editor Control and TextBox, then add an option to switch between the two modes.
Wondering what's the use case to stick with the TextBox 🤔

@htcfreek
Copy link
Collaborator

htcfreek commented Oct 3, 2024

@htcfreek I think we can abstract the editor under an IEditor interface with two implementation: Monaco Editor Control and TextBox, then add an option to switch between the two modes.
Wondering what's the use case to stick with the TextBox 🤔

Only thought that not everybody like the colored view. And maybe with complex file it is easier to have no color than to have a rainbow.

@davidegiacometti davidegiacometti force-pushed the users/davidegiacometti/registry-prev-monaco branch from eb6a10e to 3d2f944 Compare October 5, 2024 09:41

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@davidegiacometti davidegiacometti force-pushed the users/davidegiacometti/registry-prev-monaco branch from 94ad8f1 to 7fddb81 Compare October 5, 2024 12:25
@davidegiacometti davidegiacometti marked this pull request as ready for review October 7, 2024 20:37
@davidegiacometti davidegiacometti changed the title 🚧 [RegistryPreview] Adopt Monaco Editor [RegistryPreview] Adopt Monaco Editor Oct 7, 2024
@davidegiacometti
Copy link
Collaborator Author

This is ready for review! 😃

  • I wasn't able to simulate an upgrade of the NuGet package in DevHome due to the WASDK mismatch but I will do it once DevHome will be on 1.6 too.
    If we are lucky, adding the new required assets in the package should be enough 🤞

image

  • Let me know if we want to add an option to turn on/off Monaco.

@crutkas crutkas added the Needs-Review This Pull Request awaits the review of a maintainer. label Oct 8, 2024
@stefansjfw
Copy link
Collaborator

image

Can we disable command pallete? Not sure if it can cause any harm, but I'd argue it's too much to have for just a text input

@davidegiacometti
Copy link
Collaborator Author

davidegiacometti commented Oct 22, 2024

Hi @stefansjfw

Unfortunately there is no API to disable the command palette but I managed to achieve that with a few lines of JavaScript 😅

// Remove the command palette item from the context menu: https://github.com/microsoft/monaco-editor/issues/1280#issuecomment-2099873176
const contextmenu = editor.getContribution('editor.contrib.contextmenu');
const realMethod = contextmenu._getMenuActions;
contextmenu._getMenuActions = function () {
    const items = realMethod.apply(contextmenu, arguments);
    return items.filter(function (item) {
        return item.id != 'editor.action.quickCommand';
    });
};

// Prevent pressing F1 from opening the command palette
editor.addCommand(monaco.KeyCode.F1, function () { });

It's working fine and I am not against doing that, just keep in mind that some actions from the command palette are still accessible using the relative shortcut.
Also not a big deal but I think this solution will have to be tested after upgrading Monaco.

Would like to know your thoughts about the solution before pushing the change.

Thanks for the review! 😃

@htcfreek
Copy link
Collaborator

Wondering if disabling command palette is really needed. I mean, it does annoys the users.

Copy link
Collaborator

@stefansjfw stefansjfw 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! Nice work!

Comment on lines +74 to +75
Browser.CoreWebView2.Settings.IsScriptEnabled = true;
Browser.CoreWebView2.Settings.IsWebMessageEnabled = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

are these two needed? can we turn them off?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Double checked this before merge but we need them
Scripts of course are needed to run Monaco 😄
Web messages are used for the communication between Monaco and Registry Preview

private void CoreWebView2_WebMessageReceived(CoreWebView2 sender, CoreWebView2WebMessageReceivedEventArgs args)
{
var json = JsonNode.Parse(args.WebMessageAsJson);

Copy link
Collaborator

Choose a reason for hiding this comment

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

got it. thank you. feel free to merge whenever :)

@stefansjfw
Copy link
Collaborator

Wondering if disabling command palette is really needed. I mean, it does annoys the users.

let's leave it. It can be useful for some more power users that use RegPrev for editing and writing reg files

@davidegiacometti davidegiacometti merged commit f9127b6 into main Oct 24, 2024
21 checks passed
@davidegiacometti davidegiacometti deleted the users/davidegiacometti/registry-prev-monaco branch October 26, 2024 14:50
stefansjfw added a commit that referenced this pull request Oct 31, 2024
PesBandi added a commit to PesBandi/PowerToys that referenced this pull request Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Review This Pull Request awaits the review of a maintainer.
Projects
Development

Successfully merging this pull request may close these issues.

5 participants