Wynntils

Wynntils

611k Downloads

/wynntils reload needs some rethinking

magicus opened this issue ยท 11 comments

commented

It is unclear what it does and what it should do. After the webapi refactoring, this will become more apparent.

commented

/wynntils reload was added in the comparatively early pr of`db5f133 by me, which from now I think was a way for the user to signal that setting up an account had failed and should be rerun. This has generalized into the idea of reloading as much as possible about the mod if desired by the user, in the cases of something failing.

commented

Does any feature do any setup that needs reloading? They mostly just respond to events. The only things that do setup is managers, but I am not sure if any of those do anything except resetting

commented

I can think of two potentially useful commands:

  1. clear caches, and
  2. retry login to Athena+Hades, in case it has failed

I think we need to add a bit of scaffolding to be able to gracefully clear caches and redownload data. But it might be worth adding.

commented

This could also clear the mod update folder since it could be bugged and have problems.

commented

So how do we want this to work? Let's focus on splashes, they are simple. Let's say we have downloaded the splashes.json and stored it in the cache directory, and also read and parsed that json and stored the splashes in memory.

Now the user does /wynntils reload.

What should happen?

  1. We delete the cached file
  2. We try download the file anew from the net
  3. We delete/empty the internal java structure (List, in this case)
  4. We replace the internal structure when we have parsed a new one
  5. We notify users of the structure that it has changed

These are not individual steps, to be taken, one after each other. These are binary questions with a yes/no answer. Like, we can chose to delete the file, then re-download it to the same place, then delete the internal structure, and re-parse it from what we downloaded, and then send an event that it is updated, and fix so all users listen to that event. That is "yes" to all questions.

But we could also skip deleting, just try to download the file anew to a temp location, and if it fails, stick to the one we already have, and only replace it if it succeeds. Similarly, we can keep the internal data, never ever empty it, but if we succeeded in downloading a new file, and succeeded in reading it, we could replace it. User's would then have to rediscover themselves that the data has changed next time they read from it. That would be "yes" to 2 and 4, and no to the rest.

Etc. Basically all 2^5 combinations are possible; though most of them do not make sense.

I am not sure what we think we want to achieve with this. Is the use case "the cache is f*cked up beyond repair, let's just delete it" a more relevant use case than "the internet connection to service X is shaky, better keep a tight grip on that precious cache!"?

commented

Also, if "rescue us from a broken cache" is the use case, would "empty the cache dir and then exit minecraft to force a mod restart" a reasonable way to handle the case of broken caches? It would sure be simpler than trying to design the entire mod correctly from the premise that data loaded from a downloaded file might suddenly disappear, after having existed previously.

commented

I was just about to bring this up.

I think we need to split reload up.

  • wynntils clearcaches -> Clear every cache, and close the game
  • wynntils reauth -> Reauth wynntils token (and signal changes to customers, blocked by #829)

This way, we have 2 separate, but sane options about reloading. I too, don't think that we should design our data around the fact that we would reload it.

But, what if we add a wynntils feature reload MapFeature command, which would re-load MapModel, which could async recheck/redownload map data? Or just have a universal reload command for all features. Or maybe, Features could implement Reloadable, and have a method onReload. That way, we only reload what we need and we have a sane way of checking what is "dynamic" (and this way, we only show reloadable features to the user).

commented

onReload is just disable then re-enable

commented

I think splitting wynntils reload into feature reloading and clearing caches/redoing auth token is a good idea

commented

onReload is just disable then re-enable

onReload would be a way to generalize reloading, instead of splitting it weirdly up into onDisable/onEnable. At least, in my opinion (I am not sure that those two methods are really necessary even).

Let's take magicus' five steps:

We delete the cached file - no, a feature does not know about the data caches
We try download the file anew from the net - yes
We delete/empty the internal java structure (List, in this case) - yes
We replace the internal structure when we have parsed a new one - yes
We notify users of the structure that it has changed - I am not fully sure. This step sounds like, "we start using the new data". In that case, yes. But we cannot reload everything that uses the old data, like items. But we can use the new data, from now on.

If you look at these steps, I think it would be weird to split these into onEnable/onDisable.

commented

Also, with the new request code, do we need to reload Urls? From what I have seen, we don't, but correct me. Or hashes? We surely need to refresh those, don't we? But where ..? Maybe as a first step of wynntils feature reload, as the other 2 (clearcache/reauth) does not use the hash data.