-
Notifications
You must be signed in to change notification settings - Fork 49
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
Zest 2.0 #416
Comments
@ptziegler thx for this very nice summary on re-integrating Zest 2.0. Should we announce this also on the GEF and maybe also on the cross-projects mailing lists to get a wider reachability of this news. |
It'll probably still take a few months before everything is done, so I think that's a little premature. But it makes sense once the updated API is somewhat cleaned up and functional. |
Ok that is a good point. I thought more as a pre-anouncement that Zest users can get a a heads-up. Should we set-up a dedicated branch for that work? |
The branch already exists. A lot of the classes overlap with one another, so I don't think it's possible to reintegrate #410 without also addressing the API which is why everything has to be done in one go.
Which is something we should certainly do. I just don't see that there is anything more to say than "We'll eventually move to Zest 2.0". |
Hopefully I can make some time to continue working on this again tomorrow or within the next few days. Once I have a rough idea I can also draft an email. |
I thought more like a feature branch in this repo here. So that we can work in parallel and merge it back to master when its ready.
I was also not sure what to announce.
If I can support you in any way. Let me know. |
I've thought about the whole ordeal and also went through some of the bugs related to the previous Zest 2.0 update. https://bugs.eclipse.org/bugs/show_bug.cgi?id=283179 In short, there was a general concern regarding backwards compatibility and API breakage, which is why it was proposed to do the update in two steps. In the first step, Zest Layouts is updated to 2.0 but in such a way that it doesn't cause API breakage in Zest Core. And I think that's something worth pursuing. With the latest changes, I've (hopefully) reintegrated everything that came from the Zest 2.0 fork and marked everything as deprecated which should not be carried over. Zest Core remains at 1.x.
I suppose that makes sense. In that case I would push the current state as a starting point and everything else is then done via pull requests.
The worst part is probably done already :) Though there are some parts that broke due to this update (and were already broken on the fork), so if you can find the time to review some of the fixes, you'd already help out a lot. |
Also a few words regarding backwards compatibility: With the current implementation, the graphs support both Zest 1.x and Zest 2.x layout algorithms. Given that the API is completely different, I have done this by moving the old code to dedicated subclasses. For example, you have now a GridLayoutAlgorithm, which uses the Zest 2.x API and a GridLayoutAlgorithm.Zest1, which contains the old and unmodified implementation. At the few places in the graph where we actually perform the layout, we check the type of the layout algorithm and then call the matching "applyLayout()" method. Clients that only use the default layouts would automatically switch to the new API without having to do any additional work. And in case they have defined custom algorithms, they would first have to enable the legacy mode by implementing/extending the "Zest1" classes and then switch to the new API in a second phase. |
As suggested, I've pushed the Zest 2.0 branch to this repository via #476. I think the current state is something that can be merged, though I think only for the 2024-12 release. For the winter release, Zest Layouts will be raised to 2.0, while Zest Core remains at 1.x. Everything that has been deprecated in Zest 2.0 has been marked as deprecated and "for removal", so we should be able to freely remove it at a later point. For Zest Core, I would target the 2.0 for a later release, where the primary focus is to move the viewer into a separate bundle, so that the base bundle only depend on Draw2D and SWT. Unless there are any objects, I'll try to prepare a pre-annoucement where I summarize the current state. |
I think this is a very good approach. Please move forward with it. |
Given that the PR only slightly exceeds 1000 lines threshold, I've requested a review from the EMO team: https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/15699 This contribution should be fine I suppose, given that we're effectively merging one Eclipse project into another, but better safe than sorry. |
There is only one project: https://projects.eclipse.org/projects/tools.gef Moving or copying code between the Git repositories of one project should be a non-issue; the Platform has done this massively when they merged repositories. |
But we're effectively try to merge https://github.com/fsteeg/zest, which is a fork of the original GEF/Zest repository. My concern are the contributions that have been made outside of the Eclipse project, which I assume need to be vetted. |
I agree with @ptziegler that we should do some pre-checks here. Better save then sorry. |
The PR has been approved and the pre-announcement is out. There are still some minor things I want to clean up over the next few weeks but otherwise, I think we're good to go. |
I'm currently in the process of cleaning up most of the remaining compiler warnings, which should span roughly 20 PRs. @azoitl How should we handle them? Do you want to have a look at them or should I merge them when I think they're good? |
@ptziegler I was about to ask you about this. If you want to have second pair of eyes I'm happy to review all of them. |
Most of the changes just consists of cleaning up the raw types. You're free to have a look at them, if you want, but I don't think they're very interesting. Though I'll probably request a review for cases that affect the runtime behavior. |
I've integrated the README file of the Zest 2.0 fork into our wiki (excluding the DOT stuff), and extended the chapter about "Migrating Zest 1.x to Zest 2.x" to also include the legacy mode for old layout algorithms: |
As Trapattoni would say: "Ich habe fertig". The remaining PRs are all that's left to get everything up-to-date. At least in the test environment, everything seems to be running within expectations. So I think it's fine to merge everything next week. |
Many thx for this amazing work @ptziegler. I will create the maintenance branch and release builds on the weekend then master is ready for bringing it in. |
The Zest 2.0 branch is now on the master. Ideally, I'd like to create a milestone tomorrow, so that projects have a fixed version to migrate against. I wonder... does it make sense to already contribute it to SimRel, even if it isn't the M1 yet? |
An early milestone sounds good. Contributing to simrel early also sounds fine. |
I was looking at https://github.com/eclipse/gef-classic/wiki/Zest#zest-2x regarding the migration and I am a bit skeptical whether this migration approach is ideal: Ideally, we'd like out product to be compatible with both Eclipse 2024-09 (Zest 1.x) and 2024-09 (Zest 2.x). But we are inheriting from So code changes will be necessary either way and it seems impossible to have code that is compatible with both versions, as the Is there a best practice on how this scenario could be handled? Maybe contribute both 1.x and 2.x bundles to SimRel over one or more releases as transition period? |
I've been trying to migrate the p2 Aggregation Analyzer editor to Zest 2.0 layout with limited success. Formerly I can produce nice graph layouts like this: But I really can't get it quite right for the new stuff: The view is always too wide with the left most node is always touching the edge, but I printed out the location and it's not at the edge when I set the location during the layout:
Also nodes overlap, but I'm laying them out the same way as before, getting the width of the node and moving increasing the by So the EGit node should not overlap with the GMF Runtime node:
because 416 + 148 should be the right most x of the EGit rectangle and which to the left of 595. I don't really have a clue if I'm missing something (scaling?) or if these are bugs. Also, the layout is called very often when moving nodes and then selecting something else, for example. I don't expect that and it didn't behave that way before. There's a setup: https://github.com/eclipse-cbi/p2repo-aggregator/blob/main/CONTRIBUTING.md These are all the changes: This contains both the old Zest1 fully functional layout and the new Zest2 layout. I use the SimRel.aggran as the sample; sorry it does a lot of work before showing the graph. The other only thing I did for preparation was modify the setup to include the staging repository so that I pick up the staging version of GEF: |
@merks That is indeed a bug. Your new layout algorithm looks good to me, but the problem is that the coordinates are not transferred correctly from the layout to the widget. This line here looks very strange to me: gef-classic/org.eclipse.zest.core/src/org/eclipse/zest/core/widgets/InternalNodeLayout.java Lines 297 to 299 in c623fcf
And after a minor tweak, I got this result: I'll provide a fix shortly. |
Cooooollll! You're the best! Did you notice too how often layout is invoked. I.e., if you move a node and select another node after, the layout is run again? I added a Layout context menu action to layout out explicitly, but now just a Refresh from the context menu does a layout too. Otherwise the migration was much easier than I expected and the code looks cleaner to me... |
When you say Eclipse 2024-09, to you mean the Eclipse Platform or the SimRel release (Platform + everything else). Zest itself has a very weak dependency to the Platform. Meaning as long as you ship the 2.0 bundle with your product, it should be possible to install it in both releases. I suppose it would be possible to contribute both Zest 1.x and Zest 2.x to SimRel, but I'd really like to avoid that, if possible. Your product isn't part of the release train, I assume? Wouldn't it be possible to then also consume Zest from https://download.eclipse.org/tools/gef/classic/release/3.21.0 and then manage the version via an upper bound? |
@merks Does this also happen when you disable the dynamic layout? gef-classic/org.eclipse.zest.core/src/org/eclipse/zest/core/widgets/Graph.java Lines 457 to 477 in c623fcf
Once disabled, it shouldn't apply the layout unless explicitly requested. It possible that I missed some cases but then those simply need to be fixed. |
Let me try that tomorrow! 😁 |
Yes, I see no reason for older content to be on the release train. If one needs older content, there are plenty of repositories from which to get it. Not that many things depend on Zest: |
is it this one: #565 ? |
Yes |
I tried the nightly build just now and all problems I was seeing are resolved. 🥳 |
No it's a commercial (closed source) product delivered by SAP directly to our customers. |
And that's why I have a hard time understanding the underlying problem. Zest has indeed a dependency to the Eclipse Platform, because it's using JFace to implement its viewer. But those methods are ancient or at the very least don't use anything that has only been implemented in the 2024-09.
I can go into more detail, if you want. But what it boils down to is: The Zest 2.x and the Zest 1.x layouts are fundamentally different. The current approach is based on both the desire to keep changes to Zest Core to a minimum and to have the result be somewhat maintainable. Even the current compatibility mode was not an initial goal, but rather an afterthought due to Zest being robust enough to support both implementations at the same time. If you have a different solution that can achieve the same goals while at the same time also providing binary compatibility, I'm more than willing to discuss it. Just keep in mind that I think this requires a significant amount of additional work, which is a big part of the reason why there is now a 2.x at the start of the bundle version and something that would have to be initiated and contributed from your side. On a personal note: I'm well aware that major version changes and API breakage is always a real pain to deal with. Because of this, I deliberately announced this change several months ahead of time, in the hopes that everyone gets the chance to try out the branch, raise their concerns or bring up their own requirements, so that they can be taken into consideration while the implementation wasn't set in stone yet. And I apologize for sounding contemptuous, but that you bring up this issue after development has concluded is frustrating, to say the least and a part of me is even tempted to call it "too little too late". |
I'm here totally with @ptziegler. From all GEF Classic components Zest 1 was in the worst shape. He spent a tremendous amount of time to get Zest 2.x into such a shape that the impact is as small as possible. For me this is a major step forward in getting the whole GEF Classic in a modern maintainable state where we can also provide new features and improvements. |
The problem is that is is not a stand-alone product but essentially just an extension to be installed into an existing Eclipse installation. And our goal is to support the latest 2 SimRel versions of Eclipse at the same time.
Absolutely. Don't get me wrong, I am not trying to diminish the work that went into this. I am just trying to find ways how this can be made to work for us. This is the situation:
Therefore, it currently seems impossible to be compatible with a 2024-09 and a 2024-12 SimRel at the same time with standard means (*). Hence I was wondering whether the compat layer ( (*) I have looked into a framework |
I'm still not sure I understand the problem exactly. The new versions of Zest can install in really really old versions of the Platform: Nothing in SimRel needed to change to accommodate the new version. So you should be able to use the new version, strictly requiring via lower bound that new version, and you just need to make those zest things available in your own update site. Then it should be possible to install it into a 2024-09 installation (or 2024-06 for that matter)... But for some reason you seem to feel you can only get this working if the new version and the old version of zest are strictly binary compatible. But we were warned months ago that this would not be the case.... |
Only if there is nothing else installed in that 2024-09 / 06 installation already, which strictly requires Zest [1.0.0, 2.0.0). But yeah, maybe this is unlikely and I am indeed overthinking this. |
So effectively AbstractLayoutAlgorithm would be the Zest 1.x API and e.g. AbstractLayoutAlgorithm.Zest2 the Zest 2.x API. This is something I did consider, but I dismissed it for the following reason: Once we remove the deprecated Zest 1.x API, we would effectively have an empty AbstractLayoutAlgorithm class, with only AbstractLayoutAlgorithm.Zest2 remaining. The logical next step would then be to move everything from Addendum: Regardless of either approach, using Zest Core 1.14.0 will not work with Zest Layout 1.x, as it referencing the Zest1/Zest2 interface in order to determine, when the compatibility mode needs to be used. So while Zest Layout would be binarily compatible, Zest Core would not. |
Concretely these are the current strict requirements on zest: They each either specify no bounds or specify only a lower bound. So only in a installation with Linux tools (IDE for C++) or PHP (IDE for PHP) could there be a potential problem, but nothing would restrict zest.layout 2.x from being installed as required by your contribution. Likely PHP and C++ would have broken layouts, but that's what happens when you don't specify proper bounds on dependencies. Likely neither of these are the likely targets for your product... So yes, I hope you are overthinking the problem. In any case, careful thinking is a good thing! 👅 |
Motivation
Throughout the migration from SWT to FX, Zest received several bugixes, as well as a (from my perspective) cleaner API. With https://github.com/fsteeg/zest, containing the latest version that is still using SWT, we now have the unique opportunity to reintegrate those changes.
Goal
Full integration of Zest Layouts, Core, JFace and CloudIO, but not DOT (due to dependencies to XText/EMF).
Backwards compatibility with Zest 1.x layouts: Layouts are the bread and butter of Zest. Downstream projects should be able to use their custom layouts without requiring major changes.
Preservation of new Zest 1.x features: Our own implementation of Zest has evolved throughout the years and now contains functionality that never existed in Zest 2.0. Those features must not be removed.
Non-Goal
Roadmap
2024-12
Due dilligence process for new contribution
Reintegration of Zest 2.0 via Reintegrate Zest 2.x fork into master #476
Annotation of all deprecated classes as
@deprecated
andforRemoval
.Create an
AbstractLayoutAlgorithm
for the new Zest 2.0 layoutsRemove internal references to the deprecated API
Cleanup internal references to deprecated classes
2025-03
Separation of Zest Core into Zest Viewer and Zest Core
Update to 2.0 for Zest Core
The text was updated successfully, but these errors were encountered: