Bug - Fixed mood clear does not clear entire mood

[ckb]

Per https://wiki.kolmafia.us/index.php/Mood:
mood clear - Remove all triggers from the current mood. Ignores composable moods.

This is how I think it used to work, but it is inconsistent and only removes some triggers: Multiple 'mood clear' will eventually remove all triggers.

> mood list

When I get A Little Bit Poisoned, uneffect A Little Bit Poisoned
When I get Hardly Poisoned at All, uneffect Hardly Poisoned at All
When I get Majorly Poisoned, uneffect Majorly Poisoned
When I get Really Quite Poisoned, uneffect Really Quite Poisoned
When I get Somewhat Poisoned, uneffect Somewhat Poisoned

> mood clear

Cleared mood.

> mood list

When I get Hardly Poisoned at All, uneffect Hardly Poisoned at All
When I get Really Quite Poisoned, uneffect Really Quite Poisoned

> mood clear

Cleared mood.

> mood list

When I get Really Quite Poisoned, uneffect Really Quite Poisoned

> mood clear

Cleared mood.

> mood list

 

[fronobulax]

There were changes in the area in December so something probably got broken. I'll look into it, especially since a before/after test might be an easy thing to write

 

[fronobulax]

As a progress report, it might be working as intended.

It does seem to remove all triggers from the current mood, but "mood list" returns all triggers for all moods. So if there are multiple moods some of which have the same triggers then that explains the beginning of the log. But repeated invocations of mood clear shouldn't change anything unless something else is changing the mood.

 

[ckb]

I checked my moods, and I have only 1 mood with any triggers. That first mood list is all the triggers in that one mood. A single mood clear only removes some of the triggers in that mood.
For now, I am doing this ugly thing:

while (count(mood_list())>0) { cli_execute("mood clear"); }

 

[fronobulax]

I'm in the middle of the debugger and there is definitely a case where something iterates over a list with four elements but stops after two. Not working as intended

 

[fronobulax]

PR in draft and will be submitted once I add some more tests. Simplistically a change in December set up a situation where the code was iterating over the list it was deleting from and that is never a good idea.

 

[Veracity]

If you have an Iterator and want to delete the current element, you can use it.remove(). Otherwise, no joy.

 

[heeheehee]

Seems like it's slightly more nuanced than that; it's more like:

void removeFromOtherList(List<T> list) {
  for (T element : list) {
    otherList.remove(element);
  }
}

where in some cases otherList was being passed as an argument.

 

[Veracity]

Seems like it's slightly more nuanced than that; it's more like:

void removeFromOtherList(List<T> list) {
  for (T element : list) {
    otherList.remove(element);
  }
}

where in some cases otherList was being passed as an argument.

Yes. But notice that it is calling remove() on the list, rather than on the list iterator, which is implicit within the foreach loop.
Which was my point; if list == otherList, I expect this code to fail. As it is.
(I had studied the code in question before I wrote my response, BTW, so I understood the nuance...)

 

[fronobulax]

I assumed this was new and caused by recent changes so started with the commit differences after I observed, in the debugger, iteration stopping when there were still items left.

 

[Veracity]

Which is to say, I was suggesting something like this:

Replace:

  public static void removeTriggers(final Collection<MoodTrigger> triggers) {
    for (MoodTrigger trigger : triggers) {
      if (MoodManager.currentMood.removeTrigger(trigger)) {
        MoodManager.displayList.remove(trigger);
      }
    }
  }

with:

  public static void removeTriggers(final Collection<MoodTrigger> triggers) {
    Iterator<MoodTrigger> it = triggers.iterator();
    while (it.hasNext()) {
      MoodTrigger trigger = it.next();
      if (MoodManager.currentMood.removeTrigger(trigger)) {
        if (triggers == MoodManager.displayList) {
          it.remove();
        } else {
          MoodManager.displayList.remove(trigger);
        }
      }
    }
  }

(That method works, by the way; also add an import of java.util.Iterator, create a test mood, type "mood clear" in the gCLI, and voila! the mood is now empty. In order for it to become a PR, I'd expect it would include a test which would fail with the current method and succeed with the new method.)

 

[fronobulax]

PR merged.