Bug - Fixed Sorting on the restore pane causes Unexpected error/debug

MCroft · Sep 23, 2020

OK, I've managed to get the error from this report to happen reliably (for me) and I was wrong about where it was. It was happening for me in the Palindome because that's the first time I generally need to restore HP.

It happens when I am using Item Manager:Restores tab and press the column header for the HP Restore column.

Note that it does sort the first 3 items in place, but not the blanks and not the items after that.

Screen Shot 2020-09-22 at 11.34.55 PM.png

Screen Shot 2020-09-22 at 11.35.06 PM.png

version: KoLmafia v20.7 r20382 (my local source build)
Java: 13.0.2

gCLI text
Using 1 scroll of drastic healing...
Unexpected error, debug log printed.
Finished using 1 scroll of drastic healing.

View attachment DEBUG_20200923.txt

Since I started from terminal, I also have this. I can make this happen as many times as I press the sort button.

Code:

Exception in thread "AWT-EventQueue-0" java.lang.ClassCastException: java.lang.Long incompatible with java.lang.Integer
	at net.sourceforge.kolmafia.swingui.widget.ShowDescriptionTable$RenderedComparator.compare(ShowDescriptionTable.java:412)
	at org.jdesktop.swingx.decorator.Sorter.compare(Sorter.java:136)
	at org.jdesktop.swingx.decorator.Sorter.compare(Sorter.java:112)
	at org.jdesktop.swingx.decorator.ShuttleSorter.sort(ShuttleSorter.java:157)
	at org.jdesktop.swingx.decorator.ShuttleSorter.sort(ShuttleSorter.java:127)
	at org.jdesktop.swingx.decorator.ShuttleSorter.sort(ShuttleSorter.java:126)
	at org.jdesktop.swingx.decorator.ShuttleSorter.sort(ShuttleSorter.java:127)
	at org.jdesktop.swingx.decorator.ShuttleSorter.filter(ShuttleSorter.java:77)
	at org.jdesktop.swingx.decorator.Filter.refresh(Filter.java:349)
	at org.jdesktop.swingx.decorator.Sorter.refresh(Sorter.java:58)
	at org.jdesktop.swingx.decorator.Filter.refresh(Filter.java:74)
	at org.jdesktop.swingx.decorator.FilterPipeline.setSorter(FilterPipeline.java:129)
	at org.jdesktop.swingx.decorator.FilterPipeline$SorterBasedSortController.toggleSortOrder(FilterPipeline.java:171)
	at org.jdesktop.swingx.JXTable.toggleSortOrder(JXTable.java:2046)
	at org.jdesktop.swingx.JXTableHeader$HeaderListener.doSort(JXTableHeader.java:608)
	at org.jdesktop.swingx.JXTableHeader$HeaderListener.mouseClicked(JXTableHeader.java:589)
	at java.desktop/java.awt.AWTEventMulticaster.mouseClicked(AWTEventMulticaster.java:278)
	at java.desktop/java.awt.AWTEventMulticaster.mouseClicked(AWTEventMulticaster.java:277)
	at java.desktop/java.awt.Component.processMouseEvent(Component.java:6639)
	at java.desktop/javax.swing.JComponent.processMouseEvent(JComponent.java:3342)
	at java.desktop/java.awt.Component.processEvent(Component.java:6401)
	at java.desktop/java.awt.Container.processEvent(Container.java:2263)
	at java.desktop/java.awt.Component.dispatchEventImpl(Component.java:5012)
	at java.desktop/java.awt.Container.dispatchEventImpl(Container.java:2321)
	at java.desktop/java.awt.Component.dispatchEvent(Component.java:4844)
	at java.desktop/java.awt.LightweightDispatcher.retargetMouseEvent(Container.java:4918)
	at java.desktop/java.awt.LightweightDispatcher.processMouseEvent(Container.java:4556)
	at java.desktop/java.awt.LightweightDispatcher.dispatchEvent(Container.java:4488)
	at java.desktop/java.awt.Container.dispatchEventImpl(Container.java:2307)
	at java.desktop/java.awt.Window.dispatchEventImpl(Window.java:2762)
	at java.desktop/java.awt.Component.dispatchEvent(Component.java:4844)
	at java.desktop/java.awt.EventQueue.dispatchEventImpl(EventQueue.java:772)
	at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:721)
	at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:715)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:704)
	at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:85)
	at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:95)
	at java.desktop/java.awt.EventQueue$5.run(EventQueue.java:745)
	at java.desktop/java.awt.EventQueue$5.run(EventQueue.java:743)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:704)
	at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:85)
	at java.desktop/java.awt.EventQueue.dispatchEvent(EventQueue.java:742)
	at java.desktop/java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:203)
	at java.desktop/java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:124)
	at java.desktop/java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:113)
	at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:109)
	at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:101)
	at java.desktop/java.awt.EventDispatchThread.run(EventDispatchThread.java:90)

