Skip to content

Issues/1546#474

Open
Sakrafux wants to merge 6 commits intoeclipse-glsp:issues/1546from
Sakrafux:issues/1546
Open

Issues/1546#474
Sakrafux wants to merge 6 commits intoeclipse-glsp:issues/1546from
Sakrafux:issues/1546

Conversation

@Sakrafux
Copy link
Copy Markdown
Contributor

Extends the GLSP MCP Server started in PR-456 according to our meetings @martin-fleck-at.

Copy link
Copy Markdown
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

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

Thank you @Sakrafux! I think we need to make some changes here but I believe some of the problems will go away once we properly implement eclipse-glsp/glsp#607 which I plan to tackle next, so we can leave that for now as is.

return {
kind: KIND,
mcpRequestId,
...options
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Options are spread here into their nested fields but in the action interface they are declared in a single options field, this will not work as expected.

kind: KIND,
mcpRequestId,
png,
...options
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Options are spread here into their nested fields but in the action interface they are declared in a single options field, this will not work as expected.

import { inject, injectable } from 'inversify';

@injectable()
export class GetSelectionMcpCommand extends Command {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this might need to be a SystemCommand so that we do not pollute the undo/redo stack.

const ctx = offscreen.getContext('2d');

if (!ctx) {
reject(new Error('Failed to get offscreen context.'));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should make sure to call URL.revokeObjectURL(url) here as well.

};
reader.readAsDataURL(outBlob);
} catch (err) {
reject(err);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should make sure to call URL.revokeObjectURL(url) here as well.

mcpRequestId: string;
}
export namespace GetSelectionMcpAction {
export const KIND = 'GetSelectionMcpAction';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In GLSP we typically use camelCase and not PascalCase for this sort of string. Also true for the others.

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.

2 participants