Entry Changes #364

2xsaiko · 2021-03-25T19:35:17Z

Makes the concept of changes on a single mapping a single unit (an EntryChange<E>), instead of handling javadoc, deobf, etc. changes separately on both the internal and network layer. Also, EntryMapping no longer has to have a deobfuscated name attached, allowing an unobfuscated entry to have Javadocs.

This changes the file format of Enigma: "-" is now a valid deobfuscated name in the file format for all entries that is read as the entry being read having no name. This allows access modifiers to be saved without a deobfuscated name on the entry.
Example:

(currently valid in mainline Enigma)

CLASS net/minecraft/class_377 net/minecraft/client/font/FontStorage ACC:PROTECTED

(added by this PR)

CLASS net/minecraft/class_377 - ACC:PROTECTED

The following would also be read correctly but isn't saved due to being redundant (an entry defined by just its obfuscated name is still valid as before):

CLASS net/minecraft/class_377 -

Files saved that have this changed syntax in them (this should only happen if someone changes the access modifiers) can obviously not be read by older Enigma versions, but older mapping files can be read as is.

Changes:

Changes on an Entry mapping are described as a single patch unit (EntryChange<E>)
Entry mappings can exist without a mapped name, allowing an entry to be unmapped but have Javadoc attached
As a result, entry mappings can be empty
Empty entry mappings (EntryMapping.DEFAULT) replace null entry mappings (and everything dealing with entry mappings should no longer accept null values)
Network protocol changes to accommodate the above changes
File format changes to accommodate the above changes

…ryChange in favor of entry changing methods in GuiController

…sing name collisions with itself Closes FabricMC#246.

Closes FabricMC#314

liach · 2021-07-05T17:52:50Z

enigma-server/src/main/java/cuchaz/enigma/network/packet/PacketHelper.java

+					throw new IOException("Field requires class parent");
+				}
+				TypeDescriptor desc = new TypeDescriptor(readString(input));
+				return new FieldEntry((ClassEntry) parent, name, desc, javadocs);


can use pattern matching instanceof now

liach · 2021-07-05T17:56:52Z

enigma/src/main/java/cuchaz/enigma/newabstraction/EntryChange.java

@@ -0,0 +1,96 @@
+package cuchaz.enigma.newabstraction;


Hmm, mind choose a better package name, like cuchaz.enigma.change?

That package was temporary initially, I wanted to move everything out of it. Let me move these to cuchaz.enigma.translation.mapping

liach · 2021-07-05T17:57:56Z

enigma/src/main/java/cuchaz/enigma/newabstraction/EntryChange.java

