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

Fix cloning pinned nodes #364

Merged
merged 1 commit into from
Dec 2, 2024
Merged

Fix cloning pinned nodes #364

merged 1 commit into from
Dec 2, 2024

Conversation

pythongosssss
Copy link
Member

Pin node
right click -> clone -> crash
or copy/paste -> crash

@webfiltered
Copy link
Collaborator

Hrm, I don't think pin itself is the issue - more that it shouldn't be getting called at all on any node that doesn't have a valid graph. Having a look.

@pythongosssss
Copy link
Member Author

It's being called from LGraphNode.configure which also has the check on graph.
Nodes are configured then added to the graph, so graph wont be set at this point.

@webfiltered
Copy link
Collaborator

Yeah - just tested this:

    this.resizable = !this.pinned

Works perfectly.

@webfiltered
Copy link
Collaborator

Amusingly, the preceding comment was better than the code.

// Sync the state of this.resizable.

@pythongosssss
Copy link
Member Author

If in future pin were to have some additional logic, e.g. pin to the top of the stack, would it not be better to have the logic for pin in the pin method, rather than duplicated?

@webfiltered
Copy link
Collaborator

webfiltered commented Dec 1, 2024

It would be better to actually serialise and deserialise any additional *state, no?

@pythongosssss
Copy link
Member Author

I don't think so, as if new functionality is added to pin that shouldn't rely on the state of the serialized graph

@webfiltered
Copy link
Collaborator

Is that a good reason to have the code work that way right now? The code should really just.. do what it's designed to do. afaik it was never designed to call pin this way - I believe pin got changed, and this call site didn't.

But I just spent 20mins trying to ensure I didn't come across as argumentative. So if you're set on this, I won't butt in further! =P

@huchenlei huchenlei merged commit a0b50dc into master Dec 2, 2024
4 checks passed
@huchenlei huchenlei deleted the fix-clone-pinned branch December 2, 2024 01:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants