Implement registry removal check #3258
Conversation
apple502j
added
enhancement
|
|
||
| public final class RegistryRemovalChecker { | ||
| private static final int VERSION = 1; | ||
| public static final Logger LOGGER = LoggerFactory.getLogger(RegistryRemovalChecker.class); |
There was a problem hiding this comment.
Generally we try to keep loggers private?
| public static final Logger LOGGER = LoggerFactory.getLogger(RegistryRemovalChecker.class); | ||
| private static final Gson GSON = new Gson(); | ||
| public static final String FILE_NAME = "fabric_registry_removal_check.json"; | ||
| private static final boolean DISABLED = Boolean.getBoolean("fabric.registry.debug.disableRemovalCheck"); |
There was a problem hiding this comment.
Move this field to the top, a small comment might be helpful to make it more visable?
| Path jsonFile = session.getDirectory(WorldSavePath.ROOT).resolve(RegistryRemovalChecker.FILE_NAME); | ||
| RegistryRemovalChecker.write(jsonFile); |
There was a problem hiding this comment.
We should backup the old file, maybe store 5 or so previous files. This will match the previous reg sync behaviour.
| RegistryRemovalChecker checker = RegistryRemovalChecker.runCheck(jsonFile); | ||
|
|
||
| if (checker == null || checker.getMissingNamespaces().isEmpty()) { | ||
| RegistryRemovalChecker.write(jsonFile); |
There was a problem hiding this comment.
Maybe I need the code in front of me, but I dont see where this is saved after a user backups and loads the world.
|
|
||
| @SuppressWarnings("unchecked, rawtypes") | ||
| public RegistryRemovalChecker(JsonObject root) { | ||
| JsonObject json = root.getAsJsonObject("entries"); |
There was a problem hiding this comment.
This never checks the version, it should handle the case of the version being newer than supported. I imagine telling the user this.
| JsonObject json = serializeRegistries(); | ||
| Files.writeString(jsonFile, GSON.toJson(json), StandardCharsets.UTF_8); | ||
| } catch (IOException e) { | ||
| LOGGER.warn("Could not write {}", FILE_NAME, e); |
There was a problem hiding this comment.
Let it crash, something has gone badly wrong if this fails to write.
| import net.minecraft.client.gui.screen.Screen; | ||
| import net.minecraft.text.Text; | ||
|
|
||
| public final class RemovedRegistryEntryWarningScreen extends BackupPromptScreen { |
There was a problem hiding this comment.
Might be nice to include a screenshot in the PR.
| import net.fabricmc.fabric.api.event.registry.RegistryAttribute; | ||
| import net.fabricmc.fabric.api.event.registry.RegistryAttributeHolder; | ||
|
|
||
| public final class RegistryRemovalChecker { |
There was a problem hiding this comment.
Some unit tests would be great for this.
| for (Map.Entry<String, JsonElement> registry : json.entrySet()) { | ||
| Identifier registryId = Identifier.tryParse(registry.getKey()); | ||
|
|
||
| if (registryId == null) continue; |
There was a problem hiding this comment.
If there is an entry it doesnt understand (maybe from a future version) this warrents showing the backup screen IMO.
| return; | ||
| } | ||
|
|
||
| RegistryRemovalChecker.LOGGER.warn("Registry removal check failed! This often occurs when you update or remove a mod."); |
There was a problem hiding this comment.
Maybe out of scope for this PR, but it could check the ServerBrands entry in the level.dat and show a warning. This way it could warn if a user tries to load a Forge world for example.
WIP. Will need to test with a better equipment. Thanks to @LambdAurora for the proposal!