Question(s) about LockableListModel and the source code in general

[ajoshi]

Thanks guys! 99% done with formatting, will upload the final patch tomorrow. I also created an eclipse formatting file (whatever it's called), so I should magically have the same whitespace rules as you guys now.

Re: smaller patches, I absolutely understand what you mean, however this review is essentially changing LLMs in KoLConstants to Lists, and all the changes I had to make to get the app to compile. There are a number of other classes I haven't even touched yet! That said, I think I'm doing KoLConstants and KoLCharacter here, so I'll see if I can split those two.

 

[ajoshi]

Happy Valentine's Day (almost)! To show my love and appreciation for you guys, here is the formatted LockableListFactory patch number 1. There are more LockablelistModels to convert, but that can be done later (on another holiday perhaps).
Quick summary of changes:

1. Major change: Any class that tried to create a new LockableListModel now gets an instance of a List from LockableListFactory. This can be a LLM, but in environments where Swing cannot be found (and so we cannot have LLM at all), an ArrayList is returned. This means that all LockableListModels in KolConstants became Lists, which causes a bunch of edits across the codebase.
2. Similarly, LockableListFactory.getsortedInstance() will return a SortedListModel in Swing envs, and a SortedList in Swingless envs.
3. New utility class has been created: SwinglessUIUtils. It does very little: detects if Swing exists, has an accessor to check that, and that's it. I was thinking of moving calls like getmirror/getsize/setSelectedIndex to it in case you guys thought the Factory got too busy?
4. New Class: KoLGUIConstants which contains Constants that are only used in the GUI. This was actually created to hold the one LLM that would never need to exist in a non-Swing env.
5. The ComboSeparatorsRenderer in PartialMRUList is now an Object. I can rip this out of this change if you guys want, but this change is limited to the PartialMRUList class.

Sorry for the delay- it took forever to test GUI, CLI and then CLI again with ArrayList and SortedList being used.

 

[fronobulax]

I got errors applying the patch on two different machines. It may be my tools don't recognize something your tools allow in a patch file - one of those machines has problems generating patch files that everyone else on the team can read successfully - but I have not has the time to track this down any further. Sorry, especially since it might not be your patch that is at fault.

 

[ajoshi]

Reverted everything locally, updated to newest revision, applied patch, and yes, there were lines missing. No idea how it happened, I can only assume I misclicked on a checkbox when telling Eclipse to make the patch. Now I know to add one more step to my testing process: After creating patch, apply it locally to confirm I've made it correctly. EquipmentManager did not have all the required changes and that caused issues.
So sorry for the confusion! And I know the new path is probably causing a lot of madness so I get that you guys are busy

 

[heeheehee]

Your patch does have a bunch of carriage returns, but other than that, it applied without complaint. I'll give this a shot.

 

[ajoshi]

Whoops! I'll change my carriage return prefs as well. You'll never see them again!

 

[ajoshi]

Alrighty! Here's a new patchset that can be applied asap! The previous patch conflicted with code put in (I think) for Witchess.
This one contains all but one of the following:

• All the same changes as before!
• Unix newlines!
• The ability to be applied without conflicts!
• Bunnies!

 

[Bale]

Bunnies?! Hot damn! If this patch adds bunnies to KoLmafia it needs to be vetted and applied. Just yesterday I was thinking (yes, I was, don't call me a liar!) that the one thing wrong with KoLmafia is that it doesn't have enough bunnies. (Okay, I confess I was lying about thinking that yesterday, but I am definitely thinking that now.)

 

[Crowther]

Bunnies?! Hot damn! If this patch adds bunnies to KoLmafia it needs to be vetted and applied. Just yesterday I was thinking (yes, I was, don't call me a liar!) that the one thing wrong with KoLmafia is that it doesn't have enough bunnies. (Okay, I confess I was lying about thinking that yesterday, but I am definitely thinking that now.)

 

[Bale]

Yay! I got some bunnies added to KoLmafia today!

 

[ajoshi]

16786: Adding (more) bunnies to KoLmafia
by Bale

Well, would ya look at that!

 

[heeheehee]

So, I've been playtesting this locally, and the one thing I did notice is that in CharPaneRequest#refreshEffects (invoked by an api refresh), you add visibleEffects to KoLConstants.activeEffects twice. That has the nasty effect of mucking up my modifiers when running scripts, while going mostly undetected when playing through the relay browser (as charpane refreshes are common there and reset my effects properly).

Removing the duplicate addAll line fixes this issue.

 

[ajoshi]

Sorry, I was away from my computer this week (which was the reason I was working on the Android port in the first place!).

That duplicate addAll line is a mistake (as you realized), and it was one that I'm really sorry for having put in. I've been running volcano mining scripts as my major script-to-catch-issues, since I generally don't run scripts- I tend to use just the relay browser on my main. What scripts would you recommend I use to catch issues like these?

 

[heeheehee]

I'm not really sure there's a catch-all script, although Ezandora likes to call Guide "torture for ASH" (or something along those lines).

 

[ajoshi]

Not that I didn't believe you, but I just saw this in action. Obviously, I've fixed it, but now I'll be running multiple scripts and such (especially Guide) to confirm that there aren't any other issues.

 

[Darzil]

I'm not really sure there's a catch-all script, although Ezandora likes to call Guide "torture for ASH" (or something along those lines).

I'm not sure I see it as torture for ASH, but it's generally great for catching issues in status and quest tracking.

 

[ajoshi]

Alright, I added another month of testing, and saw no issues arising from this patch. Every time I saw behavior I felt was weird, I ran vanilla mafia and ensured it wasn't coming from this patch. Not sure how busy you guys are because of the new summer path, but here's to hoping someone can take a look!
I ran the Guide script where I saw an interesting issue arise due to difference in Android's Java, but that's not related to this change (Android doesn't recognize week_year 'Y' when parsing dates- it only accepts normal year 'y').

 

[ajoshi]

So sorry, but has anyone been able to look at that patch?

 

[lostcalpolydude]

I've added this code locally, and I will probably commit it in a few days if I don't run into any issues. If I haven't said or done anything by next weekend, bump this thread again.

In CustomCombatPanel.java you cast KoLCharacter.getBattleSkillNames() to a (LockableListModel<String>) even though the function already returns that type. I left that out since it seemed unnecessary.

 

[fronobulax]

Thank you lost for following up on this. I clearly have not managed to do so even though I offered.