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

Symbol Change (right click->Change Symbol) #286

Merged
merged 2 commits into from
Sep 4, 2023

Conversation

teadrinker
Copy link
Contributor

Implementation info, steps (for each selected SymbolChild):
(1) Create a new SymbolChild
(2) Loop through in/out and match connections/data by name and type and copy/connect to newly created
(3) If conversion was lossless, remove original SymbolChild
(4) Select new SymbolChild

For UI, I modified the SymbolBrowser to be able to return the target Symbol in a callback instead of creating it directly

Reason for this PR:
I felt it was slow and error prone to manually reproduce properties and connections when switching between different versions or implementations of operators (for instance when using "Duplicate as new type..." to edit a built-in operator)

(I have almost no experience with the codebase so I probably missed some stuff, or messed up some stuff!)

@pixtur
Copy link
Collaborator

pixtur commented Sep 4, 2023

Wow, that looks like an incredible PR! The only thing that pops into my mind is the handling of the Undo-Stack. Did you try to undo the replacement? From looking at the code, it would probably take multiple undo-commands which will lead to an inconsistent state.

There are two things that could be changed ideally:

  1. You could simply collect the commands and then create and add a MacroCommand to the UndoStack.
  2. You could insert the replace operator into the ConnectionMaker class. This implements "operations" of which the replacement could be one.

Let me know what you think. I could also take over from here.

@pixtur pixtur merged commit db02024 into tooll3:master Sep 4, 2023
@pixtur
Copy link
Collaborator

pixtur commented Sep 4, 2023

Ups. Merged by accident. Sorry for the confusion.
I noticed that you did your changes on the master branch. Would it be possible to create a PR (or change this one) to dev?

@teadrinker
Copy link
Contributor Author

Ah, yeah you're right, the multi-step undo would be confusing for the user!

I have updated the branch (teadrinker:feat/change-symbol) so that it uses MacroCommand. I also merged in dev.

@teadrinker
Copy link
Contributor Author

(should I create a new PR since this is closed btw?)

@pixtur
Copy link
Collaborator

pixtur commented Sep 10, 2023

I noticed you already did! Thanks a lot!

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.

2 participants