dillydill123 / inventory-setups

Plugin for RuneLite, an open source game client for the MMO Old School RuneScape
https://runelite.net/plugin-hub/dillydill123
BSD 2-Clause "Simplified" License
53 stars 30 forks source link

Use separate config keys per InventorySetup #241

Closed poi56iop closed 8 months ago

poi56iop commented 8 months ago

Also handles migration.

Fixes #206.

Let me know if there's anything I should fix or change; thanks!

poi56iop commented 8 months ago

I believe every comment has been handled.

dillydill123 commented 8 months ago

Gonna do some testing on my side. did you test importing?

dillydill123 commented 8 months ago

Also, could you fixup/squash your commits into one?

poi56iop commented 8 months ago

Gonna do some testing on my side. did you test importing?

I did not; I didn't think that workflow would be affected.

Also, could you fixup/squash your commits into one?

Done

dillydill123 commented 8 months ago

I made some comments regarding the testing I've done. Additionally, there's one problem (side effect?) of the current implementation. Setups are not in any particular order when read. Previously, setups had an inherent order that was stable and wouldn't change unless the user changed the order specifically (right click menus). Now setups have an order defined by what order the config manager fetches the keys.

I would like to preserve the "natural" order of the setups. Previously it was just the order setups were created in. Not sure the best way to do this at the moment. I had two ideas so far

You could keep track of a list which contains the hashes of the setup names in order. Given that each hash is 32 chars, you could realistically have ~250000/32 = over ~7000 setups before we hit the config line limit, at which point I'm pretty sure there will be other problems lol. This seems like the easiest approach. First read this sorted list, then find each V3 key in order and load the setups. Since we batch process everything anyways even for one singular update, it should be fine just to recreate this list order every time and update the config value.

Another solution could be to add a underscore _ after the hash with a number indicating the position of the setup, so like inventorysetups.setupsV3.hash_0 would indicate position 0. This seems more difficult. Each time a setup moves or is created or is deleted, some of the positions need to be updated.

Other things I've tested:

poi56iop commented 8 months ago

Very good points, and great catch on the ordering; I didn't notice the stable ordering as a feature of the original implementation.

To avoid frequent mass-config-key-rewrites, something like the first solution would be better. I'll need to investigate/understand how ordering within different sections is handled. I'd be tempted to use the fix as an opportunity to handle the unique IDing if it can solve any problems here. If we were only concerned about sorting by creation order, using epoch of creation/import time would be a reasonable solution, but we also allow manual re-ordering by the user. Rather than having an ordering-preservation config key refer to setups by hash while the sections-key refers to them by name, it could be time to unify on an ID that could be used by bank tag layouts etc. as discussed in discord.

Thinking about it more though, it'll probably be easier to keep changes isolated to serialization/deserialization in this file. I'm guessing per-section ordering will be unaffected, so your proposal should solve things nicely and without much extra code.

I'll try to work on it soon.

dillydill123 commented 8 months ago

Very good points, and great catch on the ordering; I didn't notice the stable ordering as a feature of the original implementation.

To avoid frequent mass-config-key-rewrites, something like the first solution would be better. I'll need to investigate/understand how ordering within different sections is handled. I'd be tempted to use the fix as an opportunity to handle the unique IDing if it can solve any problems here. If we were only concerned about sorting by creation order, using epoch of creation/import time would be a reasonable solution, but we also allow manual re-ordering by the user. Rather than having an ordering-preservation config key refer to setups by hash while the sections-key refers to them by name, it could be time to unify on an ID that could be used by bank tag layouts etc. as discussed in discord.

Thinking about it more though, it'll probably be easier to keep changes isolated to serialization/deserialization in this file. I'm guessing per-section ordering will be unaffected, so your proposal should solve things nicely and without much extra code.

I'll try to work on it soon.

section and section ordering should not be affected. It’s not ideal to have one organized by hash and one organized by name, but it’s totally serviceable. For bank tag layouts, all that matters is the setup name or hash, so I would avoid touching sections for now.

poi56iop commented 8 months ago

I would like to preserve the "natural" order of the setups. [...]

  • Migration from V1 to V3 (broken atm, see my comments)
  • Moving setups (Doesn't do anything anymore)

I believe these should all be fixed.

poi56iop commented 8 months ago

Let me know when everything seems fully ready to go; and I can squash again if you want. (Though IIRC doesn't GitHub let you do that at merge time?)

dillydill123 commented 8 months ago

gonna do some more testing and try to break it

dillydill123 commented 8 months ago

Looks good. I tried to break it many different ways but could not. Things start to get weird past 500 setups. menu entries in show worn items will start breaking. I'll just toss a warning about that in the readme. Nice work by the way!