Feature - Implemented Make Maximizer Filters checkboxes instead of radio buttons

gausie

gausie

D̰͕̝͚̤̥̙̐̇̑͗̒e͍͔͎͈͔ͥ̉̔̅́̈l̠̪̜͓̲ͧ̍̈́͛v̻̾ͤe͗̃ͥ̐̊ͬp̔͒ͪ
Staff member
MCroft said:
Patch for review: View attachment 9912

Done: add filter checkboxes and add set all filters/clear filters buttons.

Done: Add filter field to results, so it's more like other panes.
Not sure if it's useful, but it's easy and might help. My best use case for it is "show me all the +20 ML results". Change to Scrollable Filtered Panel also affects Museum, but not in a bad way.​

Done: Removing the Equipment: none radio button, since it's redundant with unchecking the equipment checkbox.
This may solve gausie's problem of not seeing "pullable/buyable" by making that row shorter. Downside of that: Removing the checkbox in the MaximizerFrame changes the Index of the options in the radio button. MaximizerFrame talks to the Maximizer by sending it that index as "equipLevel".

While I'd love to go in and rip out all of the "equipLevel" code and pass an Enum ("Enums are Integers, except when they're not") that's not on the plate for tonight, and I don't want to mess up the gCLI/ASH by changing the all those "if equipLevel == 1" lines to 0, or other fun things.

Immediate inelegant solution is to send RadioButtonGroupIndex+1...
​


Out of Scope: Anything about changing sort order
Out of Scope: Fixing the existing bug I'm about to report on text field focus when starting with a frame open.
Click to expand...

This looks great MCroft, and works on my machine. You removed the copyright block in MaximizerFrame.java but I could reintroduce it before committing it.

For whatever reason though I feel like I should wait for Veracity to review it before merging - perhaps I'm just not confident with providing decent review for the very Javay bits of mafia
 
MCroft

MCroft

Developer
Staff member
gausie said:
This looks great MCroft, and works on my machine. You removed the copyright block in MaximizerFrame.java but I could reintroduce it before committing it.

For whatever reason though I feel like I should wait for Veracity to review it before merging - perhaps I'm just not confident with providing decent review for the very Javay bits of mafia
Click to expand...
I didn’t mean to remove it. It called it a “dangling javadoc” and wanted it inside the package, but I thought I undid applying that change. Does IJ have it inside a fold?

If it needs more review, that’s fine. It’s not a trivial change like some of my other ones. If it needs to wait until the next milestone release, I want people to be confident in it. And we can use it from source if we want to until then.
 
fronobulax

fronobulax

Developer
Staff member
I will look. I hate Swing but I can figure out some of the more egregious issues.

Given that the code base of KoLmafia spans a decade I find that IntelliJ's lint like analysis cannot be blindly accepted and applied. The dangling JavaDoc occurs in almost every file and given that JavaDoc is not a standard KoLmafia ever used the check should probably be disabled. Some of the probable bugs aren't and some of the suggested changes because of newer Java require me to look up and see what the version of Java the source must comply with because some of those changes will break that.
 
MCroft

MCroft

Developer
Staff member
yep, failed undo. My bad. That should be reverted. I will do so if there's an second patch. And I'll turn it off in the IDE and the checker. Good suggestion.

For anyone who wants to find this quickly in IJ, it's not part of the "code style", it's in the Inspections.
Preferences>Editor>Inspections>Java>JavaDoc>Dangling JavaDoc Comment
 
Last edited:
MCroft

MCroft

Developer
Staff member
Does anyone else want to review this or should I land it and let it get some run-time before the next release?
 
P

PeKaJe

Member
OK, looks like I stumbled upon a little bug. There's a preference, maximizerEquipmentLevel, which sets default level of item creation. Values are 0-3 for "none", "on hand", "creatable", "pullable/buyable". This is used directly to set the selected index on the "equipment" selection. However, "none" was removed so the default shifted, as I discovered when not paying enough attention and ended up crafting stuff I didn't intend to.
 
MCroft

MCroft

Developer
Staff member
PeKaJe said:
OK, looks like I stumbled upon a little bug. There's a preference, maximizerEquipmentLevel, which sets default level of item creation. Values are 0-3 for "none", "on hand", "creatable", "pullable/buyable". This is used directly to set the selected index on the "equipment" selection. However, "none" was removed so the default shifted, as I discovered when not paying enough attention and ended up crafting stuff I didn't intend to.
Click to expand...

I'll take a look. I removed "none" from "Equipment" because it was redundant with this patch and pullable/buyable was not visible to some users due to the LookAndFeel value. I pulled it from the pref to match the new values.
 
