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

fix: view and monaco glyph interactions with debug and diffviedw #571

Merged
merged 2 commits into from
Feb 21, 2024

Conversation

Kevin101Zhang
Copy link
Contributor

@Kevin101Zhang Kevin101Zhang commented Feb 20, 2024

Logic fix for handling monaco glyph mounting.

Originally it will error on diffView/debug mode due to rendering of unmounted glyphs.
Screenshot 2024-02-20 at 3 05 05 PM

Video post fix:
https://www.loom.com/share/dd797612590d4f12a27e7216ab009b68?sid=4ec944fd-e718-4851-b231-4339ffab5bd8

@Kevin101Zhang Kevin101Zhang requested a review from a team as a code owner February 20, 2024 20:05
@Kevin101Zhang Kevin101Zhang linked an issue Feb 20, 2024 that may be closed by this pull request
glyphMarginClassName: "glyphSuccess",
glyphMarginHoverMessage: { value: "" },
if (!diffView) {
const decorations = editor.deltaDecorations([],
Copy link
Collaborator

@darunrs darunrs Feb 20, 2024

Choose a reason for hiding this comment

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

What purpose does this particular delta decoration actually do? Is this a leftover from when you were doing the green box? If so, can we just remove it?

Also, I believe WillMount is only done on init.

I think there's a small bug with context.db where on initial reload, it doesn't set the types. You have to change tabs to get it to load, or click the button. This was because of a race condition where WillMount may be done before the editor is actually available. My guess is that is the cause of the error, not that it's setting decorations that don't exist.

Maybe you can experiment with that later. But its best to do any monaco things when we're sure it's completely initialized.

Copy link
Contributor Author

@Kevin101Zhang Kevin101Zhang Feb 20, 2024

Choose a reason for hiding this comment

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

Its necessary to mount the glyph here.
We do this to grab the decoration that takes a shape ['W;3'] and the monaco editor ref.
Note: the decoration letter and number changes everytime
When we run parseGlyph it is overwriting the one decoration in line

monacoEditorRef.current.deltaDecorations( [decorations], [ { range: new monaco.Range(startLine, startColumn, endLine, endColumn), options: { isWholeLine: true, glyphMarginClassName: error ? "glyphError" : "glyphSuccess", glyphMarginHoverMessage: { value: displayedError }, }, }, ] );

The other option is to create the the decoration for the first time in parseGlyphError. I tried this approach but it makes the code less readable since we are instantiating the glyph here here.

What can be done is the code onMount can be shortened without the key/value pairs in the options.

if (!diffView) {
const decorations = editor.deltaDecorations([],
[
{
range: new monaco.Range(1, 1, 1, 1),
options: {},
},
]
);
monacoEditorRef.current = editor;
setDecorations(decorations);
}

Copy link
Contributor Author

@Kevin101Zhang Kevin101Zhang Feb 20, 2024

Choose a reason for hiding this comment

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

Will go ahead and push the version without options. Doing this will keep the behavior the same. Will look into the type issue in a different branch

Copy link
Collaborator

@darunrs darunrs left a comment

Choose a reason for hiding this comment

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

A comment on the fix.

@darunrs
Copy link
Collaborator

darunrs commented Feb 20, 2024

Ok sounds good!

@Kevin101Zhang Kevin101Zhang merged commit 92d5526 into main Feb 21, 2024
@Kevin101Zhang Kevin101Zhang deleted the 569-glyph-view-on-debug-mode branch February 21, 2024 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Glyph view on debug mode
2 participants