badasintended / wthit

what the hell is that?
https://docs.bai.lol/wthit
Other
122 stars 20 forks source link

Attempt to fix the blacklist config rolling back and improper backup issue. #216

Closed CrimsonEdgeHope closed 1 year ago

CrimsonEdgeHope commented 1 year ago

Previously on https://github.com/badasintended/wthit/pull/215

https://github.com/badasintended/wthit/pull/215#issuecomment-1605381094

So, it's mentioned that the plugin is supposed to be reset config when the hash value mismatch, otherwise keep using the config value and don't change. However, in Registrar enum class, after invoking addBlacklist in WailaPluginVanilla that adds values to blacklist field declared in Registrar, all the values are written to blacklist.json without further checking, right in lock method. This results in any default value that is removed from blacklist.json being re-added upon every time launching game.

(The Registrar: https://github.com/badasintended/wthit/blob/22fcc2b4224a0df185acdc11cbb263aacd81c253/src/main/java/mcp/mobius/waila/registry/Registrar.java)

Note, under my circumstance, I want to remove barrier block from blacklist.json once for all

First:

Hash the blacklist entries from plugins

int[] hash = {0, 0, 0};
hash[0] = hash(blacklist.blocks, BuiltInRegistries.BLOCK);
hash[1] = hash(blacklist.blockEntityTypes, BuiltInRegistries.BLOCK_ENTITY_TYPE);
hash[2] = hash(blacklist.entityTypes, BuiltInRegistries.ENTITY_TYPE);

When it comes to this piece of code, there are already default blacklist values added in blacklist field. I get the following hash seen in blacklist.json:

"pluginHash": [
  1510828157,
  0,
  -846312502
]

Second:

Compare the hash with the pluginHash value from the config.

if (Waila.BLACKLIST_CONFIG.isFileExists() && !Arrays.equals(Waila.BLACKLIST_CONFIG.get().pluginHash, hash)) {
    Waila.BLACKLIST_CONFIG.backup();
}

This is exactly what the if statement does.

Third:

Backup the config and reset if the hash mismatched, use the config value otherwise.

if (Waila.BLACKLIST_CONFIG.isFileExists() && !Arrays.equals(Waila.BLACKLIST_CONFIG.get().pluginHash, hash)) {
    Waila.BLACKLIST_CONFIG.backup();
}

BlacklistConfig newBlacklist = Waila.BLACKLIST_CONFIG.get();
newBlacklist.pluginHash = hash;
newBlacklist.blocks.addAll(blacklist.blocks);
newBlacklist.entityTypes.addAll(blacklist.entityTypes);
Waila.BLACKLIST_CONFIG.save();

During my manual test process, If I purposefully edit the hash value in the json to a wrong one, it's definitely going to trigger a backup operation. But this is the only thing to be done under the if statement. Right after this, all the blacklist values in blacklist field previously added by invoking addBlacklist in WailaPluginVanilla are all added without checking, including BARRIER block. As a consequence, my own setting that removes barrier block from blacklist gets reverted.

I think the hash-value mechanism is just to write a set of default values when creating the json.

Later on I make a small change in Registrar enum class, so when it comes to lock method, first it would compare the hash value. It's notable that if the json doesn't exist at first, invoking Waila.BLACKLIST_CONFIG.get() will lead to the creation of an empty blacklist.json, where all the three blacklist setting arrays are empty, and all the hashes are same 0. The ConfigIO does the work, when invoking IJsonConfig#get without the json's existence, config = factory.get() in ConfigIO#read returns an empty set of blacklist config, boolean init remains true and finally if (init) is true so an empty blacklist.json gets written. The empty blacklist.json is read, Waila.BLACKLIST_CONFIG.get() returns a new instance of BlacklistConfig, the pluginHash are all 0 which does not equal to 1510828157, 0, -846312502. if (!Arrays.equals(Waila.BLACKLIST_CONFIG.get().pluginHash, hash)) becomes true. Because the empty json gets written first, Waila.BLACKLIST_CONFIG.isFileExists() is true thus the backup operation will be triggered, leaving an empty backup. newBlacklist points to the loaded reference, adding values and invoking save will perfectly save default values.

Now using this patch I rolled out some manual tests. I can see my own settings get preserved. Easy fix!

Please see if I had messed up with anything.

CrimsonEdgeHope commented 1 year ago

emm One more thing.. Have you tried the backup mechanism? I was rolling out tests and found that the backup gets overwritten. I purposefully edited the json into a wrong format. It should backup the bad config with useful values however I see an empty one with all three hashes in 0 instead...

CrimsonEdgeHope commented 1 year ago

There is a problem invoking the backup method, sad losing my config values :(

When it comes to if (!Arrays.equals(Waila.BLACKLIST_CONFIG.get().pluginHash, hash)), inside read method in ConfigIO class, only when no IOException, all correct json format, and matching config version will set init = false and will not invoke config = factory.get();

If config = factory.get();, an empty set of blacklist config is returned.

if (init) {
    versionSetter.accept(config, currentVersion);
    write(path, config);
}

After this, an empty json will appear. Before this, a backup operation should be already done (except the scenario where json doesn't exist) unless it comes to init = false;.

That change (https://github.com/badasintended/wthit/pull/216/commits/257caa5bf886d00c61bdef1d193408d1c32f7f14) literally didn't make any change. ConfigIO#read itself checks the json's existence as well. Thus, at the end of ConfigIO#read, blacklist.json will come to existence for sure, regardless of what scenario at beginning.

CrimsonEdgeHope commented 1 year ago

No still no luck... (https://github.com/badasintended/wthit/pull/216/commits/e80c281503325383209666e148f388e0388007ea)

CrimsonEdgeHope commented 1 year ago

Try this one (https://github.com/badasintended/wthit/pull/216/commits/e4e586e0626bd04aabea890f19ed354980ef3811) it should do the trick.

CrimsonEdgeHope commented 1 year ago

I think deleting the following block is enough:

if (Waila.BLACKLIST_CONFIG.isFileExists()) {
    Waila.BLACKLIST_CONFIG.backup();
}

When the json doesn't exist, ofc no backup. But when it does exist, unless the json format is correct, all hashes and configversion match, backup shall be done only once for sure. Invoking backup another time in lock is actually backing up the empty json.

The DATE_FORMAT uses "yyyy.MM.dd.HH.mm.ss", which brings a great chance of overwritting the backup with the same file name.

CrimsonEdgeHope commented 1 year ago

image image

deirn commented 1 year ago

Hmm, this became a more complicated problem than I anticipated 😅 .

The current solution is not ideal as that would make the config lost and not backuped when the hash changes. I think it would need a marker property inside the json itself to mark whether it is empty because no blacklist from plugin or because of user. OR, just simply skip the backup if the current config hash is [0, 0, 0], as the plugin hash would never be [0, 0, 0] in runtime anyway.

deirn commented 1 year ago

I think it works now, please test it on your end.

CrimsonEdgeHope commented 1 year ago

Yeah it looks the magic works now, awesome! (Minecraft Fabric 1.20.1)