BIRT 4.16 202405030536, Designer-Crashed: topic depended on JRE 17 vs. 21

[speckyspooky]

The All-In-One-designer is crashed with the currently "nightly build".

Tested designer versions from my side 4.16:

» N202405030536: 4.16 designer crashed » N202404301804: 4.16 ok » N202404291807: 4.16 ok » N202404271127: 4.16 ok » N202404261927: 4.16 ok

Could it be a topic with the latest eclipse changes an the required version of JRE 21 for some libraries or some central changes at eclipse-framwork side? I have no problems with my dev-environment locally.

Log: log.zip

Designer

Error-Screen 01, 1st message

Error-Screen 02, 2nd message

[merks]

GEF has been making lots of changes lately. It seems to me we already dealt with a problem like this in the recent past.

I have limited capacity this week to investigate problems.

[merks]

@speckyspooky

If you do Help -> Perform Setup Tasks... you will update the target platform and see this problem.

The problem is a result of this commit:

https://github.com/eclipse/gef-classic/commit/451bc1fff9a904b7e303c5747d05989b738d1c70

Which is for this issue:

https://github.com/eclipse/gef-classic/issues/418

It happens here:

org.eclipse.birt.report.designer.internal.ui.editors.rulers.EditorGuideEditPart.updateLocationOfFigures(int)

Apparently that should now be a Rectangle, but I have no idea the design intent here in BIRT, i.e., how to convert the position to a rectangle.

@azoitl @ptziegler

I'm not sure this new exception throwing is okay. The API being called has no constraint on the constraint:

Here are other places it's called by BIRT:

Probably the x-ed out one is the only bogus one...

@speckyspooky

Commenting out that line seems to fix the problem; maybe this never did anything and was always just quietly ignore.

[ptziegler]

I'm a little bit confused. I see a call to getRulerEditPart() using a XYLayout, even though I would expect a RulerLayout.

The reason we now thrown an exception when calling setLayoutConstraint with an invalid parameter is because it would otherwise only lead to a ClassCastException when calling layout.

public void layout(IFigure parent) {
    Point offset = getOrigin(parent);
    for (IFigure f : parent.getChildren()) {
        Rectangle bounds = (Rectangle) getConstraint(f);
        if (bounds == null) {
            continue;
        }
        if (bounds.width == -1 || bounds.height == -1) {
            Dimension preferredSize = f.getPreferredSize(bounds.width, bounds.height);
            bounds = bounds.getCopy();
            if (bounds.width == -1) {
                bounds.width = preferredSize.width;
            }
            if (bounds.height == -1) {
                bounds.height = preferredSize.height;
            }
        }
        bounds = bounds.getTranslated(offset);
        f.setBounds(bounds);
    }
}

I can have a look this evening, but it looks a lot like the wrong layout is assigned to the edit part.

[merks]

FYI, I'm traveling this week so my connectivity will be spotty at best.

[ptziegler]

The EditorRulerEditorPart (or the EditorRulerFigure, to be more specific) is using the EditorRulerLayout, which extends the XYLayout.

Because it overwrites the layout, method, everything works at runtime, even when integers are used as constraint. But by doing so, it violates the contract specified in the JavaDoc of setConstraint, which is what's leading to the error mentioned above.

/**
 * Sets the layout constraint of the given figure. The constraints can only be
 * of type {@link Rectangle}.
 *
 * @see LayoutManager#setConstraint(IFigure, Object)
 * @since 2.0
 */

[ptziegler]

Given that I can't predict how many other applications might be doing something similar, it probably makes sense to simply log an error/warning, rather than throwing an exception. This way clients still get feedback that something fishy is going on, without immediately breaking the editor.

@azoitl wdyt?

[azoitl]

@ptziegler Given the legacy that is out I think this is a better solution. I also had a look what we could do to make the live for our users easier. Because I still strongly think that BIRT should not have used XYLayout here. One problem I noted is that all the GEF Classic ruler stuff is internal. Which is bad because my initial suggestion would have been to inherit from GEF ClassicsRulerLayout. But at that point I can not grasp the implication of making some of the ruler stuff API.

One thing that we can do for people that are doing similar things is to introduce a new AbstractConstraintLayout, where we move the constraint handling stuff from XYLayout and RulerLayout.

[ptziegler]

One problem I noted is that all the GEF Classic ruler stuff is internal.

My impression is that BIRT effectively copy-pasted the RulerLayout class to avoid exactly this problem. Which is why the EditorRulerLayout extends XYLayout, as this also used to be the case for the original class.

One thing that we can do for people that are doing similar things is to introduce a new AbstractConstraintLayout, where we move the constraint handling stuff from XYLayout and RulerLayout.

Makes sense to me.

[azoitl]

With this GEF Classic PR the quickest and most correct fix is to change the parent class of EditorRulerLayout to AbstractConstraintLayout.

[ptziegler]

In addition, I've also created https://github.com/eclipse/gef-classic/pull/440, which fixes this problem without the need to adapt BIRT.

