MidoriKami / SortaKinda

SortaKinda is a XivLauncher/Dalamud plugin.
Other
10 stars 5 forks source link

Item Name Filter Regex stops working after logout #26

Closed R4d1o4ct1v3 closed 1 month ago

R4d1o4ct1v3 commented 2 months ago

Since DT I've noticed that any rule using a Regexp name filter will work only during the game session it is created in. After login it will act as it if matches nothing.

I compiled the plugin directly from GitHub and added it as a Dev Plugin.

Steps to reproduce:

  1. Put a couple of different "Grade [1-8] Dark Matter" in your inventory.
  2. Create a new rule at the bottom of the rules with that string as the Name Filter.
  3. It will now sort those items as expected.
  4. Logout and back in.
  5. It no longer sorts those items according to the rule, but rather sorts them according to earlier rules. (In my case a rule that catches any Catalyst type item.)
MidoriKami commented 2 months ago

Thank you for the reproduction steps, this issue has been added to the queue.

MidoriKami commented 2 months ago

Not able to reproduce this unfortunately. Going to close now until someone else is able to reproduce with different steps.

PrincessRTFM commented 2 months ago

Can confirm the same issue, but it also appears to be happening on plugin unload/reload. I logged in, fixed a few broken rules, checked sorting was working correctly, updated the plugin, and suddenly regex name rules were no longer working again. Manually disabling and re-enabling the plugin has the same effect. If logging out causes the plugin to save the sorting rules to disk and logging in loads them from the config file, then my best guess is that something in the de/serialisation is mangling the patterns somehow. I can provide my sorting rule export, but my rules are extensive, so it may be better to get a simpler set from elsewhere.

MidoriKami commented 2 months ago

If I can get a absolutely barebones reproduction I can look into it more, I tried making a few named filters, and unloading/reloading the plugin several times, it broke once, but I was unable to reproduce it after that.

PrincessRTFM commented 2 months ago

I'll admit it's not exactly barebones but the issue is entirely consistent on my setup, so here's my config files in the hopes that it'll help.

R4d1o4ct1v3 commented 2 months ago

I tried this again on a completely barebones install. No other plugins, no other rules, only the base game on Win10, opened through XIVLauncher with a clean Dalamud config, and SortaKinda installed as a Dev plugin compiled from GitHub.

Was able to reproduce the problem consistently on every attempt. Following the same steps I listed in the original report. A couple of screenshots here to demonstrate what I was seeing.

Immediately after creating the rule: 01_first

After relogging. (Just going back to login screen and logging back in.) 02_after_relog

Another thing I noticed. After the filter broke, I would be able to add an exact copy of the Item Name Filter, and that would fix it. The list would then show two identical name filters. - After reloging again tho, it would remove the duplicate and the filter would stop working again. 03_after_relog_duplicate_rule

doubleyewdee commented 1 month ago

Same issue here, down to the "add a duplicate and it fixes it."

doubleyewdee commented 1 month ago

Sorry, double-dipping. It kind of feels like a deserialization problem with UserRegex in KamiLib. I think, on load, only the default ctor is called, and the Text property is what's deserialized into with a direct assignment, but its setter is bare when it seemingly should not be. It strikes me that KamiLib shouldn't actually allow its Regex property to ever be null, but that might be harder to implement without a lot of boilerplate JSON deserialization contract garbage.

in KamiLib/Classes/UserRegex.cs I think you need to remove the basic Text property and replace it with:

    [JsonIgnore] private string text = string.Empty;
    /// <summary> The user-supplied text. </summary>
    public string Text
    {
        get { return this.text; }
        set { this.UpdateText(value); }
    }

and then patch UpdateText:

    /// <summary> Update the filter via text. </summary>
    public void UpdateText(string text, RegexOptions options = DefaultOptions) {
        this.text = text;
        Regex = BuildRegex(text, options);
    }

    /// <summary> Update the filter via regex. </summary>
    public void UpdateText(Regex? regex) {
        this.text = regex?.ToString() ?? string.Empty;
        Regex = regex;
    }

I'd PR a fix myself but I don't have any way to test this and I don't want to send untested PRs (or set up enough dev environment to validate :)).

ETA: it looks like you lost your JsonConstructor attribute when moving off Newtonsoft.Json, but System.Text.Json does support that and it might be the way to go?

R4d1o4ct1v3 commented 1 month ago

I tested the changes suggested by @doubleyewdee and they do in fact seem to fix the issue!

Created a PR for it here: https://github.com/MidoriKami/KamiLib/pull/8