MCroft

MCroft

Developer
Staff member
MCroft said:
I'll take a look. I removed "none" from "Equipment" because it was redundant with this patch and pullable/buyable was not visible to some users due to the LookAndFeel value. I pulled it from the pref to match the new values.
Click to expand...
OK, I have an approach
1: new Prefs: maximizerSelectedFilters and maximizerEquipmentScope
2: On create, if maximizerSelectedFilters and maximizerEquipmentScope have no state and maximizerEquipmentLevel does, convert maximizerEquipmentLevel to maximizerEquipmentScope and abandon Level.
3: On Maximizer Update Button, save maximizerSelectedFilters and maximizerEquipmentScope.
4: since the maximizer won't let you eat or drink if you're at max, disable rather than uncheck the boxes.
5: set a default for the new prefs.
6: change prefs to set new value.

Downside: people who ran since Revision 20488 will get a one-time descoping

Now I just gotta code it.
 
P

PeKaJe

Member
MCroft said:
4: since the maximizer won't let you eat or drink if you're at max, disable rather than uncheck the boxes.
Click to expand...
Please don't, there are times I want to know what is possible, even if it isn't right now. Uncheck is fine, disable is not.
 
MCroft

MCroft

Developer
Staff member
PeKaJe said:
Please don't, there are times I want to know what is possible, even if it isn't right now. Uncheck is fine, disable is not.
Click to expand...
I'll check before I do anything, but I thought the maximizer skipped eating/drinking if you can't eat/drink.
 
P

PeKaJe

Member
Huh, OK, perhaps it does and I just patched it out of my own build (it's been a while since I touched those parts). I know I did something to make it still give suggestions for stuff to acquire, despite having settings to disallow buying.
 
Boesbert

Boesbert

Member
I am usually just checking equip and cast because those use no resources (equipment) or a neglible one (MP). So the change made it necessary for me to click twice for switching from one to the other (uncheck equip, check cast, and vice versa) instead of just one click with the radio button system. As such, the change is a negative for my user experience.

Another thing that's worse is that the new version does not seem to remember the equipment radio button anymore and always defaults back to "creatable" between log-ins. I have overlooked this and created ice nines by accident when setting up for rollover, mistakenly assuming that it would have remembered the "on hand" setting.
 
MCroft

MCroft

Developer
Staff member
On hand should be remembered, I'll verify that when I work on the issue PeKaJe identified. I'll have to think about a way to reduce clicks. I was thinking that case might be covered by selecting both, since they're segregated top/bottom.

I assume this is all through the GUI, not a combination of scripting/CLI/GUI?
 
R

Ryo_Sangnoir

Developer
Staff member
I suspect defaulting to "creatable" is due to maximizerEquipmentLevel defaulting to 1, which is "creatable" now that "none" is removed.

kolmafia/swingui/MaximizerFrame.java:

Code: 
MaximizerFrame.this.equipmentSelect.setSelectedIndex( Preferences.getInteger( "maximizerEquipmentLevel" ) );

And searching for that string shows me that Preferences -> General -> Maximizer still has the old four radio buttons set.
 
Last edited:
MCroft

MCroft

Developer
Staff member
Ryo_Sangnoir said:
Code: 
MaximizerFrame.this.equipmentSelect.setSelectedIndex( Preferences.getInteger( "maximizerEquipmentLevel" ) );

And searching for that string shows me that Preferences -> General -> Maximizer still has the old four radio buttons set.
Click to expand...
bugger. missed that in the changeset. Well, that will be in the fix I'm working on.
 
P

PeKaJe

Member
Looking a little deeper, it seems the maximizer does show consumables you can't currently use, though greyed out. I'm pretty sure that's not a personal patch of mine, at least I can't seem to find it. So disabling the checkboxes would remove functionality. Also, clicking around a bit on the checkboxes and re-running the maximization gives ... odd results. Like there's a spleen item that shows up no matter what I selected, sometimes the boozes also show up despite not being selected, but I can't see a pattern to it. I'm thinking something's cached and not re-calculated/re-filtered.
 
MCroft

MCroft

Developer
Staff member
I know it's not resetting, because it's not re-fetching values. More after I dig in.
 
Boesbert

Boesbert

Member
MCroft said:
On hand should be remembered, I'll verify that when I work on the issue PeKaJe identified. I'll have to think about a way to reduce clicks. I was thinking that case might be covered by selecting both, since they're segregated top/bottom.

I assume this is all through the GUI, not a combination of scripting/CLI/GUI?
Click to expand...

Yes, GUI.
 
You must log in or register to reply here.
Top