Instead of throwing an exception, the layouts now simply create a warning and add them to the error log.

[merks]

I'm switching the target platform to the nightly build:

https://github.com/eclipse-birt/birt/pull/1676

Indeed it only logs now. What should be properly do to actually fix the problem? I don't understand what the constraint's purpose is in the BIRT usage....

[ptziegler]

What should be properly do to actually fix the problem?

Instead of extending the XYLayout, the EditorRulerLayout should instead extend the AbstractConstraintLayout.

[speckyspooky]

I have retested 2 things: - 1st test: latest version with the fixes on GEF without an change on BIRT - 2nd, test: change BIRT, the EditorRulerLayout extend the AbstractConstraintLayout (below chang screen)

The result is in both cases the same. The good thing is the designer is running. And in both cases the same warnings will be thrown on the "error log"-page.

But I thought with the move to the AbstractConstraintLayout we will fix the warnings too.

[ptziegler]

And in both cases the same warnings will be thrown on the "error log"-page.

That's not something I can reproduce. I also see that there is a noticable difference in the timestamps. Are you certain that those aren't just the warnings created from before your change?

But I thought with the move to the AbstractConstraintLayout we will fix the warnings too.

This only removes the warnings for the EditorRulerLayout. But the same problem seems to also exist for the FixTableLayout (or rather the TableLayout); You are extending the XYLayout (which expects Rectangles as constraint), but use instances of type WorkingData

I think this warning is accurate and it highlights a problem that can very easily lead to a ClassCastException. Because TableLayout doesn't overwrite calculatePreferredSize(), one would get a ClassCastException due to the following implementation in XYLayout:

public void layout(IFigure parent) {
    Point offset = getOrigin(parent);
    for (IFigure f : parent.getChildren()) {
        Rectangle bounds = (Rectangle) getConstraint(f);
        if (bounds == null) {
            continue;
        }

        if (bounds.width == -1 || bounds.height == -1) {
            Dimension preferredSize = f.getPreferredSize(bounds.width, bounds.height);
            bounds = bounds.getCopy();
            if (bounds.width == -1) {
                bounds.width = preferredSize.width;
            }
            if (bounds.height == -1) {
                bounds.height = preferredSize.height;
            }
        }
        bounds = bounds.getTranslated(offset);
        f.setBounds(bounds);
    }
}

Furthermore, this layout is used within the TableXYLayoutEditPolicy. Because it extends the XYLayoutEditPolicy, you end up with the same potential ClassCastException in e.g. getCurrentConstraintFor()

protected Rectangle getCurrentConstraintFor(GraphicalEditPart child) {
    IFigure fig = child.getFigure();
    return (Rectangle) fig.getParent().getLayoutManager().getConstraint(fig);
}

My suggestion is repeat the same thing for the TableLayout class and have it extend AbstractConstraintLayout and for the TableXYLayoutEditPolicy, it should extend the ConstrainedLayoutEditPolicy instead.

[speckyspooky]

Yes, I saw same classes on my debug session some minutes ago. I will directly test your hints. Thanks for that!

[speckyspooky]

Summary of the 3 extended changes and I agree to solve all topics there is more to after the replacement:

EditorRulerLayout

• changed from XYLayout to AbstractConstraintLayout
• missing methods: none

TableLayout

• changed from XYLayout to AbstractConstraintLayout
• missing methods: calculatePreferredSize(IFigure f, int wHint, int hHint)

TableXYLayoutEditPolicy

• changed from XYLayoutEditPolicy to ConstrainedLayoutEditPolicy
• missing methods: getConstraintFor(Point), getConstraintFor(Rectanlge)
• existing methods not defined: setXyLayout(layout), is given in XYLayoutEditPolicy

[ptziegler]

TableLayout

• changed from XYLayout to AbstractConstraintLayout
• missing methods: calculatePreferredSize(IFigure f, int wHint, int hHint)

TableLayout already has a method called calculateMinimumSize(). My naïve approach would've been to simply rename it to calculatePreferredSize(). The method from XYLayout couldn't have been called before, given that it would've caused a CCE, so it shouldn't cause any undesirable side effects.

TableXYLayoutEditPolicy

• changed from XYLayoutEditPolicy to ConstrainedLayoutEditPolicy
• missing methods: getConstraintFor(Point), getConstraintFor(Rectanlge)
• existing methods not defined: setXyLayout(layout), is given in XYLayoutEditPolicy

From what I understand, this edit policy is only used to handle resizing the table cells. It's probably fine to leave them as dummy implementations (similar to most of the other methods).

[speckyspooky]

I have created a first PR #1678 and added the missing methods accordingly the original sources to avoid behavior changes. The warnings are not longer listed :o)

My be we can change these according to your suggestion as dummy-methods but it is the first time that I'm so deep into the designer-engine level and therefore the pesimistic implementation of the copied methods.

@merks What do you think about it?

[speckyspooky]

The change is merged to the BIRT-master.