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

Refactor code base #259

Merged
merged 9 commits into from
Jul 6, 2023
Merged

Refactor code base #259

merged 9 commits into from
Jul 6, 2023

Conversation

tortmayr
Copy link
Contributor

@tortmayr tortmayr commented Jul 6, 2023

Refactor base package

  • Move FeedbackActionDispatcher and related components into base package
  • Refactor FeedbackAwareModelCommand. Remove ModelRootChangeListener API from this command and add it to the command stack instead. This ensures that the model listeners are notified after the update has been processed succeessfully. It also makes it easier to rebind the UpdateModelCommand as you don't have to consider the listeners anymore.

Update to lerna 7 and fix test cases

Refactor SelectionService

  • Remove unnecessary Types.SelectionService binding and bind SelectionService directly instead
  • Move SelectionService into base package. A lot of feature modules are dependent on this service => they would no longer work if the selectModule is not bound or customized

Move container module out of base package

The base package should have no direct dependencies to feature module to avoid circular dependency issues.

Fixes eclipse-glsp/glsp#1049

- Move `FeedbackActionDispatcher` and related components into base package
- Refactor `FeedbackAwareModelCommand`. Remove `ModelRootChangeListener` API from this command and add it to the command stack instead. This ensures that the model listeners are notified after the update has been processed succeessfully. It also   makes it easier to rebind the `UpdateModelCommand` as you don't have to consider the listeners anymore.
- Remove unnecessary Types.SelectionService binding and bind SelectionService directly instead
- Move `SelectionService` into base package. A lot of feature modules are dependent on this service => they would no longer work if the selectModule is not bound or customized
The base package should have no direct dependencies to feature module to avoid circular dependency issues.
The mouseTool is part of the default sprotty module and our customization should therefore be part of the `glspDefaultModule`.
- Move mouse tool into base package
- Rework `Rank` interface and utility function to our default interface+namespace pattern
- Remove `TYPES.MouseTool` service identifert, and unecessary interface wrapper. Directly inject mousetool instead
Copy link
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.

I love a good refactoring and cleanup commit, so thank you very much Tobias! From my tests, everything seems to work well but I do have some questions and minor comments.

packages/client/src/base/di.config.ts Outdated Show resolved Hide resolved
packages/client/src/base/di.config.ts Show resolved Hide resolved
packages/client/src/base/view/mouse-tool.ts Show resolved Hide resolved
packages/client/src/base/selection-service.ts Show resolved Hide resolved
packages/client/src/base/command-stack.ts Outdated Show resolved Hide resolved
@tortmayr
Copy link
Contributor Author

tortmayr commented Jul 6, 2023

Thanks for the fast review Martin. I have pushed an update to address the open issues.

Copy link
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.

LGTM!

@tortmayr tortmayr merged commit f12aad0 into master Jul 6, 2023
5 checks passed
@tortmayr tortmayr deleted the wip branch July 6, 2023 13:51
tortmayr added a commit to eclipse-glsp/glsp-theia-integration that referenced this pull request Jul 9, 2023
tortmayr added a commit to eclipse-glsp/glsp-theia-integration that referenced this pull request Jul 12, 2023
* GLSP-1049: Update to new client version

Update code base to conform to API breaks from eclipse-glsp/glsp-client#259
Part of eclipse-glsp/glsp-client#259

* GLSP-1050: Update to Event API

Requires eclipse-glsp/glsp-client#261
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve client API
2 participants