Ruler guides are broken in GEF 3.19.0.202402212051

[wiresketch]

When using ruler guides the following exception is constantly being logged and ruler guides do not work:

java.lang.NullPointerException: Cannot invoke "java.lang.Integer.intValue()" because the return value of "org.eclipse.gef.internal.ui.rulers.RulerLayout.getConstraint(org.eclipse.draw2d.IFigure)" is null
	at org.eclipse.gef.internal.ui.rulers.RulerLayout.layout(RulerLayout.java:55)
	at org.eclipse.draw2d.Figure.layout(Figure.java:1201)
	at org.eclipse.draw2d.Figure.validate(Figure.java:2060)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at org.eclipse.draw2d.Figure.validate(Figure.java:2061)
	at org.eclipse.draw2d.Viewport.validate(Viewport.java:378)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at org.eclipse.draw2d.Figure.validate(Figure.java:2061)
	at org.eclipse.draw2d.DeferredUpdateManager.performValidation(DeferredUpdateManager.java:225)
	at org.eclipse.draw2d.DeferredUpdateManager.paint(DeferredUpdateManager.java:167)
	at org.eclipse.draw2d.LightweightSystem.paint(LightweightSystem.java:202)
	at org.eclipse.draw2d.LightweightSystem.lambda$0(LightweightSystem.java:110)
	at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:89)
	at org.eclipse.swt.widgets.Display.sendEvent(Display.java:5855)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1529)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1555)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1538)
	at org.eclipse.swt.widgets.Control.gtk_draw(Control.java:3927)
	at org.eclipse.swt.widgets.Scrollable.gtk_draw(Scrollable.java:365)
	at org.eclipse.swt.widgets.Composite.gtk_draw(Composite.java:506)
	at org.eclipse.swt.widgets.Canvas.gtk_draw(Canvas.java:174)
	at org.eclipse.swt.widgets.Widget.windowProc(Widget.java:2482)
	at org.eclipse.swt.widgets.Control.windowProc(Control.java:6883)
	at org.eclipse.swt.widgets.Display.windowProc(Display.java:6162)
	at org.eclipse.swt.internal.gtk3.GTK3.gtk_main_do_event(Native Method)
	at org.eclipse.swt.widgets.Display.eventProc(Display.java:1598)
	at org.eclipse.swt.internal.gtk3.GTK3.gtk_main_iteration_do(Native Method)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:4514)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$5.run(PartRenderingEngine.java:1151)

The issue is introduced in commit 4635985 from Dec 30, 2023 which apparently only modernizes code, but breaks the ruler functionality. The problem is with the change in XYLayout where constraints are forced to be Rectangles. Since setConstraint method takes an object the change adds an instanceof check and silently rejects all other types of constraints:

	public void setConstraint(IFigure figure, Object newConstraint) {
		super.setConstraint(figure, newConstraint);
		if (newConstraint instanceof Rectangle rect) {
			constraints.put(figure, rect);
		}
	}

Previously this method looked like this:

	public void setConstraint(IFigure figure, Object newConstraint) {
		super.setConstraint(figure, newConstraint);
		if (newConstraint != null)
			constraints.put(figure, newConstraint);
	}

This change breaks RulerLayout class which is a subclass of XYLayout and which uses Integer constraints instead of Rectangle.

This issue is really critical since I couldn't not find a way to add a workaround in my app. The fix could either revert this change, or perhaps make RulerLayout inherit directly from AbstractLayout to remove the dependency on XYLayout.

I am really worried about all this changes lately. Code modernization is nice, but doing it just for the sake of it feels unnecessary to me. On the other hand this is the second breaking change I find, and this just in the code I use, so this trend really worries me.

[azoitl]