+import cuchaz.enigma.translation.mapping.AccessModifier;
+import cuchaz.enigma.translation.representation.entry.Entry;
+
+public class EntryChange<E extends Entry<?>> {


This definitely should be a record.

Hmm, can you have a private constructor on a record? Cause I'd really hate to have the constructor public on this

rip no. In the canonical constructor you cannot even throw checked exceptions, but you can still have public static factory methods to help

now I'd have to decide between cleaner API or cleaner implementation
aaAAAAAAAA java why
tbh since I think it's unlikely that fields will be added to this class I'm inclined to leave it as is and not turn it into a record

liach · 2021-07-05T17:58:29Z

enigma/src/main/java/cuchaz/enigma/newabstraction/EntryChange.java

+		EntryChange<?> that = (EntryChange<?>) o;
+		return Objects.equals(this.target, that.target) &&
+				Objects.equals(this.deobfName, that.deobfName) &&
+				Objects.equals(this.javadoc, that.javadoc);


Why is access ignored, just curious?

I probably forgot to regenerate equals/hashcode when I added it.

Record time 😆 I don't forget about handling extra fields again

liach · 2021-07-05T18:00:35Z

enigma/src/main/java/cuchaz/enigma/translation/mapping/EntryMapping.java

 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;

-public class EntryMapping {
-	private final String targetName;
+public final class EntryMapping {


probably can be a record as well

liach · 2021-07-05T18:01:43Z

enigma/src/main/java/cuchaz/enigma/translation/mapping/serde/RawEntryMapping.java


 	public RawEntryMapping(String targetName) {
 		this(targetName, null);
 	}

 	public RawEntryMapping(String targetName, AccessModifier access) {
 		this.access = access;
-		this.targetName = targetName;
+		this.targetName = targetName != null && !targetName.equals("-") ? targetName : null;


Hmm, what is the - check for?

According to the PR description, it's used as a stand-in for a deobfuscated name in the Enigma format, to allow writing the access modifier component without a deobfuscated name.

https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.2.2
I suggest using one of . ; [ / to indicate an unchanged target name to avoid potential conflicts, preferably just .

Is that necessary? These are deobf names, "-" is not a valid Java identifier character so you can't set it from the enigma UI and if you were to edit the file manually and have it remap correctly, you can't use methods with "-" in them from Java (nor probably any other JVM language). For all intents and purposes I think this is not really an issue (and I'd say "-" is more fitting as a placeholder to denote "no value" than any of these). I could make it "[none]" or something like that but as I said I don't think it'd be a problem.

Nitpick: dashes are usable in Kotlin if you escape the name with backticks (though that's extremely cursed)

liach · 2021-07-05T18:02:16Z

enigma/src/main/java/cuchaz/enigma/translation/mapping/serde/enigma/EnigmaMappingsWriter.java

 		if (entry instanceof ClassEntry) {
-			String line = writeClass((ClassEntry) entry, mapping);
-			writer.println(indent(line, depth));
+			line = writeClass((ClassEntry) entry, mapping);


same pattern matching comment

liach · 2021-07-06T01:17:40Z

enigma/src/main/java/cuchaz/enigma/utils/TristateChange.java

+
+import java.util.Objects;
+
+public class TristateChange<T> {


Suggested change

public class TristateChange<T> {

public final class TristateChange<T> {

liach · 2021-07-06T01:21:48Z

enigma/src/main/java/cuchaz/enigma/translation/mapping/EntryMapping.java

 		this(targetName, AccessModifier.UNCHANGED, javadoc);
 	}

-	public EntryMapping(@Nonnull String targetName, AccessModifier accessModifier) {
+	public EntryMapping(@Nullable String targetName, AccessModifier accessModifier) {
 		this(targetName, accessModifier, null);
 	}



Consider adding the canonical constructor to ensure accessModifier is not null

public EntryMapping { if (accessModifier == null) { accessModifier = AccessModifier.UNCHANGED; } }

Oh, that's some neat syntax! And good point, let me add an error message too though. I did test and fixed any occurrences of this getting passed null (there was only one from what I've tested and it broke loading mappings so I noticed immediately haha), but just in case.

2xsaiko mentioned this pull request Apr 9, 2021

javafx #380

Open

liach added optimization work-in-progress labels May 15, 2021

YanisBft mentioned this pull request Jun 28, 2021

Name suggested fields with the suggested names if they have javadoc added #413

Closed

2xsaiko force-pushed the the-mega-restructure branch from aa1db5a to cd7968c Compare July 5, 2021 16:13

2xsaiko and others added 7 commits July 5, 2021 18:48

Initial refactor: Allow EntryMapping to have null targetName, add Ent…

96d6484

…ryChange in favor of entry changing methods in GuiController

Fix resetting name not actually removing it, and renaming a class cau…

fc53f65

…sing name collisions with itself Closes FabricMC#246.

Use name proposer for setting default deobf name

52a56d5

Closes FabricMC#314

Make network protocol use EntryChange directly

ad0ef2d

Handle writing other data correctly when the deobf name is null

3ba30d9

b

046d576

Add some new abstraction stuff

188a944

2xsaiko force-pushed the the-mega-restructure branch from cd7968c to 188a944 Compare July 5, 2021 16:56

2xsaiko marked this pull request as ready for review July 5, 2021 17:04

2xsaiko requested review from Gegy, modmuss50 and liach July 5, 2021 17:05

2xsaiko changed the title ~~The Mega Restructure™~~ Entry Changes Jul 5, 2021

Gegy approved these changes Jul 5, 2021

View reviewed changes

2xsaiko added mapping and removed work-in-progress labels Jul 5, 2021

modmuss50 approved these changes Jul 5, 2021

View reviewed changes

liach reviewed Jul 5, 2021

View reviewed changes

Use pattern matching instanceof

7e5085c

liach requested a review from a team July 5, 2021 18:45

2xsaiko added 4 commits July 5, 2021 20:47

Move classes out of newabstraction package

69dd77a

Make EntryChange final

b295c77

Regenerate equals and hashCode

729a81c

Convert EntryMapping to record

cc951f6

liach approved these changes Jul 6, 2021

View reviewed changes

2xsaiko added 2 commits July 6, 2021 10:44

Make TristateChange final

e8be0b0

Safety guard null accessModifier initialization

72a6a1d

2xsaiko merged commit 8efb624 into FabricMC:master Jul 8, 2021

YanisBft mentioned this pull request Jul 8, 2021

Fix entry mapping #237

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Entry Changes #364

Entry Changes #364

2xsaiko commented Mar 25, 2021 •

edited

Loading

liach Jul 5, 2021

liach Jul 5, 2021

2xsaiko Jul 5, 2021

liach Jul 5, 2021

2xsaiko Jul 5, 2021

liach Jul 5, 2021

2xsaiko Jul 5, 2021

liach Jul 5, 2021

2xsaiko Jul 5, 2021

liach Jul 5, 2021

liach Jul 5, 2021

liach Jul 5, 2021

sciwhiz12 Jul 5, 2021

liach Jul 5, 2021

2xsaiko Jul 5, 2021

Juuxel Jul 5, 2021

liach Jul 5, 2021

liach Jul 6, 2021

liach Jul 6, 2021

2xsaiko Jul 6, 2021

	public class TristateChange<T> {
	public final class TristateChange<T> {

Entry Changes #364

Entry Changes #364

Conversation

2xsaiko commented Mar 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2xsaiko commented Mar 25, 2021 •

edited

Loading