Results 1 to 8 of 8

Thread: Unexpected Item Manager Panel Behaviour

  1. #1
    Developer fronobulax's Avatar
    Join Date
    Feb 2009
    Location
    Central Virginia, USA
    Posts
    4,397

    Default Unexpected Item Manager Panel Behaviour

    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 by fronobulax; 06-21-2019 at 02:07 PM. Reason: Add Info
    Well, thank you.
    Originally Posted by Veracity View Post

  2. #2
    Developer Veracity's Avatar
    Join Date
    Mar 2006
    Location
    The Unseelie Court
    Posts
    11,876

    Default

    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.
    Ph'nglui mglw'nafh Cthulhu
    R'lyeh wgah-nagl fhtagn.

  3. #3
    Developer fronobulax's Avatar
    Join Date
    Feb 2009
    Location
    Central Virginia, USA
    Posts
    4,397

    Default

    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.

  4. #4
    Developer Veracity's Avatar
    Join Date
    Mar 2006
    Location
    The Unseelie Court
    Posts
    11,876

    Default

    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.

  5. #5
    Developer fronobulax's Avatar
    Join Date
    Feb 2009
    Location
    Central Virginia, USA
    Posts
    4,397

    Default

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

    r19327

  6. #6
    Developer fronobulax's Avatar
    Join Date
    Feb 2009
    Location
    Central Virginia, USA
    Posts
    4,397

    Default

    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.

  7. #7
    Developer Veracity's Avatar
    Join Date
    Mar 2006
    Location
    The Unseelie Court
    Posts
    11,876

    Default

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

  8. #8
    Developer fronobulax's Avatar
    Join Date
    Feb 2009
    Location
    Central Virginia, USA
    Posts
    4,397

    Default

    I studied that code - hard - trying to understand why food and only food was not working as expected.
    Your eyes are better than mine.
    Originally Posted by Veracity View Post
    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. :-)
    Well, thank you.
    Originally Posted by Veracity View Post

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •