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 package rename #549

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix package rename #549

wants to merge 2 commits into from

Conversation

Floweynt
Copy link

@Floweynt Floweynt commented Dec 5, 2024

Fixed package rename:

  • Breaking with nested packages
  • Breaking the tree view
  • Failing to remap some classes

Fixed package rename:
- Breaking with nested packages
- Breaking the tree view
- Failing to remap some classes
public void onRenameFromClassTree(ValidationContext vc, String targetName, DefaultMutableTreeNode node) {
List<EntryChange<ClassEntry>> task = new ArrayList<>();
onRenameFromClassTree(targetName, node, task);
this.controller.applyChanges(vc, task);
Copy link
Collaborator

@2xsaiko 2xsaiko Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
this.controller.applyChanges(vc, task);
task.forEach(c -> this.controller.validateChange(vc, task));
if (!vc.canProceed()) {
return;
}
task.forEach(c -> this.controller.applyChange(vc, task));

This is better because now all the changes will be validated and it won't bail out at the first bad one (the iterator in applyChanges you added doesn't validate the rest after one fails) and also won't partially apply a bad change set. This is how this should be used, validate everything first, then actually apply, and they are deliberately separate steps for batch operations like this.

You don't need to change anything about GuiController.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fixed.

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