Bug - Fixed Unexpected Item Manager Panel Behaviour

fronobulax

Developer
Staff member
This may be operator error but with the Potions Panel (thank you!) requiring other changes I may be seeing an unexpected side effect.

Item Manager->Usable->Food

I expect the state of the checkboxes for "per full" and "by room" to persist when I shut down and restart mafia and then log in with the same character. What I see is that they are both unchecked after login/restart.

This happens with both Food and Booze.

When I have both boxes checked and I successfully consume the first item on the list and there is room remaining, I expect the list to resort so that the room required for the top item is less than or equal to the amount of room remaining. What I am seeing is that consumption does not trigger any resort or filtering constrained by available room.

This also happens with both Food and Booze. "per full" and "by room" are the only boxes checked.

r19326

Edit: So after seeing this for a day or two and posting, I went back to check and the sorting worked as expected for booze. The food failure had room 15, an epic food at 8 and an epic food at 7. I ate an 8 (ha ha) and expected the 7 to sort to the top. It didn't. The 8 was still present even though there was only 7 fullness left. The booze , on the other hand, only had two epic choices and the second one was "room" one. After I consumed the first, the only remaining epic booze sorted to the top.

Edit: Just noticed a debug log that may be related.

Code:
Unexpected error, debug log printed.
class java.lang.IllegalArgumentException: Comparison method violates its general contract!
java.lang.IllegalArgumentException: Comparison method violates its general contract!
	at java.base/java.util.TimSort.mergeLo(TimSort.java:781)
	at java.base/java.util.TimSort.mergeAt(TimSort.java:518)
	at java.base/java.util.TimSort.mergeCollapse(TimSort.java:448)
	at java.base/java.util.TimSort.sort(TimSort.java:245)
	at java.base/java.util.Arrays.sort(Arrays.java:1515)
	at java.base/java.util.ArrayList.sort(ArrayList.java:1749)
	at java.base/java.util.Collections.sort(Collections.java:177)
	at net.java.dev.spellcast.utilities.LockableListModel.sort(LockableListModel.java:150)
	at net.sourceforge.kolmafia.swingui.panel.UseItemEnqueuePanel$ReSortListener.execute(UseItemEnqueuePanel.java:477)
	at net.sourceforge.kolmafia.swingui.listener.ThreadedListener.run(ThreadedListener.java:239)
	at net.sourceforge.kolmafia.RequestThread$SequencedRunnable.run(RequestThread.java:418)
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at java.base/java.lang.Thread.run(Thread.java:834)
 
Last edited:

Veracity

Developer
Staff member
This and this and this.

Reasonable to have a separate bug report to discuss this, rather than as responses to various commits.

The "per full", "per drunk" and "per spleen" checkboxes are all tied to a single setting - "showGainsPerUnit". That means if you check any of them, they will all get checked. Since it loads the setting when creating the panel, and those checkboxes all listen for that setting, if you log out wiuth the checkbox set and log in again. the checkboxes will be set. The code for that did not change. That is how it worked before and that is how it works now, for me. Puzzing that it works differently for you since, as I said, the code to tie the checkboxes to the setting did not change.

The "by room" checkbox used to be tied to the "sortByRoom" setting. I thought it was weird that choosing that for one typoe of consumable would change all of them, so I removed it. That was a change. But, I'm OK with tying it back to that setting again - especially since the "per full" changes all three panels.

I'll ponder that debug log. Seems like it is pointing to an error in my comparator.
 

fronobulax

Developer
Staff member
I decided to make a new report elsewhere because it happened with a non-TCRS character as well and I would rather start a new report than incorrectly point the finger at the wrong commit. I make enough mistakes as it is :)

I just started mafia and logged in. I verified that sortByRoom was true by inspecting the settings file and displaying it in the gCLI. Nevertheless the Item Manager GUI displays the "by room" check box as unchecked for Food, Booze and Spleen. I went through a cycle of displaying sortByRoom in the gCLI, toggling the check box and displaying again. sortByRoom was always displayed as true in the gCLI. I did not try and set it to false via the gCLI and see what the GUI did.

I tried something similar with showGainsPerUnit and was unable to reproduce the unexpected (by me) behaviour. Since I'd rather not plead guilty to wasting your time with a false report, perhaps what I saw was the result of something not being set or updated because of the comparison exception? But in any event I can not reproduce the persistence problem with showGainsPerUnit so that portion of the report is OBE.

Thank you.
 

Veracity

Developer
Staff member
The "reports" I pointed to were not in the TCRS thread, so that's sort of irrelevant.
Regardless, I specifcally said it was reasonable for you to make a new thread, so you didn't really need to try to justify it. :)

Revision 19327 puts the "by room" checking bback in the Concoction, even though it is only needed in the UseItemEnqueue Panel. I spent a lot of time today experimenting with mirror images of LockableListModels and letting you assign a Comparator to them to be used for sorting, to no avail. Some things worked well, others not so well.

No more time to deal with it at the moment.

Try that revision.
 

fronobulax

Developer
Staff member
I ran through the preference persistence on one character and stopping and starting mafia. Everything worked as expected as far as persistence.

r19327
 

fronobulax

Developer
Staff member
See r19337.

I found what was probably a typo or copy/paste error in a comparator. It could explain the debug log. My ability to write test cases was lousy to begin with and has atrophied so I did not do so, although I should have. In any event I'd be comfortable closing this since everything that is easily repeatable has been fixed and one change has been made that probably caused the debug log.
 

Veracity

Developer
Staff member
I studied that code - hard - trying to understand why food and only food was not working as expected.
Your eyes are better than mine. :)
 

fronobulax

Developer
Staff member
I studied that code - hard - trying to understand why food and only food was not working as expected.
Your eyes are better than mine. :)

I was looking at the comparator thinking I might be able to write a test that the contract was met and it was actually my IDE that flagged it. Since the typo caused a variable to be set and then set again before it was used, the IDE saw the first setting as dead code and displayed the line differently. :)
 
Top