r15514. I think I corrected some whitespace issues but I think that a lot and get it wrong.
Question(s) about LockableListModel and the source code in general
- Thread starter ajoshi
- Start date
Alrighty, I have another straightforward review: BuffBotHome and BuffBotFrame.
BuffBotHome contained the following code:
which as far as I can tell does nothing. In case you're wondering, it is empty. BBH isn't extended by anything so it's not like this is being overridden. Since this created a dependency on BBF from BBH, I removed it. As before, please tell me if this is a bad change. Thanks!
BuffBotHome contained the following code:
which as far as I can tell does nothing. In case you're wondering, it is empty. BBH isn't extended by anything so it's not like this is being overridden. Since this created a dependency on BBF from BBH, I removed it. As before, please tell me if this is a bad change. Thanks!
CakeArenaManager
Another patch! In this one I do something different- I register 2 listeners and fire them in CakeArenaManager.
This is because CAM needs to append to the ChatBuffer as well as clear it, but update() doesn't tell me which listener is fired (if multiple listeners are registered). I could have solved this by sending in the listener key as well (via ListenerRegistry) but I was afraid that change might be too big.
Anyway, CAM was accessing a StyledChatBuffer in FamiliarTrainingFrame. Now I register 2 listeners in FTF (I do not make FTF implement Listener- I create 2 Listener objects) which append to and reset the buffer.
Please tell me if I should instead make the change in ListenerRegistry to allow differentiation between two different listeners. Thanks!
Another patch! In this one I do something different- I register 2 listeners and fire them in CakeArenaManager.
This is because CAM needs to append to the ChatBuffer as well as clear it, but update() doesn't tell me which listener is fired (if multiple listeners are registered). I could have solved this by sending in the listener key as well (via ListenerRegistry) but I was afraid that change might be too big.
Anyway, CAM was accessing a StyledChatBuffer in FamiliarTrainingFrame. Now I register 2 listeners in FTF (I do not make FTF implement Listener- I create 2 Listener objects) which append to and reset the buffer.
Please tell me if I should instead make the change in ListenerRegistry to allow differentiation between two different listeners. Thanks!
Last edited:
Small patches, and if no one else is looking or commenting PM me. Submitting other people's work seems to be my greatest contribution of late
r15826 for the BuffBot patch. I tested it by running a do nothing BuffBot but there is plenty of opportunity for me to have missed something.
I tried the cakepatch, started getting all kinds of errors reverted it and the errors went away, so this one is going to need a bit more than a casual inspection and quick test. The errors occurred while running bccascend in a hardcore standard run. They all seemed to be related to automation - a script would fire and then fail, such as BBBS trying to do something while KoL thought it was still on a fight page. I can construct scenarios where this is the result of something not directly related - things that work because listeners fire in a specific order, for example - but doing so will take some time.
While I have your attention, your patch files had unnecessary whitespace changes. I have found and deleted them but that is something you could do next time around ;-)
While I have your attention, your patch files had unnecessary whitespace changes. I have found and deleted them but that is something you could do next time around ;-)
Whoa, I didn't see any issues with the cakepatch, which means I obviously missed something. I'll try out bccascend today and see what's going on!
Re: whitepace, noted! I'll make sure that my whitespace matches the project standard. I'm used to different whitespace rules at work, so I guess it's muscle memory at this point!
Re: whitepace, noted! I'll make sure that my whitespace matches the project standard. I'm used to different whitespace rules at work, so I guess it's muscle memory at this point!
So.. one year later, yes- there is definitely some bug introduced by cakepatch. I don't believe it is a threading issue though- what I see is that listeners don't get fired after a while. I still don't see a crash, but I will keep investigating and update this thread (next year, probably).
It is nice to know after a year that I was not seeing something no one else could. That Way Lies Madness!
I await your annual update, am curious about what the fundamental problem actually is/was and can still work with you yo get a fix or new feature committed.
Alrighty, figured it out ahead of schedule! So it was all entirely my fault.
The ListenerRegistry keeps a bunch of WeakRefs to all the registered Listeners. The new Listener instances in my patch were only defined within the scope of the FamiliarTrainingFrame constructor, so once that was done, the Listeners could be GCed. Aaand that would happen- If I ran some scripts, the GC would run and get rid of my Listeners. The ListenerRegistry would try to fire the event, fail, and nothing would work. Keeping a normal ref to the Listeners by making them fields in the FamiliarTrainingFrame is working exactly as it should. Their lifetime is then bound to the Frame as it should be.
Still unable to find a crash, so I'll look for it a bit more. Will update in 2 years!
The ListenerRegistry keeps a bunch of WeakRefs to all the registered Listeners. The new Listener instances in my patch were only defined within the scope of the FamiliarTrainingFrame constructor, so once that was done, the Listeners could be GCed. Aaand that would happen- If I ran some scripts, the GC would run and get rid of my Listeners. The ListenerRegistry would try to fire the event, fail, and nothing would work. Keeping a normal ref to the Listeners by making them fields in the FamiliarTrainingFrame is working exactly as it should. Their lifetime is then bound to the Frame as it should be.
Still unable to find a crash, so I'll look for it a bit more. Will update in 2 years!
Alrighty! So this is a new patch that works correctly as far as I can tell. Things I'm trying now:
Use GUI to train
Use cli to train
Run a script that adventures and use gui/cli to train while the script is running
What I've done in this patch:
The listeners I added are now fields of the Frame they belong to.
Make sure I follow the whitespace rules you guys have. Yell at me more if I still did it wrong!
I did another thing that was bothering me, but I can take it out if it does not fit in with the way you guys do stuff: All the listener code was in the form
I decided instead to use static final Strings to store the listener name, so I now have
This helps me see where and when this change is being fired, etc. Plus it makes sure that I don't typo the name and fire the wrong thing. Yes/No?
Use GUI to train
Use cli to train
Run a script that adventures and use gui/cli to train while the script is running
What I've done in this patch:
The listeners I added are now fields of the Frame they belong to.
Make sure I follow the whitespace rules you guys have. Yell at me more if I still did it wrong!
I did another thing that was bothering me, but I can take it out if it does not fit in with the way you guys do stuff: All the listener code was in the form
HTML:
NamedListenerRegistry.fireChange( "(CakeArenaManagerAddChat)" );
HTML:
NamedListenerRegistry.fireChange( LISTENER_NAME_ADD_CHAT );This helps me see where and when this change is being fired, etc. Plus it makes sure that I don't typo the name and fire the wrong thing. Yes/No?
Using static Strings for listener names seems fine, probably. I don't think I've ever messed with any of the Listener code, so I don't know how much sense it makes to do that in general (if they tend to actually get used more than once). I think LISTENER_ would be a better prefix than LISTENER_NAME_, but that's an arbitrary decision.
For whitespace, you have the opening { on the same line that starts a block, rather than on its own line.
For whitespace, you have the opening { on the same line that starts a block, rather than on its own line.
FWIW, mafia now runs a little bit on Android:
01-30 17:02:41.939 1232-7002/? I/System.out﹕ KoLmafia v17.2 r16639
01-30 17:02:41.939 1232-7002/? I/System.out﹕ Released on November 28, 2015
01-30 17:02:41.939 1232-7002/? I/System.out﹕ Currently Running on Linux
01-30 17:02:41.940 1232-7002/? I/System.out﹕ Local Directory is /data/data/ajoshi.com.basicandroidproject/files
01-30 17:02:41.940 1232-7002/? I/System.out﹕ Using Java 0
(and then it crashes)
In other news, Android is Java 0, so that's cool.
01-30 17:02:41.939 1232-7002/? I/System.out﹕ KoLmafia v17.2 r16639
01-30 17:02:41.939 1232-7002/? I/System.out﹕ Released on November 28, 2015
01-30 17:02:41.939 1232-7002/? I/System.out﹕ Currently Running on Linux
01-30 17:02:41.940 1232-7002/? I/System.out﹕ Local Directory is /data/data/ajoshi.com.basicandroidproject/files
01-30 17:02:41.940 1232-7002/? I/System.out﹕ Using Java 0
(and then it crashes)
In other news, Android is Java 0, so that's cool.
Alrighty, so I finally got started on LockableListModel. I tried using a List in some places and LLM in others, but there are some LLMs in KolConstants that absolutely needed to be LLM when it came to GUI. So I ended up going with a Factory. I also moved some things from KoLConstants to KolGUIConstants. These are things like Colors and other GUI-only constants.
This patch is not done- it's more of a progress indicator. Whitespace is all jacked, I'm thinking of extracting the utility methods to their own class, and there are conflicts I need to resolve.
I'd love it if someone could take a quick look and tell me if what I'm doing is sensible or if I'm going about this the wrong way. and sorry for the zip- the uncompressed patch exceeded the max attachment size
. Thanks!
Edit: There is a bug in this code- one of the replacements is wrong and casting fails. So I'm replacing the equipment array with an equipment list, which seems to be working fine. Still testing more, and I'll put up a nicer patch if someone tells me this is worth pursuing
Super Edit: Don't download this patch. The next one is much nicer!
This patch is not done- it's more of a progress indicator. Whitespace is all jacked, I'm thinking of extracting the utility methods to their own class, and there are conflicts I need to resolve.
I'd love it if someone could take a quick look and tell me if what I'm doing is sensible or if I'm going about this the wrong way. and sorry for the zip- the uncompressed patch exceeded the max attachment size
. Thanks!Edit: There is a bug in this code- one of the replacements is wrong and casting fails. So I'm replacing the equipment array with an equipment list, which seems to be working fine. Still testing more, and I'll put up a nicer patch if someone tells me this is worth pursuing
Super Edit: Don't download this patch. The next one is much nicer!
Last edited:
Just what you always wanted! A LockableListModel patch that actually works! Well, you can update your Crimbo list because I have what you want!
So this patch is pretty much the same as the last one, but with a few changes:
And this one is still not ready for submission because whitespace issues. Speaking of which, do you guys have an Eclipse code formatting file? Or do you guys format things by hand/muscle memory?
So this patch is pretty much the same as the last one, but with a few changes:
- The very eloquently named FakeLLM class has been deleted.
- A 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?
- Mafia no longer fails to init GearChangeFrame.
And this one is still not ready for submission because whitespace issues. Speaking of which, do you guys have an Eclipse code formatting file? Or do you guys format things by hand/muscle memory?
I let my IDE do the formatting and live in fear that when I update or change IDEs I have forgotten something that I won't remember until it is pointed out to me. Since my IDEs have stabilized on some flavor of IntelliJ, my settings file is probably not going to help.
As for the patches, I have skimmed them but quite honestly they won't get my undivided attention until you think they are done and work. That is when I will get out my fine toothed comb. If you want/need feedback breaking thing sup into tiny little bites would make goving it easier for me.
As for the patches, I have skimmed them but quite honestly they won't get my undivided attention until you think they are done and work. That is when I will get out my fine toothed comb. If you want/need feedback breaking thing sup into tiny little bites would make goving it easier for me.