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

Ensure we apply feedback actions already on SetModel #322

Merged
merged 7 commits into from Mar 12, 2024

Conversation

martin-fleck-at
Copy link
Contributor

@martin-fleck-at martin-fleck-at commented Mar 4, 2024

What it does

Ensure that feedback actions are already applied on set-model and not only on update-model.
Fixes eclipse-glsp/glsp#1239

How to test

In the workflow example, you can add a startup listener that adds feedback early on, e.g.,

import { IDiagramStartup, IFeedbackActionDispatcher, MaybePromise, SelectAction, TYPES } from '@eclipse-glsp/client';
import { inject, injectable } from 'inversify';

@injectable()
export class StartupFeedback implements IDiagramStartup {
    @inject(TYPES.IFeedbackActionDispatcher) protected feedbackDispatcher: IFeedbackActionDispatcher;

    preRequestModel(): MaybePromise<void> {
        this.feedbackDispatcher.registerFeedback(this, [SelectAction.setSelection(['task0'])]);
    }
}

With this (and the correct binding in the module) you should see that we have a select feedback registered and applied even if all you do is open the model.

Follow-ups

Changelog

  • This PR should be mentioned in the changelog

  • This PR introduces a breaking change (if yes, provide more details below for the changelog and the migration guide)

  • Method FeedbackAwareUpdateModelCommand.getFeedbackCommands was move to IFeedbackEmitter for re-use, resulting in two new methods: getFeedbackCommands and applyFeedbackCommands.

  • Method FeedbackAwareUpdateModelCommand.getPriority was replaced by a generic rank property and the Ranked namespace.

  • The priority property (higher priority equals earlier execution) in FeedbackCommand was superseeded by a rank property (lower rank equals earlier execution).

@martin-fleck-at
Copy link
Contributor Author

@tortmayr Locally I seem some exceptions I still need to have a look at. Will report back.

@martin-fleck-at
Copy link
Contributor Author

@tortmayr I managed to resolve the dependency issue by using the contribution provider approach from Theia.

packages/protocol/src/utils/contribution-provider.ts Outdated Show resolved Hide resolved
packages/protocol/src/utils/contribution-provider.ts Outdated Show resolved Hide resolved
packages/protocol/src/utils/contribution-provider.ts Outdated Show resolved Hide resolved
packages/protocol/src/utils/contribution-provider.ts Outdated Show resolved Hide resolved
packages/protocol/src/utils/contribution-provider.ts Outdated Show resolved Hide resolved
packages/protocol/src/utils/contribution-provider.ts Outdated Show resolved Hide resolved
packages/protocol/src/utils/contribution-provider.ts Outdated Show resolved Hide resolved
- Avoid unnecessary types in contribution-provider, re-use GLSP ones
- Use undefined instead of 'null'
- Export 'ContributionProvider' also as 'TYPES.IContributionProvider'
- Adapt copyright header
Copy link
Contributor

@tortmayr tortmayr left a comment

Choose a reason for hiding this comment

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

Thanks! Change seems to work great for the most part.
I only have two minor nitpicks, (the final ones I promise😉 )

packages/glsp-sprotty/src/types.ts Show resolved Hide resolved
@tortmayr tortmayr self-requested a review March 11, 2024 17:00
Copy link
Contributor

@tortmayr tortmayr left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for your patience 😉
One minor suggestion, but nothing that should prevent merging this.

@martin-fleck-at martin-fleck-at merged commit 5e43503 into master Mar 12, 2024
6 checks passed
@martin-fleck-at martin-fleck-at deleted the issues/1239 branch March 12, 2024 07:39
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.

FeedbackActions are not applied after initial SetModel
2 participants