Skip to content

fix: add null check to connection validation message #364

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.

Sign up for GitHub

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

Jump to bottom
Merged
tbouffard merged 4 commits into from
Apr 19, 2024
Merged

fix: add null check to connection validation message #364

tbouffard merged 4 commits into from
Apr 19, 2024

Conversation

@eRedekopp
Copy link

@eRedekopp eRedekopp commented Mar 19, 2024 •
edited by tbouffard
Loading

Summary
If-statement in ConnectionHandlerCellMarker was looking for this.connectionHandler.error to be an empty string, but used check this.connectionHandler.error && this.connectionHandler.error.length === 0, which is false when the string is empty because "" is falsy. Reverted to the syntax used in mxgraph. This was noticed by issue #362

Description for the changelog
Add null check to connection validation message in ConnectionHandlerCellMarker to handler invalid connection targets.

Closes #362

@eRedekopp
Copy link
Author

eRedekopp commented Mar 19, 2024

This specifically needs to check for an empty string because validateConnection returns an empty string to indicate that the connection target was invalid. Do you think it might make more sense to indicate this in a more robust way, like maybe returning a specific message indicating that? For example, what if getEdgeValidationErrors returns an empty string as an error message -- is that something we need to worry about?

@eRedekopp eRedekopp force-pushed the development branch 4 times, most recently from caf50de to 073dbd1 Compare March 19, 2024 22:44
@tbouffard tbouffard added the bug Something isn't working label Mar 21, 2024
@tbouffard
Copy link
Member

tbouffard commented Apr 4, 2024

Sorry for the late answer, I don't have time to review this PR for now.
I need some time to figure out what is the context. I hope to come back to you soon with insights.

@tbouffard
Copy link
Member

tbouffard commented Apr 18, 2024 •
edited
Loading

Apparently, the Validation story reproduces the original problem (see #73), so it should work with this fix.

tbouffard
tbouffard approved these changes Apr 18, 2024
View reviewed changes
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.

I have not been able to reproduce the problem with the Validation story. The null check in Multiplicity.checkTerminal remove errors in the Story.
As the proposed fix restores the mxGraph implementation, this is OK for now. Improvements of the error management can be done later.

Notice that the Validation story shows that the error message are not correctly display. A "key" is used instead of the actual error message. Fixing this problem was out of the scope of this PR.

Here is how the Validation story behaves with this PR 👇🏿

PR_364_Validation_story

@tbouffard tbouffard merged commit 12d3447 into maxGraph:main Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tbouffard tbouffard tbouffard approved these changes

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

The performance of the getCell method of the CollectionHandler class is different from that of the old version after migration.

2 participants

@eRedekopp @tbouffard