Skip to content

Conversation

@mayorovad
Copy link

@mayorovad mayorovad commented Oct 3, 2022

Summary
This is fix for #114. There is logical error in processing ChildChange in Graph. When cell is removed, ChildChange created to change parent of removed cell to null. But in code, if parent is null, code to destroy child cell state is not called, so cell stays rendered on canvas.

else if (change instanceof ChildChange) {
const newParent = change.child.getParent();
this.view.invalidate(change.child, true, true);
if (
newParent &&
(!this.getDataModel().contains(newParent) || newParent.isCollapsed())
) {
this.view.invalidate(change.child, true, true);
this.removeStateForCell(change.child);

Added check for "falsy" parent so correct branch of code will be executed and cells will be destroyed.

Description for the changelog
Fixed bug with cells not removing when calling Graph.removeCells()

closes #114

@tbouffard tbouffard added the bug Something isn't working label Oct 7, 2022
@tbouffard tbouffard changed the title Fix for cells not removing #114 fix: Fix for cells not removed Nov 20, 2022
@mcyph
Copy link
Collaborator

mcyph commented Nov 22, 2022

I think I have some idea what's happened here.

Previously, many functions were quite lenient in what they accepted, and would allow for null/undefined values to be passed and return. I know I tried to reduce the number of times functions simply returned when passed null or undefined values to reduce the risk of errors happening silently, but this also may have introduced unintentional side effects.

I'm not sure whether it was me or someone else in this instance, but that newParent check was not in the original code of mxGraph, so it may be it was added to silence TypeScript warnings after making other functions less lenient in the parameters they accepted:

...
	// Adds or removes a child to the view by online invaliding
	// the minimal required portions of the cache, namely, the
	// old and new parent and the child.
	else if (change instanceof mxChildChange)
	{
		var newParent = this.model.getParent(change.child);
		this.view.invalidate(change.child, true, true);
		
		if (!this.model.contains(newParent) || this.isCellCollapsed(newParent))
		{
			this.view.invalidate(change.child, true, true);
			this.removeStateForCell(change.child);
			
			// Handles special case of current root of view being removed
			if (this.view.currentRoot == change.child)
			{
				this.home();
			}
		}
...

If you look into the contains method in mxGraphModel it calls isAncestor with the root as the parent, and the child as the newParent. If newParent is null or undefined, it will immediately return false in the original code if the parent is also not null or undefined:

/**
 * Function: isAncestor
 * 
 * Returns true if the given parent is an ancestor of the given child. Note 
 * returns true if child == parent.
 *
 * Parameters:
 * 
 * parent - <mxCell> that specifies the parent.
 * child - <mxCell> that specifies the child.
 */
mxGraphModel.prototype.isAncestor = function(parent, child)
{
	while (child != null && child != parent)
	{
		child = this.getParent(child);
	}
	
	return child == parent;
};

I think this patch may correct this, as it seems to mirror the original behaviour of mxGraph.

Copy link
Member

@tbouffard tbouffard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mayorovad for this new contribution ❤️
✔️ @mcyph did a formal investigation to validate the implementation logic
✔️ Tested with the DynamicLoading story

development branch PR 115
dynamic_loading_01_development dynamic_loading_02_PR_115

@tbouffard tbouffard merged commit a3b5a60 into maxGraph:development Dec 8, 2022
@mayorovad mayorovad deleted the remove-cells-fix branch December 12, 2022 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cells are still rendering after removing using removeCells method of Graph

3 participants