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

ItemList Changes #1119

Merged
merged 6 commits into from
Jan 5, 2025
Merged

ItemList Changes #1119

merged 6 commits into from
Jan 5, 2025

Conversation

Manchick0
Copy link
Contributor

This update introduces a bunch of changes to the ItemList feature, to hopefully enhance user experience. Most importantly, you can now filter out specific elements in the list to narrow down the results. The lore search now works properly, and you're now able to use the wiki lookup key in the recipe book. Besides all that, shift-clicking on the recipe result button will suggest the recipe to the player using the viewrecipe command, to allow quick super-craft access.

P.S. The second commit only exists because I forgot to check those files in the first one.

This update introduces a bunch of changes to the ItemList feature, to hopefully enhance user experience. Most importantly, you can now filter out specific elements in the list to narrow down the results. The lore-search now works properly, and you're now able to use the wiki lookup key in the recipe book. Besides all that, shift-clicking on the recipe result button will suggest the recipe to the player using the `viewrecipe` command, to allow quick super-craft access.
@LifeIsAParadox LifeIsAParadox added the reviews needed This PR needs reviews label Dec 31, 2024
Manchick0 and others added 2 commits January 1, 2025 15:07
Properly use parentheses in the `(animal)` and `(pest)` flagged entries.
Replaces direct chat command handling with MessageScheduler.
Copy link
Collaborator

@kevinthegreat1 kevinthegreat1 left a comment

Choose a reason for hiding this comment

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

Overall pretty good logic, you can decide to accept the nit picks as you see fit.

@LifeIsAParadox LifeIsAParadox added changes requested This PR need changes and removed reviews needed This PR needs reviews labels Jan 2, 2025
Replaced the Identifiable interface with a Supplier-based design for enum, updated related classes to use Supplier methods and refactored filtering logic for consistency. Additionally, added a new tip for viewing recipes in the recipe book, and changed the Shift+Click requirement to a simple right click.
@LifeIsAParadox LifeIsAParadox added reviews needed This PR needs reviews and removed changes requested This PR need changes labels Jan 2, 2025
Copy link
Collaborator

@kevinthegreat1 kevinthegreat1 left a comment

Choose a reason for hiding this comment

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

Just one more question.

var rawID = ItemUtils.getItemId(result.getDisplayStack());
if (result.isMouseOver(mouseX, mouseY)) {
MessageScheduler.INSTANCE.sendMessageAfterCooldown(String.format("/viewrecipe %s", rawID), true);
return true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice catch I overlooked this.

kevinthegreat1
kevinthegreat1 previously approved these changes Jan 3, 2025
Copy link
Collaborator

@kevinthegreat1 kevinthegreat1 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@LifeIsAParadox LifeIsAParadox added merge me please Pull requests that are ready to merge and removed reviews needed This PR needs reviews labels Jan 3, 2025
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think the asserts are necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They just fix all the warning in the IDE

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it makes sense to use asserts as "I know this won't be null, now stop giving me warnings."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it makes sense to use asserts as "I know this won't be null, now stop giving me warnings."

Or even with other boolean conditions, such as when the children method returns a list of three, you know for sure its size is going to be 3, so you can assert children().size() == 3 - It's just cleaner than @SurpressWarnings

@LifeIsAParadox LifeIsAParadox added changes requested This PR need changes and removed merge me please Pull requests that are ready to merge labels Jan 3, 2025
@LifeIsAParadox LifeIsAParadox added reviews needed This PR needs reviews and removed changes requested This PR need changes labels Jan 3, 2025
@Manchick0 Manchick0 requested a review from AzureAaron January 3, 2025 21:47
@LifeIsAParadox LifeIsAParadox added merge me please Pull requests that are ready to merge and removed reviews needed This PR needs reviews labels Jan 4, 2025
@kevinthegreat1 kevinthegreat1 merged commit 8575fb8 into SkyblockerMod:master Jan 5, 2025
1 check passed
@LifeIsAParadox LifeIsAParadox removed the merge me please Pull requests that are ready to merge label Jan 5, 2025
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.

4 participants