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

useRegisterEvents silently modified sigma settings #49

Closed
jacomyal opened this issue Mar 23, 2023 · 1 comment
Closed

useRegisterEvents silently modified sigma settings #49

jacomyal opened this issue Mar 23, 2023 · 1 comment
Labels
bug Something isn't working

Comments

@jacomyal
Copy link
Collaborator

I have had some trouble with the following code:

if (
eventTypes.some((event) => ["clickEdge", "rightClickEdge", "doubleClickEdge", "downEdge"].includes(event)) &&
sigmaSettings.enableEdgeClickEvents === false
) {
edgeSettings["enableEdgeClickEvents"] = true;
reverseEdgeSettings["enableEdgeClickEvents"] = false;
}
if (
eventTypes.some((event) => ["enterEdge", "leaveEdge"].includes(event)) &&
sigmaSettings.enableEdgeHoverEvents === false
) {
edgeSettings["enableEdgeHoverEvents"] = true;
reverseEdgeSettings["enableEdgeHoverEvents"] = false;
}
if (eventTypes.some((event) => ["wheelEdge"].includes(event)) && sigmaSettings.enableEdgeWheelEvents === false) {
edgeSettings["enableEdgeWheelEvents"] = true;
reverseEdgeSettings["enableEdgeWheelEvents"] = false;
}
if (Object.keys(edgeSettings).length > 0) {
setSettings(edgeSettings);
}

I have no problem with higher order libraries and/or wrappers that create new abstractions or syntactic sugar on top of existing libraries. This is what react-sigma does on many sides, that makes me happily use it in my React applications.

However, on that very decision, I think it's a design mistake: react-sigma provides both useSetSettings to manage sigma's settings, and useRegisterEvents to handle events, and as a developer, I do not expect useRegisterEvents to affect settings.

I understand this decision is made because you thought it makes no sense to listen to edge events while having edge interactions disabled in the settings. Here are some precise issues I have with this situation:

  1. I could use the settings to toggle edge interaction in an app, without wanting to change my event listeners. Sigma allows it, so I would expect react-sigma to work the same way, and it would be hard to find where the issue is (this is a similar situation to what led me to write this ticket in the first place)
  2. The way it's done allows many bugs to occur. One example:
    1. I bind a listener on edge events while settings are set to false
    2. I set the settings to true right after
    3. I unbind the events later
    4. At this point, the settings are back to false, while my code set them to true at some point (and I wouldn't expect them to be different)
  3. Same issue, with specifically enableEdgeHoverEvents set to "debounce"

I see 3 main ways to fix this issue:

  1. If you think it makes no sense to allow binding listeners to edge events while edge interaction settings are false, maybe sigma is wrong and we could address that directly in sigma
  2. If you still want to prevent your users from falling into that issue, you could clarify the situation, by creating a new react-sigma concept, that would take care both of sigma events and settings (useEdgeEvents, for example).
  3. Finally, you could want to keep the APIs as close to sigma's as possible, but still educate your users on these kind of potential mistakes. For instance, you could use console.warn when some code tries to listen to edge events while the edge settings are false
@jacomyal jacomyal added the bug Something isn't working label Mar 23, 2023
@sim51
Copy link
Owner

sim51 commented Apr 3, 2024

Fixed in v4.0

@sim51 sim51 closed this as completed Apr 3, 2024
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

No branches or pull requests

2 participants