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

[Zest 2.0] ZestRootLayer constants specify layer order #518

Open
BauePhil opened this issue Aug 20, 2024 · 5 comments
Open

[Zest 2.0] ZestRootLayer constants specify layer order #518

BauePhil opened this issue Aug 20, 2024 · 5 comments
Labels

Comments

@BauePhil
Copy link
Contributor

BauePhil commented Aug 20, 2024

So I already had a bit of a problem with the old version of the ZestRootLayer class, as that had a boolean constant to determine if the connections should be behind or in front of nodes. Now the new version is a lot cleaner but has the same issue. It just has a constant for each layer's order now.
My easy fix would be to have overridable getter methods that return the constant, which I could push onto the PR.
A perhaps prettier solution could be to use something like a LayeredPane and layers, but I could not get this to work right now.
@ptziegler and @azoitl as you two seem to be the leading pair for the reintegration of zest 2.0 what do you think?

@ptziegler
Copy link
Contributor

If I understand you correctly, you want to make the order of layers configurable, so that e.g. the connections can be show in front of the nodes (and vice versa)?

A perhaps prettier solution could be to use something like a LayeredPane and layers, but I could not get this to work right now.

I agree that this would be the cleanest solution. However, I fear that there are several other places where it is assumed that the figures are added directly to the root layer, which would then have to be adapted as well.

As a starting point, I can already say that one would would have to work with the FreeformLayeredPane and FreeformLayer and then slowly clean up everything that's causing issues.

@BauePhil
Copy link
Contributor Author

BauePhil commented Aug 21, 2024

If I understand you correctly, you want to make the order of layers configurable, so that e.g. the connections can be show in front of the nodes (and vice versa)?

Yes exactly.

As a starting point, I can already say that one would would have to work with the FreeformLayeredPane and FreeformLayer and then slowly clean up everything that's causing issues.

I tried doing this today and can only get this to work if I explicitly give all FreeFormLayers a FreeFormLayout as layoutmanager (the editparts e.g. do not have this as far as i have seen) and overwrite the validate() method of the ZestRootLayer to also validate the layers as it seems this is not done recursively. This feels rather hacky and wrong or is it? Would you know what the underlying problem is?

@ptziegler
Copy link
Contributor

I tried doing this today and can only get this to work if I explicitly give all FreeFormLayers a FreeFormLayout as layoutmanager (the editparts e.g. do not have this as far as i have seen) and overwrite the validate() method of the ZestRootLayer to also validate the layers as it seems this is not done recursively.

I assume the problem is that the layers have empty bounds? The ZestRootLayer itself is a sub-layer of a ScalableFreeformLayeredPane:

https://github.com/eclipse/gef-classic/blob/0470a02dd6c30e0b976a1213d38433da93a52b9c/org.eclipse.zest.core/src/org/eclipse/zest/core/widgets/Graph.java#L1435-L1445

Meaning the bounds of the Zest layer is calculated using the FreeFormHelper, which also contains the recursive update you were missing:

https://github.com/eclipse/gef-classic/blob/0470a02dd6c30e0b976a1213d38433da93a52b9c/org.eclipse.draw2d/src/org/eclipse/draw2d/FreeformHelper.java#L88-L97

This is also the reason why all figures in this change need to implement FreeformFigure, one way or the other. If this doesn't work on your side, then I assume this chain is broken.

That said, I played around with this class and this is the initial draft I came up with:

/*******************************************************************************
 * Copyright (c) 2000-2010, 2024 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
 * Chisel Group, University of Victoria - Adapted for XY Scaled Graphics
 *******************************************************************************/
package org.eclipse.zest.core.widgets.internal;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;

import org.eclipse.draw2d.FreeformLayer;
import org.eclipse.draw2d.FreeformLayeredPane;
import org.eclipse.draw2d.IFigure;

/**
 * The root figure for Zest. The figure is broken up into following segments:
 * <ol>
 * <li>The Connections</li>
 * <li>The Subgraphs</li>
 * <li>The Nodes</li>
 * <li>The Highlighted Connections</li>
 * <li>The Highlighted Nodes</li>
 * </ol>
 *
 */
public class ZestRootLayer extends FreeformLayeredPane {

	public static final Object CONNECTIONS_LAYER = 0;

	public static final Object SUBGRAPHS_LAYER = 1;

	public static final Object NODES_LAYER = 2;

	public static final Object CONNECTIONS_HIGHLIGHTED_LAYER = 3;

	public static final Object NODES_HIGHLIGHTED_LAYER = 4;

	public static final Object TOP_LAYER = 5;

	private final Map<IFigure, IFigure> layerFigures = new HashMap<>();