@wiresketch first of all I totally understand your concern and I'm also not happy about the situation. I'm really trying my best to not produce situations like the one you found now twice. On the other hand we have the situation that GEF Classic has a lot of dangerous code that can lead in special situation to severe issues. Also we have performance issues and would like to get new features in (e.g., #261). That is one of the reasons I spend my time on cleaning the code.

Coming back to the problem you found. After looking at the code I strongly belive that RulerLayout should never have inherited from XYLayout. If you agree I would change that to AbstractLayout as you suggest and fix the problems we have here.

[wiresketch]

@azoitl I agree that making RulerLayout extend AbstractLayout is a better solution. I gave it a try below to see what methods would need to be brought from XYLayout into RulerLayout to ensure that the behavior does not change. Feel free to use this code.

/*******************************************************************************
 * Copyright (c) 2003, 2010 IBM Corporation and others.
 *
 * This program and the accompanying materials are made available under the
 * terms of the Eclipse Public License 2.0 which is available at
 * http://www.eclipse.org/legal/epl-2.0.
 *
 * SPDX-License-Identifier: EPL-2.0
 *
 * Contributors:
 *     IBM Corporation - initial API and implementation
 *******************************************************************************/
package org.eclipse.gef.internal.ui.rulers;

import java.util.HashMap;
import java.util.Map;

import org.eclipse.draw2d.AbstractLayout;
import org.eclipse.draw2d.IFigure;
import org.eclipse.draw2d.LayoutManager;
import org.eclipse.draw2d.geometry.Dimension;
import org.eclipse.draw2d.geometry.Point;
import org.eclipse.draw2d.geometry.Rectangle;

/**
 * A custom layout manager for rulers. It is not meant to be used externally or
 * with any figure other than a
 * {@link org.eclipse.gef.internal.ui.rulers.RulerFigure ruler}.
 *
 * @author Pratik Shah
 * @since 3.0
 */
public class RulerLayout extends AbstractLayout {

	/** The layout contraints */
	private Map<IFigure, Integer> constraints = new HashMap<>();

	/**
	 * @see org.eclipse.draw2d.AbstractLayout#calculatePreferredSize(org.eclipse.draw2d.IFigure,
	 *      int, int)
	 */
	@Override
	protected Dimension calculatePreferredSize(IFigure container, int wHint, int hHint) {
		return new Dimension(1, 1);
	}

	/**
	 * @see org.eclipse.draw2d.AbstractLayout#getConstraint(org.eclipse.draw2d.IFigure)
	 */
	@Override
	public Object getConstraint(IFigure child) {
		return constraints.get(child);
	}

	/**
	 * Returns the origin for the given figure.
	 *
	 * @param parent the figure whose origin is requested
	 * @return the origin
	 */
	public Point getOrigin(IFigure parent) {
		return parent.getClientArea().getLocation();
	}

	/**
	 * @see org.eclipse.draw2d.LayoutManager#layout(org.eclipse.draw2d.IFigure)
	 */
	public void layout(IFigure container) {
		Rectangle rulerSize = container.getClientArea();
		for (IFigure child : container.getChildren()) {
			Dimension childSize = child.getPreferredSize();
			int position = ((Integer) getConstraint(child)).intValue();
			if (((RulerFigure) container).isHorizontal()) {
				childSize.height = rulerSize.height - 1;
				Rectangle.SINGLETON.setLocation(position - (childSize.width / 2), rulerSize.y);
			} else {
				childSize.width = rulerSize.width - 1;
				Rectangle.SINGLETON.setLocation(rulerSize.x, position - (childSize.height / 2));
			}
			Rectangle.SINGLETON.setSize(childSize);
			child.setBounds(Rectangle.SINGLETON);
		}
	}

	/**
	 * @see LayoutManager#remove(IFigure)
	 */
	@Override
	public void remove(IFigure figure) {
		super.remove(figure);
		constraints.remove(figure);
	}

	/**
	 * Sets the layout constraint of the given figure. The constraints can only be
	 * of type {@link Integer}.
	 *
	 * @see LayoutManager#setConstraint(IFigure, Object)
	 */
	@Override
	public void setConstraint(IFigure figure, Object newConstraint) {
		super.setConstraint(figure, newConstraint);
		if (newConstraint != null)
			constraints.put(figure, (Integer) newConstraint);
	}
}

[azoitl]

Thx this helps. I tried and in my RCP this solves the problem. I would have two questions where I would be happy to get feedback:

1. I think setConstraint should check if the newConstraint is an Integer, but what to do if it is not?
2. RulerLayout.getOrigin() is never directly called and there is no cast to XYLayout. So I think we could remove that.

[wiresketch]

1. I strongly believe that setConstraint method should throw ClassCastException instead of silently ignoring constraints of other types. This way the code that uses RulerLayout incorrectly can fail and then be directly identified. The silent solution will make problems resurface in other parts of the code, but in those cases it will be much more difficult to identify the original issue. For example to identify the current issue I had to resort to debugging to locate the original cause.
2. You are right about getOrigin method. I missed the fact that it's not inherited from AbstractLayout.

[azoitl]

ad 1) you have a good point here. Will follow that.

[ptziegler]

A few cents from my side:

1. I generally agree that an exception should be thrown when an invalid constraint is used. The JavaDoc even explicitly says `The constraints can only be of type Rectangle. But wouldn't an IllegalArgumentException be more appropriate?

An more generally, how difficult do you two think it would be to create a small ruler example? It's apparently used in the Logic example, but I have yet to figure out how to make it show up exactly.

I think a large part of this problem is caused by our abysmal test coverage. So whenever such regressions do show up, I'd like to include a reproducer it to our automatic tests.

[azoitl]

A few cents from my side:

1. I generally agree that an exception should be thrown when an invalid constraint is used. The JavaDoc even explicitly says `The constraints can only be of type Rectangle. But wouldn't an IllegalArgumentException be more appropriate?

That seems for me even better.

An more generally, how difficult do you two think it would be to create a small ruler example? It's apparently used in the Logic example, but I have yet to figure out how to make it show up exactly.

I think a large part of this problem is caused by our abysmal test coverage. So whenever such regressions do show up, I'd like to include a reproducer it to our automatic tests.

I totally agree. And I'm really bad at testing. I fear the ruler would need a full fledged GEF Classic editor to be tested. Waht I can try is to see how we get the ruler shown in the Logic editor. Which I think would anyhow be a good idea.

[wiresketch]

@ptziegler From the looks of it the logic example should have a menu entry for toggling the ruler visibility.

[ptziegler]

From the looks of it the logic example should have a menu entry for toggling the ruler visibility.

Thanks! I found it.

[azoitl]

There I wouldn't have searched for it.