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

Moved feature interfaces to sprotty-protocol #413

Merged
merged 2 commits into from
Jan 3, 2024
Merged

Conversation

spoenemann
Copy link
Contributor

I moved most of the feature interfaces to the sprotty-protocol package so we can use them there. The old definitions are deprecated and can be removed in v2.0.0.

@spoenemann spoenemann added this to the v1.1.0 milestone Jan 3, 2024
Copy link
Contributor

@gfontorbe gfontorbe left a comment

Choose a reason for hiding this comment

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

From what I can see there are some features that are not moved:

  • LayoutableChild
  • LayoutContainer
  • Decoration
  • CreatingOnDrag
  • Deletable
  • EditableLabel
  • WithEditableLabel
  • Locateable
  • Nameable
  • Connectable

Any reason why those are not moved?

Should the symbols also be moved to sprotty-protocol?

Copy link
Contributor

Choose a reason for hiding this comment

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

For code organization, I am wondering if it would make more sense to move the features interfaces into their own features.ts instead of model.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving the existing features like BoundsAware would be a breaking change because with CommonJS imports you can import directly from sprotty-protocol/lib/model. We can consider a reorganization when we switch to ESM and package exports.

Copy link
Contributor

@jbicker jbicker left a comment

Choose a reason for hiding this comment

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

LGTM so far. What Guillaume mentioned here is worth a thought though.

@spoenemann
Copy link
Contributor Author

@gfontorbe I moved Locateable.

The problem with LayoutableChild and LayoutContainer is that they extend BoundsAware, but the definition of BoundsAware in the external model (position & size) differs from the one in the internal model (bounds). I renamed the three interfaces to InternalBoundsAware, InternalLayoutableChild and InternalLayoutContainer to make the difference explicit.

The other features are either empty interfaces (Decoration, Deletable) or are used only in the frontend AFAICT.

@spoenemann spoenemann merged commit 3a3a0a0 into master Jan 3, 2024
2 of 3 checks passed
@spoenemann spoenemann deleted the feature-protocol branch January 3, 2024 14:10

export interface SShapeElement extends SModelElement, Partial<BoundsAware> {
layoutOptions?: ModelLayoutOptions
export interface SShapeElement extends SModelElement, Partial<LayoutableChild> {

Choose a reason for hiding this comment

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

SShapeElement is not BoundsAware anymore. Was that intended?
SNode, SPort, and SLabel don't have position and size anymore, or do I overlook something?

Choose a reason for hiding this comment

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

Ah, it is inherited via LayoutableChild? The name is a bit misleading, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have an alternative proposal?

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.

4 participants