fronobulax · Sep 23, 2020

Interesting. I noticed two things. First I did not have the HP Restore column displayed and so I had to enable the display. Second was Doc Galactic's Ailment Ointment. You have it. I didn't. I bought some and no amount of refreshing inventory would make it appear in the Restores list. For giggles, try closeting it and see if you can repeat. I have seen this cast error once in a while and was not really sure why. That's why I have been slowly adding annotation to debug logs

But my hypothesis is that there is some data problem with the ointment and that is the root cause.

Edit:

So I can get it to show up but the Name of the elixir is truncated.

The first image was from General with Doc entered as a search keyword. I then switched to Usable->Restores and note the truncation. My screen capture image that had more context is too big :-(

Comparatively busy day for Covid Times but I will try and look at this. If it's a data error it is probably within my skill set

Veracity · Sep 23, 2020

Your stack trace indicates that a column in the data model (presumably HP Restore) that is being sorted has an Integer in it when a Long was expected.
Perhaps, as frono posits, there is a data error and one of the restores added to the model is put in with an Integer, somehow?
If so, that is the root cause of the exception, and should be fixed.

fronobulax · Sep 23, 2020

Veracity said:
Your stack trace indicates that a column in the data model (presumably HP Restore) that is being sorted has an Integer in it when a Long was expected.
Perhaps, as frono posits, there is a data error and one of the restores added to the model is put in with an Integer, somehow?
If so, that is the root cause of the exception, and should be fixed.

Looking at it now.

From the run where I bought the Ailment.

buy 1 Doc Galaktik's Ailment Ointment for 100 each from shop #3390861 on 20200923
Unexpected error, debug log printed.

Edit: Happy Birthday, Veracity.

Didn't notice earlier

MCroft · Sep 23, 2020

Happy Birthday, Veracity.

Yep, I add the HP and MP columns so I know what restore I want. I wish it was more like Food and Drink, but it is what it is and that vague feature request is outside the scope of this bug report.

Closet Trial and Error leads me to determine that Scroll of Drastic Healing is the culprit, for me. I don't currently have the other full HP restores, but when y'all find the issue, we'll want to test several of these.

fronobulax · Sep 23, 2020

Will check scroll. Buying the Ailment did not seem to generate any problems. The name of the Elixir issue noted above is s display issue. It was truncated, I made mafia (and by implication the restore window larger) full screen and the complete name appeared. Display issue. Also am not seeing displayed blanks sorting where I expect but...

fronobulax · Sep 23, 2020

Done for the day :-(

Does not seem to be keyed to a specific item. I put everything in my closet that I could and then added items a couple at a time. The Restore display would update with the new items but seemed to forget about the old until I sorted on a column.

Blanks are not displaying where I expect them after a sort.

I wonder if there is an interaction with Take items from the clan stash when needed? Stash items don't appear in the GUI but running the debugger there are times when I see both 500 scrolls from the stash and 14 in my inventory. Need to clear my head.

MCroft · Sep 24, 2020

To Veracity's point about Frono's point, something is passing an Integer and a Long into the RenderedComparator. I think the real fix is either to find where that is and limit it to sending in objects that are Integers or Strings or else enhance the comparator to handle more types of objects.

I added the orange code to line 411 of ShowDescriptionTable.java and everything sorts correctly.

Code:

		public RenderedComparator( LockableListModel originalModel, int column, boolean[] flags )
		{
			this.model = originalModel;
			this.column = column;
			this.flags = flags;
		}

		public int compare( Object o1, Object o2 )
		{
			Object o1val = TableCellFactory.get( this.column, this.model, o1, this.flags, false, true );
			Object o2val = TableCellFactory.get( this.column, this.model, o2, this.flags, false, true );
			if ( o1val instanceof Integer )
			{
				if ( o2val == null )
				{
					return 1;
				}
	
[COLOR="#FFA500"]				if ( o2val instanceof Long)
				{
					Integer a1 = (Integer) o1val;
					Integer a2 = ((Long) o2val).intValue();				
					return a1.compareTo( a2 );
				}[/COLOR]
				
				Integer a1 = (Integer) o1val;
				Integer a2 = (Integer) o2val;
				return a1.compareTo( a2 );
			}

Ryo_Sangnoir · Sep 24, 2020

In TableCellFactory, line 322 and down:

Code:

case 4:
        double hpRestore = RestoresDatabase.getHPAverage( advresult.getName() );
        if ( hpRestore <= 0 )
        {
                return null;
        }
        long maxHP = KoLCharacter.getMaximumHP();
        if ( hpRestore > maxHP )
        {
                return maxHP;
        }
        return IntegerPool.get( (int) hpRestore );
case 5:
        double mpRestore = RestoresDatabase.getMPAverage( advresult.getName() );
        if ( mpRestore <= 0 )
        {
                return null;
        }
        long maxMP = KoLCharacter.getMaximumMP();
        if ( mpRestore > maxMP )
        {
                return maxMP;
        }
        return IntegerPool.get( (int) mpRestore );

Returns a long if the restore is more than your maximum HP (or MP) and an int otherwise.

fronobulax · Sep 24, 2020

Ryo_Sangnoir said:

In TableCellFactory, line 322 and down:

Code:

case 4:
        double hpRestore = RestoresDatabase.getHPAverage( advresult.getName() );
        if ( hpRestore <= 0 )
        {
                return null;
        }
        long maxHP = KoLCharacter.getMaximumHP();
        if ( hpRestore > maxHP )
        {
                return maxHP;
        }
        return IntegerPool.get( (int) hpRestore );
case 5:
        double mpRestore = RestoresDatabase.getMPAverage( advresult.getName() );
        if ( mpRestore <= 0 )
        {
                return null;
        }
        long maxMP = KoLCharacter.getMaximumMP();
        if ( mpRestore > maxMP )
        {
                return maxMP;
        }
        return IntegerPool.get( (int) mpRestore );

Returns a long if the restore is more than your maximum HP (or MP) and an int otherwise.

Thank you. I will fix that later today.

MCroft · Sep 24, 2020

OK, waaaay back during Advent of 2019, there was a fix for HP > 2^31 in revision 19629.

It looks like the code returns an Integer object if the item restores less than or equal to your max and a Long object if the item restores more than your max. This is intentional, and we should enhance the comparator to handle a mix of Longs and Integers.

This is in TableCellFactory.java

Code:

	private static Object getRestoresCell( int columnIndex, boolean isSelected, AdventureResult advresult, boolean raw )
	{
		switch ( columnIndex )
		{
		[...]
		case 4:
			double hpRestore = RestoresDatabase.getHPAverage( advresult.getName() );
			[...]
[COLOR="#FF0000"]			long maxHP = KoLCharacter.getMaximumHP();
			if ( hpRestore > maxHP )
			{
				return maxHP;[/COLOR]
			}
			return IntegerPool.get( (int) hpRestore );

MCroft · Sep 24, 2020

This is the logic for a Long-aware compare, assuming that we can always up convert Integers to Longs to do the comparison.

This is the patch.
View attachment LongAwareCompare.patch

I don't know what happens if we try to compare an Integer or Long to a String with the number first. It may give an error and it probably should.

Tested, somewhat.

fronobulax · Sep 24, 2020

MCroft said:
This is intentional

Would you explain why it was intentional?

MCroft · Sep 24, 2020

fronobulax said:
Would you explain why it was intentional?

It's needed for lucky users like fractalnaval who had 2176875415 HP in this bug report.

Veracity said:
Revision 19629 stores Meat, HP, and MP variables as longs, rather than as ints, and when they are made into AdventureResults, stores them in the AdventureLongCountResult subclass.

I tested a lot of things, including running my farming script for a bunch-of-turns, wherein Meat, HP, and MP were gained and lost, and it all looks normal.

I would not be at all surprised if there are obscure issues, but I expect other people doing other things will notice and report them, and I will fix them.

I would say that sorting the restores table by a column that's not on by default if you have an item that restores more than your maxHP qualifies as an obscure issue.

MCroft · Sep 24, 2020

bah. added some nulls and it failed in the way I was thinking it would on comparing a Number to a null in the reverse sort case (returns 0, this is the case Frono saw yesterday at 12:48.

I'm going to fix it by testing for nulls early on and switching the entire thing to a o1val.getClass().getSimpleName() value so I can use a case statement instead of a lot of ifs...

fronobulax · Sep 24, 2020

We weren't clear.

What I thought you were saying was intentional was having a function that sometimes returned a long and sometimes returned an int. That violates several design and coding practices so I'd like to know why. In the absence of a good reason for doing so, I'd be inclined to make sure it always returned a long.

MCroft · Sep 24, 2020

Sorry, didn't mean to be unclear. It was not intentional on my part, because it wasn't my change. I just found the revision using svn blame and searched the bug DB for what that revision addressed.

The compare method handles Strings and Integers and not Longs (currently). It is slightly broken on 01val == null and 02val is not a string (your "nulls not sorting").

It's easy (and probably correct) to change TableCellFactory to only return Longs, but then you still need the compare method to handle Longs. It could just handle Longs (not Integers) if we wanted, but I don't know where else it might get called from, which is why I approached it as adding Long support.

Do you want something different?

MCroft · Sep 24, 2020

New version of my patch. Much more readable with a case statement and it's easer to see how the logic works and how to expand it if we ever need to use the same sorter for Doubles (for instance).

This version fixes the blanks problem.

View attachment LongAwareCompare_V2.patch

fronobulax · Sep 24, 2020

MCroft said:
Sorry, didn't mean to be unclear. It was not intentional on my part, because it wasn't my change. I just found the revision using svn blame and searched the bug DB for what that revision addressed.

The compare method handles Strings and Integers and not Longs (currently). It is slightly broken on 01val == null and 02val is not a string (your "nulls not sorting").

It's easy (and probably correct) to change TableCellFactory to only return Longs, but then you still need the compare method to handle Longs. It could just handle Longs (not Integers) if we wanted, but I don't know where else it might get called from, which is why I approached it as adding Long support.

Do you want something different?

I was thinking the TCF always returns Longs and the comparator needed to handle whatever was thrown at which will now include longs. I'll check you patch eventually but it's 5 o'clock somewhere

fronobulax · Sep 25, 2020

try r20387.

I forced the cell value to be a long, not sometimes a long and sometimes an Integer. That by itself didn't give me the sort results I expected so I reworked the sort as well. If the first argument is a string then both are compared as strings. Otherwise (since everything is being displayed) convert to longs and compare. The Restore table behaves as I expect and I can't force the debug log so maybe things are fixed?

I note in HP and MP blanks sort to the top (or bottom). I'm not sure that's my preference but it seemed to be the old behaviour so I left it alone.

Bug - Fixed Sorting on the restore pane causes Unexpected error/debug

Developer

Developer

Developer

Developer

Developer

Developer

Developer

Developer

Developer

Developer

Developer

Developer

Developer

Developer

Developer

Developer

Developer

Developer

Developer

Developer