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

[Modernice Code] Typify IFigure/Figure's Children #149

Merged
merged 2 commits into from
Apr 10, 2023

Conversation

azoitl
Copy link
Contributor

@azoitl azoitl commented Mar 26, 2023

In my attempt to modernize GEF classic code I worked on the IFigure and more important the Figures infrastructure by typifying the children list in Figure and change the return value of getChildren to List<? extends IFigure>. With that we can get rid of many casts and instanceof checks. Also Client code can take advantage of it. In order to test it I compiled GEF classic and all its examples as well as with Eclipse GMF runtime and Eclipse 4diac. To ensure that I haven't broken any core features (e.g., selection) I ran the GEF classic logic editor and Eclipse 4diac.

Finally to show the advantages I completely reworked the Draw2d Tree example.

I hope with that caution it should be a save PR.

\cc @Phillipus @pcdavid @Destrolaric

@Destrolaric
Copy link
Contributor

@azoitl Hi, I think it may cause later conflicts with planned pr Destrolaric#2. Unfortunately, I can't currently open pr to this repository for now.

@azoitl
Copy link
Contributor Author

azoitl commented Mar 27, 2023

@Destrolaric I'm sorry I was not aware that you are also working on that topic. I had a quick look on your current state. Based on that I would like to start with my commit. There are a few reasons for that:

  1. You changed a lot in one commit. This is hard to review and my have lot of side effects that we don't know of yet.
  2. You have for example 'List getChildren();' as update for IFigure. I tried something similar once and this is potentially breaking existing users of GEF classic. Therefore I had to revert one of my commits in that direction. We need a less intrusive approach. That is the reason that I have in this pull request 'List<? extends IFigure> getChildren();'
  3. I have 4 commits building on this one that I would issue PRs when this one is merged:

How can we better synchronize that we are not duplicating each others work?

@Destrolaric
Copy link
Contributor

@azoitl Hmm, maybe we can split the parts each of us could refactor. I think it may be viable in that case.

@azoitl
Copy link
Contributor Author

azoitl commented Mar 28, 2023

@azoitl Hmm, maybe we can split the parts each of us could refactor. I think it may be viable in that case.

How about creating an issue where we can discuss that? This could also help us to document solutions and best practices and how to evaluate changes.

For now I would be most grateful for a fix to the Draw2d eclipse platform dependency problem as this is breaking our tests and breaking for potential users.

@Destrolaric
Copy link
Contributor

@azoitl Yes, it will be a good idea to create an issue where it could be discussed. I will try to work out the fix in a short time.

@azoitl
Copy link
Contributor Author

azoitl commented Mar 28, 2023

@Destrolaric I created Issue #155 where I tried to summarize what I think we would like to achieve. But I also mentioned some previous issues I phased with doing that and why I'm a bit more cautious now. Feel free to extend and comment.

@azoitl azoitl force-pushed the children_IFigure branch from 617c8ce to e67b5df Compare April 9, 2023 19:42
@github-actions
Copy link

github-actions bot commented Apr 9, 2023

Unit Test Results

    6 files      6 suites   9s ⏱️
166 tests 166 ✔️ 0 💤 0
332 runs  332 ✔️ 0 💤 0

Results for commit 1149e92.

♻️ This comment has been updated with latest results.

@azoitl azoitl force-pushed the children_IFigure branch from e67b5df to 3fb352d Compare April 9, 2023 20:12
azoitl added 2 commits April 10, 2023 11:55
To show how updated IFgiure getChildren typed interface can be used to
improve client code the Draw2d example was cleaned and reworked.
@azoitl azoitl force-pushed the children_IFigure branch from 3fb352d to 1149e92 Compare April 10, 2023 09:56
@azoitl azoitl merged commit 9e66566 into eclipse-gef:master Apr 10, 2023
@azoitl azoitl deleted the children_IFigure branch April 12, 2023 16:35
@azoitl azoitl added this to the 3.16.0 milestone Jun 9, 2023
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