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

onModelChange not triggered unless UndoManager.isEnabled #2

Closed
JonatanGarciaClavo opened this issue Dec 16, 2019 · 11 comments
Closed
Labels
intended behavior an issue that is describing correct behavior

Comments

@JonatanGarciaClavo
Copy link

Hi.

I have a few different diagrams (Layered, Force, Radial, Sankey, etc) and I wanted to migrate from react-gojs to your official version. First thing I notice it is new addition of the toIncrementalData, this method is called in the onModelChange. So far so good, until we want to disable undoManager. Then onModelChange it is not triggered anymore.

https://codesandbox.io/s/interesting-gauss-9pofo

In that url you could find a modification of your example(https://github.com/NorthwoodsSoftware/gojs-react-basic). The only 2 things I modified from the original it is:
Added console.log in the handleModelChange in App.tsx
Changed "undoManager.isEnabled": false in Diagram.tsx

What I detected is in const dataChanges = e.model!.toIncrementalData(e); in Diagram file it is always null with this options to false, meanwhile if you put to true is working as expected.

I report it as issues because I expected this is called no matter if undoManager is or not enable, but maybe I missing something.

Regards.

@WalterNorthwoods WalterNorthwoods changed the title onModelChange is not triggered onModelChange not triggered unless UndoManager.isEnabled Dec 16, 2019
@WalterNorthwoods
Copy link
Contributor

Yes, that is true. There needs to be some recording of all changes so that it is possible, once per transaction, to write out all of the changes. This is much more efficient than handling every ChangedEvent as it happens. Naturally the UndoManager would normally be doing this when it isEnabled, so it seemed unreasonable to reproduce all of that mechanism in some other manner.

If you do not want the UndoManager to retain a history of Transactions, set UndoManager.maxHistoryLength to zero. Because there is no Transaction history, users will be unable to undo or redo.

@JonatanGarciaClavo
Copy link
Author

Oh, nice trick, thanks. Maybe could be nice to add it in the docs, because I wasn't able to think about this combination of flags to make it work as expected.

Anyway, thanks for quick response and help provided.

@jonchardy
Copy link
Contributor

Yes, we'll be adding this to the docs here and on the intro page at our website.

@WalterNorthwoods WalterNorthwoods added the intended behavior an issue that is describing correct behavior label Dec 22, 2019
@JonatanGarciaClavo
Copy link
Author

JonatanGarciaClavo commented Sep 10, 2020

@WalterNorthwoods @jonchardy I was upgrading my project to the latest version of gojs and react-gojs and then I found a new problem, now I get a model update the first time the model is initialised, which seems a bit annoying because it isn't model change.
I just forked same example of the last time, but this time I added the flags according to the latest feedback(the once that worked for me too). But the result is the same as the first time I reported this issue.
https://codesandbox.io/s/falling-voice-e4veu?file=/src/components/Diagram.tsx
Not sure, if again I miss any new flag or if there is something that changed since the last time.

Regards.

@jonchardy
Copy link
Contributor

Yes, that was a change made in gojs-react 1.0.7. https://github.com/NorthwoodsSoftware/gojs-react/blob/master/CHANGELOG.md#107

We made that change so any changes that happen during initialization, for example layout of nodes/links, notify the model change listener.

@JonatanGarciaClavo
Copy link
Author

Well that change doesn't affect together with gojs version 2.1.0 so I am trying to see which version of gojs make it fail. I am afraid isn't related to react-gojs is related to gojs version. I already tested from 2.1.0 to 2.1.10 and all works as expected. I am close to find the failing one, once I know it I will let you know

@JonatanGarciaClavo
Copy link
Author

JonatanGarciaClavo commented Sep 10, 2020

@jonchardy Ok if you check same example with react-gojs version 1.0.8 and gojs version 2.1.15 works as expected meanwhile with gojs version 2.1.16 or higher I get the console log even if model data doesn't change.
You can try different dependencies in codesandbox, they have a selector with the version that you want to install it and then I refresh the condesandbox browser to be sure it get all updated packages. I wait until I see the log(console tab), if it appear for me means something call the on change model callback otherwise it is what I expected.
Let me know if I can help you with something else.

@JonatanGarciaClavo
Copy link
Author

https://codesandbox.io/s/charming-clarke-nxyn1?file=/src/App.tsx with gojs version 2.1.15 aka working
https://codesandbox.io/s/funny-galois-6z19c?file=/src/App.tsx with gojs version 2.1.16 aka not working

@jonchardy
Copy link
Contributor

jonchardy commented Sep 10, 2020

As I described earlier, that was an intentional change that we made. The model changes from being empty to being initialized with data, so the change handler is triggered. You can handle the change however you choose, including ignoring it if you'd like.

If you imagine a scenario using an automatic layout, that would trigger a model change during initialization, so you'd want to make sure that was propagated out to your app. Even in your case without any side effects during initialization, the model IS changing from an empty one to one with nodes and links.

There is no bug here, just a behavior change that you may need to account for. Even in your example, everything works perfectly despite the console message.

@JonatanGarciaClavo
Copy link
Author

Agree that all works as expected except the first initialisation, to us that method means changes into the model produced by gojs not by our internal state, now not sure how we can keep track about what things are really a gojs model change and what things are considered an internal state.

@jonchardy
Copy link
Contributor

jonchardy commented Sep 10, 2020

It shouldn't matter as long as you're properly merging the changes into your state.

If you really don't want to update state after initialization, just set some property/field (diagramInitialized, or something) that gets set to true after the first call to handleModelChange. That way you can exit handleModelChange if that property is false. This may cause problems if you ever introduce side effects into the diagram initialization, though (like I've described with layouts).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
intended behavior an issue that is describing correct behavior
Projects
None yet
Development

No branches or pull requests

3 participants