search

isXander / YetAnotherConfigLib

YetAnotherConfigLib (yacl) is just that. A builder-based configuration library for Minecraft.
GNU Lesser General Public License v3.0
89 stars 33 forks source link

`OptionImpl.BuilderImpl#instant` prevents saving #187

Open LucunJi opened 2 weeks ago

LucunJi commented 2 weeks ago

How to reproduce

As the title says, it simply prevents saving in all cases.

Analysis

Case 1: the common scenario

  1. After OptionImpl#requestSet: binding().getValue() and pendingValue differs; also calls OptionImpl#triggerListeners
  2. After OptionImpl#applyValue: binding().getValue() and pendingValue becomes the same
  3. After YACLScreen#onOptionChanged: pendingChanges is false

Now the configs won't be the saved since the pendingChanges is false. BROKEN.

Case 2: intentionally swap step 2 and 3, it won't be fixed

  1. After OptionImpl#requestSet: binding().getValue() and pendingValue differs; also calls OptionImpl#triggerListeners
  2. After YACLScreen#onOptionChanged: pendingChanges is true
  3. After OptionImpl#applyValue: binding().getValue() and pendingValue becomes the same

It looks fine, but what if we are pressing the Reset button, and the last option hasn't been changed?

  1. After OptionImpl#requestSet: binding().getValue() and pendingValue are still the same
  2. After YACLScreen#onOptionChanged: pendingChanges is false
  3. After OptionImpl#applyValue: binding().getValue() and pendingValue are still the same

Now the configs won't be the saved since the pendingChanges is false. BROKEN.

How to fix it

Keep track of the initial value, use a "dirty" flag, etc.

LucunJi commented 2 weeks ago

A potential patch is proposed here (I used mixins in my project)