	public ZestRootLayer() {
		add(new FreeformLayer(), CONNECTIONS_LAYER);
		add(new FreeformLayer(), SUBGRAPHS_LAYER);
		add(new FreeformLayer(), NODES_LAYER);
		add(new FreeformLayer(), CONNECTIONS_HIGHLIGHTED_LAYER);
		add(new FreeformLayer(), NODES_HIGHLIGHTED_LAYER);
		add(new FreeformLayer(), TOP_LAYER);
	}

	/**
	 * Set of all figures that are decorations for other figures. A decoration
	 * figure is always put one position (or more if there's more than one
	 * decoration for the same figure) after the decorated figure in children list.
	 */
	private final Set<IFigure> decoratingFigures = new HashSet<>();

	/**
	 * Adds a node to the ZestRootLayer
	 *
	 * @param nodeFigure The figure representing the node
	 */
	public void addNode(IFigure nodeFigure) {
		addFigure(nodeFigure, NODES_LAYER);
	}

	public void addConnection(IFigure connectionFigure) {
		addFigure(connectionFigure, CONNECTIONS_LAYER);
	}

	public void addSubgraph(IFigure subgraphFigrue) {
		addFigure(subgraphFigrue, SUBGRAPHS_LAYER);
	}

	public void highlightNode(IFigure nodeFigure) {
		changeFigureLayer(nodeFigure, NODES_HIGHLIGHTED_LAYER);
	}

	public void highlightConnection(IFigure connectionFigure) {
		changeFigureLayer(connectionFigure, CONNECTIONS_HIGHLIGHTED_LAYER);
	}

	public void unHighlightNode(IFigure nodeFigure) {
		changeFigureLayer(nodeFigure, NODES_LAYER);
	}

	public void unHighlightConnection(IFigure connectionFigure) {
		changeFigureLayer(connectionFigure, CONNECTIONS_LAYER);
	}

	private void changeFigureLayer(IFigure figure, Object newLayer) {
		List<IFigure> decorations = getDecorations(figure);
		remove(figure);

		addFigure(figure, newLayer);
		for (IFigure decoration : decorations) {
			addDecoration(figure, decoration);
		}

		this.invalidate();
		this.repaint();
	}

	private List<IFigure> getDecorations(IFigure figure) {
		List<IFigure> result = new ArrayList<>();
		int index = getPosition(figure);
		if (index == -1) {
			return result;
		}
		IFigure layer = layerFigures.get(figure);
		for (index++; index < layer.getChildren().size(); index++) {
			IFigure nextFigure = layer.getChildren().get(index);
			if (!decoratingFigures.contains(nextFigure)) {
				break;
			}
			result.add(nextFigure);
		}
		return result;
	}

	/**
	 * @param child
	 * @return position of the figure in its layer
	 */
	private int getPosition(IFigure child) {
		IFigure layer = layerFigures.get(child);
		if (layer == null) {
			return -1;
		}
		return layer.getChildren().indexOf(child);
	}

	public void addFigure(IFigure figure, Object layer) {
		IFigure layerFigure = getLayer(layer);
		layerFigures.put(figure, layerFigure);
		layerFigure.add(figure);
	}

	@Override
	public void remove(IFigure child) {
		int position = getPosition(child);
		if (position == -1) {
			return;
		}
		layerFigures.remove(child);
		if (decoratingFigures.contains(child)) {
			decoratingFigures.remove(child);
			super.remove(child);
		} else {
			List<IFigure> decorations = getDecorations(child);
			super.remove(child);
			for (IFigure decoration : decorations) {
				remove(decoration);
			}
		}
	}

	public void addDecoration(IFigure decorated, IFigure decorating) {
		int position = getPosition(decorated);
		if (position == -1) {
			throw new RuntimeException("Can't add decoration for a figuer that is not on this ZestRootLayer"); //$NON-NLS-1$
		}
		do {
			position++;
		} while (position < getChildren().size() && decoratingFigures.contains(getChildren().get(position)));
		decoratingFigures.add(decorating);
		add(decorating, position);
	}
}

If you run the example, you'll see that the graph items are show, but the layout is not applied. This is because the sub-layers containing the figures don't have a layout manager on their own and thus the constraints containing their now coordinates are lost. However, adding a layout manager causes an infinite update loop which I have not yet been able to determine the cause of.

image

@BauePhil
Copy link
Contributor Author

BauePhil commented Aug 22, 2024

That's almost exactly what I did as well. It is definitely a cleaner implementation, but I just cannot get this to work properly even with FreeFormFigures.
So I leave you with the PR for the simple "fix" #532. You can decide whether to merge that for now or not. I will be using that locally until a better solution is available for me. (edit: can just overwrite the addNode. addConnection, etc. methods with a different order.
Nevertheless, using layers could still be interesting in the future, I guess we leave this issue open for now?

Copy link

This issue is stale because it has been open for 90 days with no activity.

@github-actions github-actions bot added the stale label Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants