Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for filtering by quest experience rewards (skill filtering) #1283

Closed
wants to merge 3 commits into from

Conversation

tempest-sol
Copy link

@tempest-sol tempest-sol commented Sep 21, 2023

sorting

@tempest-sol tempest-sol marked this pull request as ready for review September 21, 2023 20:44
Copy link
Contributor

@pajlada pajlada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as expected for me!
I have a few naming/style nitpicks that you can take with a handful of salt

@@ -206,6 +208,78 @@ public static QuestFilter[] displayFilters()
}
}

enum SkillFilter implements Predicate<QuestHelper>
{
ANY(true),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: rename so its display name is "Show All", this way it's consistent with the base quest filter

enum SkillFilter implements Predicate<QuestHelper>
{
ANY(true),
ATTACK,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bit of a random suggestion, but you could use a constructor that takes a RuneLite Skill that you get the display name from, which also means you don't need to call Skill.valueOf since you already have the underlying skill
.
It would mean this line would look something like this (probably doesn't compile:

		ATTACK(Skill.ATTACK),

and the SkillFilter constructor looking something like this:

// Constructor for meta filters
SkillFilter(String displayName)
{
    this.skill = null;
    this.displayName = displayName;
    this.predicate = q -> true;
}

// Constructor for skills
SkillFilter(Skill skill)
{
    this.skill = skill;
    this.displayName = skill.getName();
	this.predicate = q -> {
		List<ExperienceReward> experienceRewards = q.getQuest().getQuestHelper().getExperienceRewards();
		if (experienceRewards != null)
		{
			return experienceRewards.stream().anyMatch(reward -> reward.getSkill() == this.skill);
		}
		else
		{
			return false;
		}
	};
}

return predicate.test(quest);
}

public List<QuestHelper> test(Collection<QuestHelper> helpers)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this function looks unused

@Override
public boolean test(QuestHelper quest)
{
return predicate.test(quest);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: consistent with this. prefix

Suggested change
return predicate.test(quest);
return this.predicate.test(quest);

@@ -616,4 +690,16 @@ default boolean showCompletedQuests()
{
return false;
}

@ConfigItem(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open question: Should this filter stick around after a restart?

If it does, the name (& key name) should be changed to match its name in the main panel

@@ -307,7 +312,7 @@ protected void startUp() throws IOException

final BufferedImage icon = Icon.QUEST_ICON.getImage();

panel = new QuestHelperPanel(this);
panel = new QuestHelperPanel(this, skillIconManager);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Since skillIconManager is unused atm, it might be best to remove this requirement for now

@@ -78,6 +81,7 @@ public class QuestHelperPanel extends PluginPanel

private final JPanel allDropdownSections = new JPanel();
private final JComboBox<Enum> filterDropdown, difficultyDropdown, orderDropdown;
private final JComboBox<Enum> skillSelectionPanel;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Rename to be consistent with the other dropdowns

Suggested change
private final JComboBox<Enum> skillSelectionPanel;
private final JComboBox<Enum> skillFilterDropdown;

@@ -297,11 +301,16 @@ public void changedUpdate(DocumentEvent e)
JPanel orderPanel = makeDropdownPanel(orderDropdown, "Ordering");
orderPanel.setPreferredSize(new Dimension(PANEL_WIDTH, DROPDOWN_HEIGHT));

skillSelectionPanel = makeNewDropdown(QuestHelperConfig.SkillFilter.values(), "skillReward");
JPanel skillSortPanel = makeDropdownPanel(skillSelectionPanel, "Skill Filtering");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd personally prefer if this was labeled Skill Filters (consistent with main Filters), or Skill Filter

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants