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.

Sign up for GitHub

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

Jump to bottom

Optimize NestedPackages by deferring sorting #513

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

Optimize NestedPackages by deferring sorting #513

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c34f422
optimize NestedPackages sorting
BasiqueEvangelist Jun 15, 2023
e074049
address reviews
BasiqueEvangelist Jun 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 15 additions & 31 deletions enigma-swing/src/main/java/cuchaz/enigma/gui/NestedPackages.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,21 @@

import java.util.Collection;
import java.util.Comparator;
import java.util.Enumeration;
import java.util.HashMap;
import java.util.Map;

import javax.swing.tree.DefaultMutableTreeNode;
import javax.swing.tree.MutableTreeNode;
import javax.swing.tree.TreeNode;
import javax.swing.tree.TreePath;

import cuchaz.enigma.gui.node.ClassSelectorClassNode;
import cuchaz.enigma.gui.node.ClassSelectorPackageNode;
import cuchaz.enigma.gui.node.SortedMutableTreeNode;
import cuchaz.enigma.translation.mapping.EntryRemapper;
import cuchaz.enigma.translation.representation.entry.ClassEntry;

public class NestedPackages {
private final DefaultMutableTreeNode root = new DefaultMutableTreeNode();
private final Map<String, DefaultMutableTreeNode> packageToNode = new HashMap<>();
private final SortedMutableTreeNode root;
private final Map<String, SortedMutableTreeNode> packageToNode = new HashMap<>();
private final Map<ClassEntry, ClassSelectorClassNode> classToNode = new HashMap<>();
private final EntryRemapper remapper;
private final Comparator<TreeNode> comparator;
Expand All @@ -42,6 +40,7 @@ public NestedPackages(Iterable<ClassEntry> entries, Comparator<ClassEntry> entry

return 0;
};
this.root = new SortedMutableTreeNode(comparator);

for (ClassEntry entry : entries) {
addEntry(entry);
Expand All @@ -52,31 +51,31 @@ public void addEntry(ClassEntry entry) {
ClassEntry translated = remapper.deobfuscate(entry);
var me = new ClassSelectorClassNode(entry, translated);
classToNode.put(entry, me);
insert(getPackage(translated.getPackageName()), me);
getPackage(translated.getPackageName()).insert(me, 0);
}

public DefaultMutableTreeNode getPackage(String packageName) {
DefaultMutableTreeNode node = packageToNode.get(packageName);
public SortedMutableTreeNode getPackage(String packageName) {
SortedMutableTreeNode node = packageToNode.get(packageName);

if (packageName == null) {
return root;
}

if (node == null) {
node = new ClassSelectorPackageNode(packageName);
insert(getPackage(ClassEntry.getParentPackage(packageName)), node);
node = new ClassSelectorPackageNode(this.comparator, packageName);
getPackage(ClassEntry.getParentPackage(packageName)).insert(node, 0);
packageToNode.put(packageName, node);
}

return node;
}

public DefaultMutableTreeNode getRoot() {
public SortedMutableTreeNode getRoot() {
return root;
}

public TreePath getPackagePath(String packageName) {
DefaultMutableTreeNode node = packageToNode.getOrDefault(packageName, root);
SortedMutableTreeNode node = packageToNode.getOrDefault(packageName, root);
return new TreePath(node.getPath());
}

Expand All @@ -90,11 +89,11 @@ public void removeClassNode(ClassEntry entry) {
if (node != null) {
node.removeFromParent();
// remove dangling packages
DefaultMutableTreeNode packageNode = packageToNode.get(entry.getPackageName());
SortedMutableTreeNode packageNode = packageToNode.get(entry.getPackageName());

while (packageNode != null && packageNode.getChildCount() == 0) {
DefaultMutableTreeNode theNode = packageNode;
packageNode = (DefaultMutableTreeNode) packageNode.getParent();
SortedMutableTreeNode theNode = packageNode;
packageNode = (SortedMutableTreeNode) packageNode.getParent();
theNode.removeFromParent();

if (theNode instanceof ClassSelectorPackageNode pn) {
Expand All @@ -104,22 +103,7 @@ public void removeClassNode(ClassEntry entry) {
}
}

public Collection<DefaultMutableTreeNode> getPackageNodes() {
public Collection<SortedMutableTreeNode> getPackageNodes() {
return packageToNode.values();
}

private void insert(DefaultMutableTreeNode parent, MutableTreeNode child) {
int index = 0;
Enumeration<TreeNode> children = parent.children();

while (children.hasMoreElements()) {
if (comparator.compare(children.nextElement(), child) < 0) {
index++;
} else {
break;
}
}

parent.insert(child, index);
}
}
10 changes: 7 additions & 3 deletions enigma-swing/src/main/java/cuchaz/enigma/gui/node/ClassSelectorPackageNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,18 @@

package cuchaz.enigma.gui.node;

import javax.swing.tree.DefaultMutableTreeNode;
import java.util.Comparator;

import javax.swing.tree.TreeNode;

import cuchaz.enigma.translation.representation.entry.ClassEntry;

public class ClassSelectorPackageNode extends DefaultMutableTreeNode {
public class ClassSelectorPackageNode extends SortedMutableTreeNode {
private String packageName;

public ClassSelectorPackageNode(String packageName) {
public ClassSelectorPackageNode(Comparator<TreeNode> comparator, String packageName) {
super(comparator);

this.packageName = packageName != null ? packageName : "(none)";
}

Expand Down
108 changes: 108 additions & 0 deletions enigma-swing/src/main/java/cuchaz/enigma/gui/node/SortedMutableTreeNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
package cuchaz.enigma.gui.node;

import java.util.ArrayList;
import java.util.Comparator;
import java.util.Enumeration;
import java.util.Iterator;
import java.util.List;

import javax.swing.tree.DefaultMutableTreeNode;
import javax.swing.tree.MutableTreeNode;
import javax.swing.tree.TreeNode;

import com.google.common.collect.Iterables;

/**
* A MutableTreeNode whose contents are always guaranteed to be sorted with the given comparator.
*/
public class SortedMutableTreeNode extends DefaultMutableTreeNode {
private final Comparator<TreeNode> comparator;
private final List<TreeNode> children;
private boolean isSorted = true;

public SortedMutableTreeNode(Comparator<TreeNode> comparator) {
this.comparator = comparator;
this.children = new ArrayList<>();
}

@Override
public void insert(MutableTreeNode child, int index) {
if (child == null) {
throw new IllegalArgumentException("child is null");
}

MutableTreeNode oldParent = (MutableTreeNode) child.getParent();

if (oldParent != null) {
oldParent.remove(child);
}

child.setParent(this);
this.children.add(child);
this.isSorted = false;
}

private void checkSorted() {
if (!this.isSorted) {
this.isSorted = true;
this.children.sort(this.comparator);
}
}

@Override
public void remove(int index) {
checkSorted();

remove((MutableTreeNode) getChildAt(index));
}

@Override
public void remove(MutableTreeNode node) {
this.children.remove(node);
node.setParent(null);
}

@Override
public TreeNode getChildAt(int childIndex) {
checkSorted();

return this.children.get(childIndex);
}

@Override
public int getChildCount() {
return this.children.size();
}

@Override
public int getIndex(TreeNode node) {
return Iterables.indexOf(this.children, other -> this.comparator.compare(node, other) == 0);
Copy link

Choose a reason for hiding this comment

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

Shouldn't this call checkSorted() as well, since the order of the node in the list before and after sorting may be different? (Or, if this is intended, perhaps a comment is in order.)

Also, why use Iterables.indexOf instead of List.indexOf? If the comparator is known to be consistent with equals (and the javadoc for Comparator recommends that comparators used with sorted collections be consistent with equals), then both ways should be equivalent. (Theoretically, List.indexOf would be faster by virtue of the List implementation taking advantage of impl. details.)

Copy link
Author

Choose a reason for hiding this comment

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

Can't the ClassEntry comparator be inconsistent with equals, though?
In any case, I'm probably just going to turn this into a manual for loop.

Copy link

Choose a reason for hiding this comment

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

As far as I can tell, the ClassEntry comparators -- defined in ClassSelector#DEOBF_CLASS_COMPARATOR and used in DeobfPanel#<init>, and defined as a lambda in ObfPanel#<init> -- are both consistent with equals.

ClassEntry#equals checks if the parent and class name of the entry are equivalent; the comparators above compare using the full name of the class1, which is derived from the class name of the entry and of its parent. Because of that, if two ClassEntrys are equal in terms of equals, the comparators will also declare them equivalent (returning 0).

Though, that is the present state of the code; the future may possibly bring comparators inconsistent with equals, but I think that's improbable (and would already cause problems anyway, because then you'd have entries that don't equal themselves or are equal to other entries according to the comparator).

Footnotes

  1. In the ObfPanel comparator's case which compares the lengths of the full names first: if the two full names are equivalent, then necessarily the lengths are also the same. ↩

}

@Override
public boolean getAllowsChildren() {
return true;
}

@Override
public boolean isLeaf() {
return this.children.isEmpty();
}

@Override
public Enumeration<TreeNode> children() {
Iterator<TreeNode> iter = this.children.iterator();
BasiqueEvangelist marked this conversation as resolved.
Show resolved Hide resolved

return new Enumeration<>() {
@Override
public boolean hasMoreElements() {
return iter.hasNext();
}

@Override
public TreeNode nextElement() {
return iter.next();
}
};
}
}